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

lib: use Object.create(null) directly #11930

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@TimothyGu
Member

TimothyGu commented Mar 19, 2017

Since v8/v8@532c16e (included in v5.6), using Object.create(null) directly is now faster than using a constructor.

Refs: emberjs/ember.js#15001
Refs: https://crrev.com/532c16eca071df3ec8eed394dcebb932ef584ee6

/cc @nodejs/v8

querystring
                                                                 improvement confidence      p.value
 querystring/querystring-parse.js n=100000 type="altspaces"           5.40 %          * 1.400406e-02
 querystring/querystring-parse.js n=100000 type="encodefake"          5.70 %            6.377693e-02
 querystring/querystring-parse.js n=100000 type="encodelast"          3.49 %            1.214386e-01
 querystring/querystring-parse.js n=100000 type="encodemany"          5.08 %          * 1.427018e-02
 querystring/querystring-parse.js n=100000 type="manyblankpairs"     -7.21 %         ** 1.152391e-03
 querystring/querystring-parse.js n=100000 type="manypairs"           8.53 %        *** 6.779383e-10
 querystring/querystring-parse.js n=100000 type="multicharsep"        6.11 %         ** 8.321237e-03
 querystring/querystring-parse.js n=100000 type="multivalue"         12.44 %         ** 1.668042e-03
 querystring/querystring-parse.js n=100000 type="multivaluemany"     19.43 %        *** 1.919249e-09
 querystring/querystring-parse.js n=100000 type="noencode"            6.23 %        *** 2.348627e-04
map-bench
es/map-bench.js millions=3 method="object": 0.39794620430879774
es/map-bench.js millions=3 method="nullProtoObject": 0.41138972726767054
es/map-bench.js millions=3 method="nullProtoLiteralObject": 0.37403618701313923
es/map-bench.js millions=3 method="storageObject": 0.39906914551802625
es/map-bench.js millions=3 method="fakeMap": 0.3576056060343219
es/map-bench.js millions=3 method="map": 0.5858516350899148
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

lib

@cjihrig

Code LGTM. I worry that this will make debugging more difficult though.

@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Mar 20, 2017

Member

There seems to be some regressions in the event benchmarks...

See numbers
                                                     improvement confidence      p.value
 events/ee-add-remove.js n=250000                       -43.29 %        *** 4.520847e-30
 events/ee-emit-multi-args.js n=2000000                   1.57 %            2.325176e-01
 events/ee-emit.js n=2000000                              8.37 %         ** 6.855389e-03
 events/ee-listener-count-on-prototype.js n=50000000    -96.06 %        *** 8.635026e-26
 events/ee-listeners-many.js n=5000000                  -17.51 %        *** 3.849835e-27
 events/ee-listeners.js n=5000000                       -56.36 %        *** 6.473337e-30
 events/ee-once.js n=20000000                           -51.46 %        *** 4.847882e-33
Member

joyeecheung commented Mar 20, 2017

There seems to be some regressions in the event benchmarks...

See numbers
                                                     improvement confidence      p.value
 events/ee-add-remove.js n=250000                       -43.29 %        *** 4.520847e-30
 events/ee-emit-multi-args.js n=2000000                   1.57 %            2.325176e-01
 events/ee-emit.js n=2000000                              8.37 %         ** 6.855389e-03
 events/ee-listener-count-on-prototype.js n=50000000    -96.06 %        *** 8.635026e-26
 events/ee-listeners-many.js n=5000000                  -17.51 %        *** 3.849835e-27
 events/ee-listeners.js n=5000000                       -56.36 %        *** 6.473337e-30
 events/ee-once.js n=20000000                           -51.46 %        *** 4.847882e-33
@TimothyGu

This comment has been minimized.

Show comment
Hide comment
@TimothyGu

TimothyGu Mar 20, 2017

Member

Interesting.

The V8 optimization is mainly designed to accommodate map-like usage of objects, by automatically using so-called "slow" properties based on a hashmap instead of trying to find hidden classes. This makes it work especially well with cases like HTTP headers, FS cache, and query string parsing that don't have a fixed pattern of object properties.

On the other hand, an instance of EventEmitter often only has a few event names, and in case of the benchmarks they all only have one event 'dummy', so slow properties get (unfairly, arguably) penalized.

Take the benchmark with the worst regression, ee-listener-count-on-prototype.js. If we make the emitter have listeners for two events instead of just one, this PR actually drastically improves its performance:

                                                     improvement confidence      p.value
 events/ee-listener-count-on-prototype.js n=50000000     61.35 %        *** 2.676747e-11

I believe having two or more events is a considerably more common case than just having one event, and therefore I think this change is still justified.

benchmark diff
diff --git a/benchmark/events/ee-listener-count-on-prototype.js b/benchmark/events/ee-listener-count-on-prototype.js
index dfe7e27..1ef0f09 100644
--- a/benchmark/events/ee-listener-count-on-prototype.js
+++ b/benchmark/events/ee-listener-count-on-prototype.js
@@ -9,12 +9,12 @@ function main(conf) {
 
   var ee = new EventEmitter();
 
-  for (var k = 0; k < 10; k += 1)
-    ee.on('dummy', function() {});
+  for (var k = 0; k < 2 * 10; k += 1)
+    ee.on(`dummy${k % 2}`, function() {});
 
   bench.start();
   for (var i = 0; i < n; i += 1) {
-    ee.listenerCount('dummy');
+    ee.listenerCount(`dummy${i % 2}`);
   }
   bench.end(n);
 }
Member

TimothyGu commented Mar 20, 2017

Interesting.

The V8 optimization is mainly designed to accommodate map-like usage of objects, by automatically using so-called "slow" properties based on a hashmap instead of trying to find hidden classes. This makes it work especially well with cases like HTTP headers, FS cache, and query string parsing that don't have a fixed pattern of object properties.

On the other hand, an instance of EventEmitter often only has a few event names, and in case of the benchmarks they all only have one event 'dummy', so slow properties get (unfairly, arguably) penalized.

Take the benchmark with the worst regression, ee-listener-count-on-prototype.js. If we make the emitter have listeners for two events instead of just one, this PR actually drastically improves its performance:

                                                     improvement confidence      p.value
 events/ee-listener-count-on-prototype.js n=50000000     61.35 %        *** 2.676747e-11

I believe having two or more events is a considerably more common case than just having one event, and therefore I think this change is still justified.

benchmark diff
diff --git a/benchmark/events/ee-listener-count-on-prototype.js b/benchmark/events/ee-listener-count-on-prototype.js
index dfe7e27..1ef0f09 100644
--- a/benchmark/events/ee-listener-count-on-prototype.js
+++ b/benchmark/events/ee-listener-count-on-prototype.js
@@ -9,12 +9,12 @@ function main(conf) {
 
   var ee = new EventEmitter();
 
-  for (var k = 0; k < 10; k += 1)
-    ee.on('dummy', function() {});
+  for (var k = 0; k < 2 * 10; k += 1)
+    ee.on(`dummy${k % 2}`, function() {});
 
   bench.start();
   for (var i = 0; i < n; i += 1) {
-    ee.listenerCount('dummy');
+    ee.listenerCount(`dummy${i % 2}`);
   }
   bench.end(n);
 }
@TimothyGu

This comment has been minimized.

Show comment
Hide comment
@TimothyGu

TimothyGu Mar 20, 2017

Member

@cjihrig

I worry that this will make debugging more difficult though.

Can you elaborate? I doubt having a generic name like StorageObject instead of nothing would improve debugging experience, and EventHandlers though descriptive is only used for a property named _events, which announces the intention of the object well enough.

Member

TimothyGu commented Mar 20, 2017

@cjihrig

I worry that this will make debugging more difficult though.

Can you elaborate? I doubt having a generic name like StorageObject instead of nothing would improve debugging experience, and EventHandlers though descriptive is only used for a property named _events, which announces the intention of the object well enough.

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Mar 20, 2017

Contributor

I meant specifically in heap snapshots.

Contributor

cjihrig commented Mar 20, 2017

I meant specifically in heap snapshots.

@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Mar 20, 2017

Member

I think the benchmarks can add a few more configurations for different use cases.

Note that the diff in #11930 (comment) also removes the loop invariant(not sure if V8 could perform LICM on that previously).

Also +1 with @cjihrig, with Object.create(null) those objects probably won't be grouped together in heap snapshots and only have an id in their labels.

Member

joyeecheung commented Mar 20, 2017

I think the benchmarks can add a few more configurations for different use cases.

Note that the diff in #11930 (comment) also removes the loop invariant(not sure if V8 could perform LICM on that previously).

Also +1 with @cjihrig, with Object.create(null) those objects probably won't be grouped together in heap snapshots and only have an id in their labels.

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Mar 20, 2017

Contributor

What's up with the 'manyblankpairs' regression?

Contributor

mscdex commented Mar 20, 2017

What's up with the 'manyblankpairs' regression?

@TimothyGu

This comment has been minimized.

Show comment
Hide comment
@TimothyGu

TimothyGu Mar 20, 2017

Member

@mscdex, it can be attributed to the initial cost of object creation:

$ ./node benchmark/es/map-bench.js method=storageObjectCreation millions=4
es/map-bench.js millions=4 method="storageObjectCreation": 216.31376787921167
$ ./node benchmark/es/map-bench.js method=objectCreation millions=4
es/map-bench.js millions=4 method="objectCreation": 99.61981839655775
$ ./node benchmark/es/map-bench.js method=nullProtoObjectCreation millions=4
es/map-bench.js millions=4 method="nullProtoObjectCreation": 33.876842956365344

$ ./node --turbo benchmark/es/map-bench.js method=storageObjectCreation millions=4
es/map-bench.js millions=4 method="storageObjectCreation": 934.5947246800764
$ ./node --turbo benchmark/es/map-bench.js method=objectCreation millions=4
es/map-bench.js millions=4 method="objectCreation": 892.728215367111
$ ./node --turbo benchmark/es/map-bench.js method=nullProtoObjectCreation millions=4
es/map-bench.js millions=4 method="nullProtoObjectCreation": 28.154238858915456
Member

TimothyGu commented Mar 20, 2017

@mscdex, it can be attributed to the initial cost of object creation:

$ ./node benchmark/es/map-bench.js method=storageObjectCreation millions=4
es/map-bench.js millions=4 method="storageObjectCreation": 216.31376787921167
$ ./node benchmark/es/map-bench.js method=objectCreation millions=4
es/map-bench.js millions=4 method="objectCreation": 99.61981839655775
$ ./node benchmark/es/map-bench.js method=nullProtoObjectCreation millions=4
es/map-bench.js millions=4 method="nullProtoObjectCreation": 33.876842956365344

$ ./node --turbo benchmark/es/map-bench.js method=storageObjectCreation millions=4
es/map-bench.js millions=4 method="storageObjectCreation": 934.5947246800764
$ ./node --turbo benchmark/es/map-bench.js method=objectCreation millions=4
es/map-bench.js millions=4 method="objectCreation": 892.728215367111
$ ./node --turbo benchmark/es/map-bench.js method=nullProtoObjectCreation millions=4
es/map-bench.js millions=4 method="nullProtoObjectCreation": 28.154238858915456
@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Mar 20, 2017

Member

Before this PR (StorageObject have names and can be searched):

screen shot 2017-03-20 at 12 22 56 pm

After:

screen shot 2017-03-20 at 12 22 25 pm

I think the performance gain is probably worth it but wonder if @nodejs/v8 have any idea to work around this sacrafice

Member

joyeecheung commented Mar 20, 2017

Before this PR (StorageObject have names and can be searched):

screen shot 2017-03-20 at 12 22 56 pm

After:

screen shot 2017-03-20 at 12 22 25 pm

I think the performance gain is probably worth it but wonder if @nodejs/v8 have any idea to work around this sacrafice

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Mar 20, 2017

Contributor

Is there any perf difference with StorageObject.prototype = null; ? I'm not sure why I didn't originally use that, but it seems to work similarly?

Contributor

mscdex commented Mar 20, 2017

Is there any perf difference with StorageObject.prototype = null; ? I'm not sure why I didn't originally use that, but it seems to work similarly?

@lpinca

lpinca approved these changes Mar 20, 2017

@lpinca

This comment has been minimized.

Show comment
Hide comment
@lpinca

lpinca Mar 20, 2017

Member

@mscdex

$ node
> function StorageObject() {}
undefined
> StorageObject.prototype = null
null
> var o = new StorageObject()
undefined
> o.
o.__defineGetter__      o.__defineSetter__      o.__lookupGetter__      o.__lookupSetter__      o.__proto__
o.constructor           o.hasOwnProperty        o.isPrototypeOf         o.propertyIsEnumerable  o.toLocaleString
o.toString              o.valueOf
Member

lpinca commented Mar 20, 2017

@mscdex

$ node
> function StorageObject() {}
undefined
> StorageObject.prototype = null
null
> var o = new StorageObject()
undefined
> o.
o.__defineGetter__      o.__defineSetter__      o.__lookupGetter__      o.__lookupSetter__      o.__proto__
o.constructor           o.hasOwnProperty        o.isPrototypeOf         o.propertyIsEnumerable  o.toLocaleString
o.toString              o.valueOf
@jasnell

changes LGTM but needs a rebase

TimothyGu added some commits Mar 19, 2017

url: use a class for url[context]
The object is used as a structure, not as a map, which `StorageObject`
was designed for.
lib: use Object.create(null) directly
After V8 5.6, using Object.create(null) directly is now faster than
using a constructor.

Refs: emberjs/ember.js#15001
Refs: https://crrev.com/532c16eca071df3ec8eed394dcebb932ef584ee6
@TimothyGu

This comment has been minimized.

Show comment
Hide comment
@TimothyGu

TimothyGu Mar 23, 2017

Member

@joyeecheung I added a commit that uses a dedicated class for url[context], which should make heap debugging for this case as easy as before. It also restores the performance loss caused by a switch to Object.create(null), since the context always has a fixed set of properties.

Member

TimothyGu commented Mar 23, 2017

@joyeecheung I added a commit that uses a dedicated class for url[context], which should make heap debugging for this case as easy as before. It also restores the performance loss caused by a switch to Object.create(null), since the context always has a fixed set of properties.

@TimothyGu

This comment has been minimized.

Show comment
Hide comment
@TimothyGu
Member

TimothyGu commented Mar 24, 2017

Landed in d9b0e4c...cfc8422.

@TimothyGu TimothyGu closed this Mar 24, 2017

@TimothyGu TimothyGu deleted the TimothyGu:storage-object branch Mar 24, 2017

TimothyGu added a commit to TimothyGu/node that referenced this pull request Mar 24, 2017

benchmark: add more options to map-bench
PR-URL: nodejs#11930
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

TimothyGu added a commit to TimothyGu/node that referenced this pull request Mar 24, 2017

url: use a class for WHATWG url[context]
The object is used as a structure, not as a map, which `StorageObject`
was designed for.

PR-URL: nodejs#11930
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

TimothyGu added a commit to TimothyGu/node that referenced this pull request Mar 24, 2017

lib: use Object.create(null) directly
After V8 5.6, using Object.create(null) directly is now faster than
using a constructor for map-like objects.

PR-URL: nodejs#11930
Refs: emberjs/ember.js#15001
Refs: https://crrev.com/532c16eca071df3ec8eed394dcebb932ef584ee6
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Mar 25, 2017

Contributor

@TimothyGu I'm seeing contradicting results on master with some more simple, direct benchmarks:

'use strict';

var n = 1e7;
var input = { mode: 0o666, flags: 'r', start: 5, end: 100 };

function StorageObject() {}
StorageObject.prototype = Object.create(null);
function copyObject(source) {
  const target = new StorageObject();
  for (var key in source)
    target[key] = source[key];
  return target;
}

function copyObject2(source) {
  const target = Object.create(null);
  for (var key in source)
    target[key] = source[key];
  return target;
}

var r;
console.time('copyObject');
for (var i = 0; i < n; ++i) {
  r = copyObject(input);
  r.mode >>>= 0;
}
console.timeEnd('copyObject');

console.time('copyObject2');
for (var i = 0; i < n; ++i) {
  r = copyObject2(input);
  r.mode >>>= 0;
}
console.timeEnd('copyObject2');

Results:

copyObject: 2487.810ms
copyObject2: 8500.589ms

With just the first two properties in input:

copyObject: 1600.427ms
copyObject2: 3985.290ms

With just the first property in input:

copyObject: 838.353ms
copyObject2: 2215.232ms
Contributor

mscdex commented Mar 25, 2017

@TimothyGu I'm seeing contradicting results on master with some more simple, direct benchmarks:

'use strict';

var n = 1e7;
var input = { mode: 0o666, flags: 'r', start: 5, end: 100 };

function StorageObject() {}
StorageObject.prototype = Object.create(null);
function copyObject(source) {
  const target = new StorageObject();
  for (var key in source)
    target[key] = source[key];
  return target;
}

function copyObject2(source) {
  const target = Object.create(null);
  for (var key in source)
    target[key] = source[key];
  return target;
}

var r;
console.time('copyObject');
for (var i = 0; i < n; ++i) {
  r = copyObject(input);
  r.mode >>>= 0;
}
console.timeEnd('copyObject');

console.time('copyObject2');
for (var i = 0; i < n; ++i) {
  r = copyObject2(input);
  r.mode >>>= 0;
}
console.timeEnd('copyObject2');

Results:

copyObject: 2487.810ms
copyObject2: 8500.589ms

With just the first two properties in input:

copyObject: 1600.427ms
copyObject2: 3985.290ms

With just the first property in input:

copyObject: 838.353ms
copyObject2: 2215.232ms
@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Mar 25, 2017

Contributor

I should add that I just tried with V8 5.7 which was just merged into master and the results are similar.

Contributor

mscdex commented Mar 25, 2017

I should add that I just tried with V8 5.7 which was just merged into master and the results are similar.

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Mar 26, 2017

Contributor

Ah, I just noticed that es/map-bench.js isn't actually benchmarking object creation, but instead property access. There is a much larger difference with object creation in StorageObject's favor compared to property access where StorageObject is still pretty close to the other top performing alternatives.

I think we should revert the changes in this PR (except maybe the URL changes where the properties are known beforehand?). /cc @nodejs/collaborators

Contributor

mscdex commented Mar 26, 2017

Ah, I just noticed that es/map-bench.js isn't actually benchmarking object creation, but instead property access. There is a much larger difference with object creation in StorageObject's favor compared to property access where StorageObject is still pretty close to the other top performing alternatives.

I think we should revert the changes in this PR (except maybe the URL changes where the properties are known beforehand?). /cc @nodejs/collaborators

@lpinca

This comment has been minimized.

Show comment
Hide comment
@lpinca

lpinca Mar 26, 2017

Member

I also noticed the huge difference with object creation time but this also "fixes" something weird which happens when using instances of

function StorageObject() {}
StorageObject.prototype = Object.create(null);

I've described the behavior in #11868 (comment). @mscdex can you take a look?

Member

lpinca commented Mar 26, 2017

I also noticed the huge difference with object creation time but this also "fixes" something weird which happens when using instances of

function StorageObject() {}
StorageObject.prototype = Object.create(null);

I've described the behavior in #11868 (comment). @mscdex can you take a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment