Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[hack pipes] Inline topic token when possible #14278

Merged
merged 4 commits into from Feb 19, 2022

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Feb 16, 2022

Q                       A
Fixed Issues?
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

I discovered that "removing unnecessary tmp vars" is quite satisfying 😁

When a topic token is only used once and either:

  • the lhs of |> is pure
  • all the operations in the rhs of |> before evaluating the topic token are pure
    the pipeline operator can be compiled without temporary variables, by directly inlining the lhs in the rhs.

cc @js-choi if you have time to review, since you initially implemented this plugin!

@nicolo-ribaudo nicolo-ribaudo added Spec: Pipeline Operator PR: Output optimization 🔬 A type of pull request used for our changelog categories labels Feb 16, 2022

return _ref4 = n, (_ref3 = Math.abs(_ref4), (_ref2 = Promise.resolve(_ref3), (_ref = await _ref2, triple(_ref))));
return _ref = Math.abs(n), triple(await Promise.resolve(_ref));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't fully inlined because Promise.resolve might be an impure getter. Since Babel assumes unmodified builtins, in Babel 8 we could expand this "implicit assumptions" to also mark this as pure and transform it to

return triple(await Promise.resolve(Math.abs(n)));

@@ -1,7 +1,7 @@
let i = 0;
let sum = 0;

while (i |> (i = ^ + 1) |> ^ <= 10)
while (i |> (i = 2 * ^ - ^ + 1) |> ^ <= 10)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just to keep the "spirit" of the test, otherwise it was completely inlined.

@babel-bot
Copy link
Collaborator

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/51308/

!state.sideEffectsBeforeFirstTopicReference &&
!path.isPure()
) {
state.sideEffectsBeforeFirstTopicReference = true;
Copy link
Contributor

@js-choi js-choi Feb 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is sideEffectsBeforeFirstTopicReference invalidated only when side effects occur before the first topic reference in an RHS? For example, will it be set to true for x |> (f(x), g(x)) x |> (f(#), g(#))?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you mean x |> (f(#), g(#)) and that f/g are defined and not reassigned. It will not be set to true because there are no side effects before the first #. However, since there are multiple #s we won't inline anything.

I only tracked the side effects before the first one, so that x |> (#, SIDE_EFFECT) doesn't prevent inlining.

}
}
},
"ClassBody|Function"(_, state) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Class body does not encapsulate all expressions under class, for example: class decorators.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's exactly why I used ClassBody: class decorators evaluation is not deferred!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If so then we have to exclude computed class element keys, too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that exactly determining what runs "now" is hard. The list includes (but is not limited to):

  • class keys
  • static fields
  • iifes

To avoid making the transform too complex, I'd prefer to leave it as-is and deoptimize when they happen.

@nicolo-ribaudo nicolo-ribaudo merged commit a35af3e into babel:main Feb 19, 2022
@nicolo-ribaudo nicolo-ribaudo deleted the optimize-hack-tmp-vars branch February 19, 2022 11:38
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label May 22, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Output optimization 🔬 A type of pull request used for our changelog categories Spec: Pipeline Operator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants