Extract for-of iterator handling to a helper #11262

nicolo-ribaudo · 2020-03-14T23:26:08Z

Q	A
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?
License	MIT

This greatly reduces the code size (*) when for-of is used multiple times across the
codebase. Also, makes it easier to read what's happening in a compiled loop.

(*) I tested the difference with 3 for loops before adding the human-friendly error message about the missing Symbol.iterator. I didn't use transform-runtime or external-helpers, so these numbers include the helper size.

These are the minified sizes (with terser):

	spec	loose
old	563 bytes	465 bytes
new	457 bytes	349 bytes

Since the error message is a bit more than 100 bytes long, the size improvement probably needs 4 loops rather than one now. However, keep in mind that if I added the human-friendly error message to the old implementation it would have added 100 bytes 3 times, making the size improvement even bigger.

Size test code

loop: for (let x of a) {
  break loop;
}
for (var x of a) {
  a = null;
}
for (x of a) {
  continue;
}

Part 1/n

nicolo-ribaudo · 2020-03-15T12:09:28Z

packages/babel-plugin-transform-for-of/src/no-helper-implementation.js

+// It must be kept for compatibility reasons.
+// TODO (Babel 8): Remove this code.
+
+export default function transformWithoutHelper(loose, path, state) {


This file contains the old plugin code needed when the helper isn't available. I just copy-pasted it without any edit.

nicolo-ribaudo · 2020-03-15T12:13:45Z

packages/babel-plugin-transform-block-scoping/test/fixtures/general/hoisting/output.js


 try {
-  for (var _iterator = nums[Symbol.iterator](), _step; !(_iteratorNormalCompletion = (_step = _iterator.next()).done); _iteratorNormalCompletion = true) {
+  for (_iteratorHelper.s(); !(_step = _iteratorHelper.n()).done;) {
    var i = _step.value;
    var x;
    var f;


This is a bug unrelated to this PR: f and x should be declared in the _loop function.

I think they have to stay here. If we moved the var inside the _loop function, they would no longer declare the variable in the function scope (it's not a block let/const).

jridgewell · 2020-03-16T01:16:31Z

packages/babel-helpers/src/helpers.js

+
+helpers.createForOfIteratorHelperLoose = helper("7.9.0")`
+  export default function _createForOfIteratorHelperLoose(o) {
+    var i = 0;


Nit: Don't share the variable here.

I mostly did it for code size, since this is a runtime helper, and i could stand both for index and iterator 😅

jridgewell · 2020-03-16T01:21:31Z

packages/babel-plugin-transform-for-of/src/index.js

+
+        const nodes = builder.build({
+          CREATE_ITERATOR_HELPER: state.addHelper(builder.helper),
+          ITERATOR_HELPER: scope.generateUidIdentifier("iteratorHelper"),


Nit:

Suggested change

ITERATOR_HELPER: scope.generateUidIdentifier("iteratorHelper"),

ITERATOR_HELPER: scope.generateUidIdentifier("iterator"),

jridgewell · 2020-03-16T01:28:14Z

packages/babel-plugin-transform-for-of/src/index.js

-        // push the rest of the original loop body onto our new body
-        block.body = block.body.concat(node.body.body);
+        t.inherits(container[0], node);
+        t.inherits(container[0].body, node.body);


Does container ever contain more than one element? If not, we can replace the replaceWithMultiple with replaceWith and drop the parent.isLabeledStatement() check.

container always contains a single node. However:

We are doing replaceWithMultiple(nodes), and in non-loose mode nodes contains 2 nodes (the var declaration and the try statement)

We can't drop the parent.isLabeledStatement() check because in non-loose mode it the label must be moved to the inner loop. If we only did path.replaceWith() it would be attached to the try statement.

If we only did path.replaceWith() it would be attached to the try statement.

I think that would actually be equivalent. The finally block should still fire correctly.

But then, it doesn't work with continue.

Ohhh. I was only thinking about break.

JLHwung · 2020-03-16T18:33:16Z

packages/babel-plugin-transform-for-of/src/index.js

+        );
+
+        if (
+          t.isIdentifier(left) ||


nit: It suffices to check t.isVariableDeclaration first and assuming it's LVal in the else branch, because we have validated ForOfStatement.left even when BABEL_TYPES_8_BREAKING is not enabled:

babel/packages/babel-types/src/definitions/es2015.js

Line 333 in 1ba41f2

return assertNodeType("VariableDeclaration", "LVal");

(Oh it is copied from the old codes.)

Dis greatly recudes the code size when for-of is used multiple times across the codebase. Also, makes it easier to read what's happening in a compiled loop.

nicolo-ribaudo added the PR: Internal 🏠 A type of pull request used for our changelog categories label Mar 14, 2020

nicolo-ribaudo force-pushed the iterables-dx/for-of-helper branch from 2c08b9b to 40ce663 Compare March 14, 2020 23:34

nicolo-ribaudo mentioned this pull request Mar 14, 2020

Add for-of fallback for arrays in browsers without symbol support #11263

Merged

1 task

nicolo-ribaudo added this to Ready for review in Iterables DX Mar 14, 2020

nicolo-ribaudo commented Mar 15, 2020

View reviewed changes

nicolo-ribaudo force-pushed the iterables-dx/for-of-helper branch from d556229 to f505349 Compare March 15, 2020 12:10

nicolo-ribaudo commented Mar 15, 2020

View reviewed changes

nicolo-ribaudo marked this pull request as ready for review March 15, 2020 13:58

jridgewell approved these changes Mar 16, 2020

View reviewed changes

JLHwung reviewed Mar 16, 2020

View reviewed changes

JLHwung approved these changes Mar 16, 2020

View reviewed changes

nicolo-ribaudo added 5 commits March 17, 2020 00:40

Extract for-of iterator handling to a helper

2f4220a

Dis greatly recudes the code size when for-of is used multiple times across the codebase. Also, makes it easier to read what's happening in a compiled loop.

Unify spec and loose code

986a3d7

Legacy implementation fallback

054cf39

Update tmp var name

8aadff9

Updates from review and after rebase

771986f

nicolo-ribaudo force-pushed the iterables-dx/for-of-helper branch from 4173b6c to 771986f Compare March 16, 2020 23:45

nicolo-ribaudo merged commit 5997668 into master Mar 16, 2020

Iterables DX automation moved this from Ready for review to Done Mar 16, 2020

snyk-bot mentioned this pull request Apr 11, 2020

[Snyk] Upgrade @babel/runtime from 7.8.7 to 7.9.0 wreiske/asteriskmonitor#16

Merged

github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jun 16, 2020

github-actions bot locked as resolved and limited conversation to collaborators Jun 16, 2020

nicolo-ribaudo deleted the iterables-dx/for-of-helper branch June 18, 2020 23:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract for-of iterator handling to a helper #11262

Extract for-of iterator handling to a helper #11262

nicolo-ribaudo commented Mar 14, 2020 •

edited

nicolo-ribaudo Mar 15, 2020

nicolo-ribaudo Mar 15, 2020

jridgewell Mar 16, 2020

jridgewell Mar 16, 2020

nicolo-ribaudo Mar 16, 2020

jridgewell Mar 16, 2020

jridgewell Mar 16, 2020

nicolo-ribaudo Mar 16, 2020

jridgewell Mar 16, 2020

nicolo-ribaudo Mar 16, 2020

jridgewell Mar 16, 2020

JLHwung Mar 16, 2020 •

edited

	ITERATOR_HELPER: scope.generateUidIdentifier("iteratorHelper"),
	ITERATOR_HELPER: scope.generateUidIdentifier("iterator"),

Extract for-of iterator handling to a helper #11262

Extract for-of iterator handling to a helper #11262

Conversation

nicolo-ribaudo commented Mar 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JLHwung Mar 16, 2020 • edited

Choose a reason for hiding this comment

nicolo-ribaudo commented Mar 14, 2020 •

edited

JLHwung Mar 16, 2020 •

edited