From de72ce6ce70c6b0c337a926314f34d9a1dca8248 Mon Sep 17 00:00:00 2001 From: Benedikt Meurer Date: Mon, 6 Nov 2017 13:23:20 +0100 Subject: [PATCH] Changed for..in loops to iterating through Object.keys, so only own properties gets processed (#6748) * Properly guard for..in loops with Object#hasOwnProperty. I noticed that babylon spends a lot of time in what we call *slow mode* `for..in` when running in Node (on V8), and the reason for that is that the version distributed on npm is build with *loose mode*, which turns methods on the prototype into enumerable properties. Let's look at a simplified example of the `State` class from `src/tokenizer/state.js`: ```js class State { constructor() { this.x = 1; } clone() { var state = new State(); for (var key in this) { var val = this[key]; state[key] = val; } return state; } } ``` According to the specification the `State.prototype.clone` method is non-enumerable. However when transpiling this with loose mode, we get the following output: ```js var State = (function() { function State() { this.x = 1; } State.prototype.clone = function clone() { var state = new State(); for (var key in this) { var val = this[key]; state[key] = val; } return state; } return State; })(); ``` So all of a sudden the `State.prototype.clone` method is enumerable. This means that the `for..in` loop inside of that method enumerates `x` and `clone` for `key`, whereas originally it was supposed to only enumerate `x`. This in turn means that the shape of the result of a call to `clone` will be different than the shape of a state that is created via the `State` constructor. You can check this in `d8` using the `--allow-natives-syntax` flag and this simple test driver: ```js const s = new State; %DebugPrint(s); %DebugPrint(s.clone()); ``` Using either the class version or the transpiled version we see: ``` $ out/Release/d8 --allow-natives-syntax state-original.js 0x2a9d7970d329 0x2a9d7970d3c1 $ out/Release/d8 --allow-natives-syntax state-loose.js 0x3729ee30d1b9 0x3729ee30d251 ``` So as you can see, the transpiled version (using *loose mode*) produces a different shape for the result of `clone`, whereas the original version is fine. This pollutes all sites which use either a state created from the `State` constructor or returned from the `clone` method. The original one has only the `x` property in either case, whereas in the transpiled version the result of `clone` has properties `x` and `clone` on the instance. To mitigate this effect, it's best to guard the `for..in` loops with `Object.prototype.hasOwnProperty` calls, such that the actual body of the loop only deals with own properties and not with properties from the prototype chain. This change does exactly that for the two affected `clone` functions. In addition to the performance hit because of the unnecessary polymorphism, there's also the performance hit because of the *slow mode* `for..in` itself, which has to collect the properties from the instance plus the prototype. Ideally the prototype properties shouldn't be enumerable to avoid this whole set of problems. I see a couple of possible solutions: 1. Distribute the original ES2015 version via npm. 2. Don't use loose mode, so that `Object.defineProperty` is used instead, correctly passing `enumerable:false`. 3. Globally change loose mode in Babel to generate the correct and fast `Object.defineProperty` instead. I'd personally prefer a combination of 1. and 3. here, but I'm aware that distributing the ES2015 code might not be an option yet. So the mitigation of properly guarding the `for..in` here should already help. But it'd be nice to have a discussion on using `Object.defineProperty` in general, as I imagine that this could easily bite other applications as well and this performance cliff is completely unobvious to developers. * Switch to Object.keys and Array.prototype.forEach. --- packages/babylon/src/parser/node.js | 4 ++-- packages/babylon/src/tokenizer/state.js | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/babylon/src/parser/node.js b/packages/babylon/src/parser/node.js index c5bfb2a82b..3ddd6038c2 100644 --- a/packages/babylon/src/parser/node.js +++ b/packages/babylon/src/parser/node.js @@ -32,13 +32,13 @@ class Node implements NodeBase { __clone(): this { // $FlowIgnore const node2: any = new Node(); - for (const key in this) { + Object.keys(this).forEach(key => { // Do not clone comments that are already attached to the node if (commentKeys.indexOf(key) < 0) { // $FlowIgnore node2[key] = this[key]; } - } + }); return node2; } diff --git a/packages/babylon/src/tokenizer/state.js b/packages/babylon/src/tokenizer/state.js index e2ad59f9f8..0041dcfbf8 100644 --- a/packages/babylon/src/tokenizer/state.js +++ b/packages/babylon/src/tokenizer/state.js @@ -185,7 +185,7 @@ export default class State { clone(skipArrays?: boolean): State { const state = new State(); - for (const key in this) { + Object.keys(this).forEach(key => { // $FlowIgnore let val = this[key]; @@ -195,7 +195,7 @@ export default class State { // $FlowIgnore state[key] = val; - } + }); return state; } }