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

Use `for..of Object.keys` instead of `for..in` #9518

Merged
merged 4 commits into from Feb 26, 2019

Conversation

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Feb 15, 2019

Q                       A
Fixed Issues? Fixes #9511
Patch: Bug Fix? Yes
Tests Added + Pass? Yes
License MIT

In #9511 (and #9495 is another symptom), @PavelKastornyy reported a node crash becaue the JavaScript heap run out of memory. The problem was that their code was adding enumerable properties to Object.prototype: it is something that shouldn't be done, but Babel shouldn't make node crash if someone adds them.
I reduced down the problem to for...in loops in @babel/traverse that grew the memory consumption exponentially because of that unexpected properties.

@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Feb 15, 2019

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

@@ -98,7 +99,7 @@ export function push(
}

export function hasComputed(mutatorMap: Object): boolean {
for (const key in mutatorMap) {
for (const key in own(mutatorMap)) {

This comment has been minimized.

Copy link
@loganfsmyth

loganfsmyth Feb 15, 2019

Member

Do we need the macro? It's pretty easy to do for (const key of Object.keys(mutatorMap)) {.

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Feb 15, 2019

Author Member

I already had the macro in another project so I published it and added there, but I can remove it if you prefer.
My fear is that for ... of + Object.keys is slower than for ... in + Object.hasOwnProperty, and it is used in some perf-sensitive places in @babel/traverse.

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Feb 15, 2019

Author Member

I was wrong https://jsperf.com/for-in-vs-for-ok/1, they are about the same. Til [Symbol.iterator] is super fast.

@nicolo-ribaudo nicolo-ribaudo force-pushed the nicolo-ribaudo:for-in-hasOwnProperty branch from 9b2dadf to 7ceb6fe Feb 15, 2019
@nicolo-ribaudo nicolo-ribaudo changed the title Wrap for...in loop bodies with hasOwnProperty check Use `for..of Object.keys` instead of `for..in` Feb 15, 2019
@nicolo-ribaudo nicolo-ribaudo force-pushed the nicolo-ribaudo:for-in-hasOwnProperty branch from 7ceb6fe to 6d50487 Feb 15, 2019
@nicolo-ribaudo nicolo-ribaudo force-pushed the nicolo-ribaudo:for-in-hasOwnProperty branch from 6d50487 to a40c2e6 Feb 15, 2019
@@ -23,7 +23,7 @@ export function explode(visitor) {
visitor._exploded = true;

// normalise pipes
for (const nodeType in visitor) {
for (const nodeType of Object.keys(visitor)) {

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Feb 15, 2019

Author Member

@loganfsmyth Since this is a pulic api, should I handle the case where visitor is null/undefined (that for..in handled by default)?

This comment has been minimized.

Copy link
@loganfsmyth

loganfsmyth Feb 15, 2019

Member

Since we're doing visitor._exploded above we shouldn't need to worry about that case here.

@danez
danez approved these changes Feb 16, 2019
@nicolo-ribaudo nicolo-ribaudo merged commit 0345c1b into babel:master Feb 26, 2019
4 checks passed
4 checks passed
babel/repl REPL preview is available
Details
buildsize Significant change of babel.min.js up by 1.84 KB (0.02%)
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nicolo-ribaudo nicolo-ribaudo deleted the nicolo-ribaudo:for-in-hasOwnProperty branch Feb 26, 2019
NMinhNguyen pushed a commit to NMinhNguyen/babel-plugin-transform-destructuring that referenced this pull request Aug 9, 2019
In babel#9511 (and babel#9495 is another symptom), @PavelKastornyy reported a node crash becaue the JavaScript heap run out of memory. The problem was that their code was adding enumerable properties to `Object.prototype`: it is something that shouldn't be done, but Babel shouldn't make node crash if someone adds them.
I reduced down the problem to `for...in` loops in `@babel/traverse` that grew the memory consumption exponentially because of that unexpected properties.
@lock lock bot added the outdated label Oct 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.