Repository navigation
Performance regression in v8.10 and v9.0 for Maps with object keys #19769
Description
- Version: >= v8.10.0, >= v9.0.0
- Platform: multiple (tested on
Ubuntu 14.04.5 LTS (GNU/Linux 3.13.0-129-generic x86_64)andmacOS 10.13.3 (17D102) Darwin 17.4.0) - Subsystem: V8
Compared to v8.9.4 there's a major performance regression in v8.10.0 and v9.0.0 for Map instances that use objects as keys. The issue is making our application unacceptably slow, forcing us to downgrade.
To be more specific, the get, set, and has methods of Map instances with several hundred (or more) keys have become very slow when those keys are objects (and exist in the map). The performance for string and number keys (the two primitive types that I've tested) seems to be fine.
Comparing run time of the script below between Node.js v8.9.4 and v8.10.0 demonstrates the problem.
'use strict';
const runCount = 100;
const keyCount = 10000;
let map = new Map();
// Create an array of unique objects used as keys below
let keys = new Array(keyCount);
for (let i = 0; i < keyCount; i++) keys[i] = {};
// Create a map entry for each key
for (let key of keys) map.set(key, true);
let startTime = process.hrtime();
// Perform runCount rounds in which each key is looked up
for (let i = 0; i < runCount; i++) {
for (let key of keys) {
// This call below is the one we're benchmarking. We're using
// `map.get` here, but `map.set` and `map.has` show a similar
// performance regression.
let value = map.get(key);
// Do something with the value to make sure that
// the get call isn't being optimized away
if (value !== true) throw new Error();
}
}
// Print the Node.js version and elapsed time in milliseconds
let elapsed = process.hrtime(startTime);
let [seconds, nanoseconds] = elapsed;
let milliseconds = Math.round(seconds * 1e3 + nanoseconds / 1e6);
console.log(`${process.version} ${milliseconds} ms`);The performance difference becomes more pronounced as the size of the map (keyCount) increases. My system produced these figures for the supplied parameters:
v8.9.4 88 ms
v8.10.0 3980 ms
This issue could perhaps be the cause of #19444.
Activity
vsemozhetbyt commentedon Apr 3, 2018
Node.js v8.9.4 has V8 6.1, while Node.js v8.10.0 and v9.0.0 have V8 6.2, so it may be V8 regression.
vsemozhetbyt commentedon Apr 3, 2018
cc @nodejs/v8
targos commentedon Apr 3, 2018
This is fixed in current V8. Using our canary builds, I can say the fix is between V8 versions 6.3.292 and 6.3.296
vsemozhetbyt commentedon Apr 3, 2018
Are we planning to upgrade Node.js v8.x LTS to V8 6.3?
targos commentedon Apr 3, 2018
@vsemozhetbyt There is no plan to do it. v9.x was never upgraded to this version.
This is very likely fixed by this commit: v8/v8@a803fad
vsemozhetbyt commentedon Apr 3, 2018
So we can only advise upgrading to Node.js v10 after the release in a month in this situation, right?
vsemozhetbyt commentedon Apr 3, 2018
In my machine:
So after upgrading to Node.js v10, there may be a 3x performance gain comparing to v8.9.4.
targos commentedon Apr 3, 2018
We can try to backport the fix. I'm doing that right now.
targos commentedon Apr 3, 2018
#19770
deps: cherry-pick a4bddba from upstream V8
deps: cherry-pick 7abdadc from upstream V8
deps: cherry-pick a803fad from upstream V8
targos commentedon Apr 5, 2018
Fix landed on
v9.x-staging. PR to v8.x-staging: #19824deps: cherry-pick a4bddba from upstream V8
deps: cherry-pick 7abdadc from upstream V8
deps: cherry-pick a803fad from upstream V8
targos commentedon Apr 27, 2018
Fix landed and will be available in the next release
nkreeger commentedon Apr 29, 2018
Hi @targos thank you for the update - do you have an ETA on the next release? Is there a place I can track this? I work on TensorFlow.js and our platform uses WeakMap for internal references to tensor data. We have a node.js binding that we plan on shipping soon, but we'd like to have the node release handy (long training loops are affected w/ this bug):
https://github.com/tensorflow/tfjs-node
https://github.com/tensorflow/tfjs-core
Koslun commentedon Apr 30, 2018
Thanks for the great work @targos !
Going from the the past half year or so I presume a new release is coming between now and the coming two months or so but if anyone has a better guess I'd greatly welcome it. Good to know it'll be any 8.x release higher than 8.11.1.
As described in this medium article it also affects Webpack 4 users as Webpack 4 relies on SortableSet internally. In that example switching back to Node.js 8.9.4 improved build times from 6s to 4.5s. I would imagine this changing wildly depending on your configuration but should affect all Webpack 4 users noticeably.
targos commentedon May 10, 2018
According to #20478, the release will be on 2018-05-18.
Koslun commentedon May 10, 2018
@targos Thanks, not too far off then.
build(webpack): Fix incremental webpack builds (upgrade to node@8.15.1)
build(webpack): Fix incremental webpack builds (upgrade to node@8.15.1)
build(webpack): Fix incremental webpack builds (upgrade to node@8.15.…
build(webpack): Fix incremental webpack builds (upgrade to node@8.15.…