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

Lazy load tagged template literal strings #7855

Merged

Conversation

@dczombera
Copy link
Contributor

dczombera commented May 2, 2018

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

This change makes _templateObject lazy loaded. While working on it, I removed the pure annotation because I was not sure if it is still useful when used like this (please correct me if wrong).
I was also not sure if it is recommended to clone templateObject every time it is used as an argument to create a new AST node. In this case, I used the existing code in the file as kind of a guidance and cloned the node each time. I appreciate every bit of feedback! :)

@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented May 2, 2018

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

Copy link
Member

nicolo-ribaudo left a comment

While working on it, I removed the pure annotation because I was not sure if it is still useful when used like this (please correct me if wrong).

It is still needed, otherwise minifiers can't remove transpiled template expressions when they are unused (because they don't know that the helper doesn't produce side effects).

I was also not sure if it is recommended to clone templateObject every time it is used as an argument to create a new AST node.

Yes, it is necessary because reused nodes often lead to bugs 👍

path.replaceWith(
t.callExpression(node.tag, [
t.cloneNode(templateObject),
t.parenthesizedExpression(conditionalAssignment),

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo May 2, 2018

Member

Why is this ParenthesizedExpression needed?

This comment has been minimized.

Copy link
@dczombera

dczombera May 2, 2018

Author Contributor

Actually, it is not. 😄 I didn't realize that this is an assignment expression that returns a value. I will remove it. Thanks for the feedback. 👍

scope.push({
id: templateObject,
init,
// This ensures that we don't fail if not using function expression helpers
_blockHoist: 1.9,

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo May 2, 2018

Member

I think that we can remove this "private" parameter now, because it was needed to initialize the template in the right place (while the var templateObject; statement can be anywhere).

This comment has been minimized.

Copy link
@dczombera

dczombera May 2, 2018

Author Contributor

I had no idea what this was good for that's why I left it. In this case I will remove it.

Do you know if there is anything written down that explains this kind of private parameters?

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo May 2, 2018

Member

I think that all the documentation that we have about _blockHoist is this 😅:

/**
* [Please add a description.]
*
* Priority:
*
* - 0 We want this to be at the **very** bottom
* - 1 Default node position
* - 2 Priority over normal nodes
* - 3 We want this to be at the **very** top
* - 4 Reserved for the helpers used to implement module imports.
*/

Copy link
Member

nicolo-ribaudo left a comment

Thank you!

@dczombera

This comment has been minimized.

Copy link
Contributor Author

dczombera commented May 2, 2018

Thanks for your feedback!

Copy link
Member

existentialism left a comment

👍

"=",
t.cloneNode(templateObject),
lazyLoad,
);

This comment has been minimized.

Copy link
@jridgewell

jridgewell May 3, 2018

Member

Nit: we can optimize a little bit more by doing the assignment inside the logical expression:

tag(_templateObject || (_templateObject = _taggedTemplateLiteral(...)))
annotateAsPure(callExpression);
callExpression._compact = true;

const lazyLoad = t.logicalExpression(

This comment has been minimized.

Copy link
@jridgewell

jridgewell May 3, 2018

Member

I wonder if we'll hit function size deopts with this? Maybe a redefining function?

function _templateObject() {
  const result = _taggedTemplateLiteral(...);
  _templateObject = function() { return result; }
  return result;
}

We use the same pattern for lazy-load imports. /cc @loganfsmyth

This comment has been minimized.

Copy link
@dczombera

dczombera May 3, 2018

Author Contributor

Can you elaborate a bit more on this? I don't understand what you mean by "function size deopts". 😭

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo May 3, 2018

Member

@jridgewell The redefining function for modules is needed because it is used in multiple places. In template literals, it would be used only in one place so it isn't needed.

This comment has been minimized.

Copy link
@jridgewell

jridgewell May 3, 2018

Member

The redefining function for modules is needed because it is used in multiple places

I understand why we do it in imports. This is a different optimization using the same code pattern.

Old V8 (Crankshaft) has a character limit for inlining functions in JIT (even comments count towards this limit!):

// I get called from a lot of places
function test() {
  // My function body is short, so I'll get inlined
  return tag(_templateObject, 1, 2, 3);
}

// But now, we lazy-load:
function test() {
  // We just inlined a really big template string expression,
  // so I cross the max_inlined_source_size limit
  // Guess this function won't be inlined anymore...
  return tag(_templateObject || (_templateObject = _taggedTemplateLiteral(['some really long string'])), 1, 2, 3);
}

Now, V8 switched to Turbofan in 5.9, so it's no big deal in recent releases. But, Chrome < 59 and Node < 8.3 have to worry about this. So, my idea is to reuse the same re-define function pattern to move the long template string out of the user's function, and get us back down to small function bodies.

This comment has been minimized.

Copy link
@dczombera

dczombera May 4, 2018

Author Contributor

Thanks a lot for this explanation. 👍 I will work on implementing the redefining function over the weekend.

@xtuc
xtuc approved these changes May 3, 2018
@dczombera dczombera force-pushed the dczombera:lazy-load-tagged-template-literal-strings branch from e5ae6ec to 9fb85ca May 9, 2018
@dczombera

This comment has been minimized.

Copy link
Contributor Author

dczombera commented May 9, 2018

I finally got to add the redefining function pattern. Feedback welcome!😀

@@ -4,6 +4,14 @@ var _interopRequireDefault = require("@babel/runtime/helpers/interopRequireDefau

var _taggedTemplateLiteral2 = _interopRequireDefault(require("@babel/runtime/helpers/taggedTemplateLiteral"));

var _templateObject = /*#__PURE__*/ (0, _taggedTemplateLiteral2.default)(["foo"]);
function _templateObject() {
const data = /*#__PURE__*/ (0, _taggedTemplateLiteral2.default)(["foo"]);

This comment has been minimized.

Copy link
@jridgewell

jridgewell May 9, 2018

Member

Pure shouldn't be necessary anymore, since the wrapper itself is statically pure.

}
`;

scope.path.unshiftContainer("body", lazyLoad);

This comment has been minimized.

Copy link
@jridgewell

jridgewell May 9, 2018

Member

Actually, do we want to push into the scope and use hoisting?

This comment has been minimized.

Copy link
@loganfsmyth

loganfsmyth May 9, 2018

Member

I think unshift is fine, since we're usually pretty consistent about helpers being at the top of each file, and this is helper-like.

@jridgewell jridgewell merged commit 4260ffd into babel:master May 10, 2018
4 checks passed
4 checks passed
babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 80.81% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jridgewell

This comment has been minimized.

Copy link
Member

jridgewell commented May 10, 2018

We can remove the PURE comment in a followup.

@lock lock bot added the outdated label Oct 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
7 participants
You can’t perform that action at this time.