GeezerVison

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

                                                     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

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.

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);
 }

@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.

I meant specifically in heap snapshots.

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.

What's up with the 'manyblankpairs' regression?

@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

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

After:

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

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?

@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

@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.

Landed in d9b0e4c...cfc8422.

@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

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

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

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?

This is a breaking change. Maybe it's faster now, but it is not backwards compatible, e.g. it broke the following code:

var reqArgs = url.parse(req.url, true).query;
if (reqArgs.hasOwnProperty("id"))
  ...

now throws TypeError: reqArgs.hasOwnProperty is not a function

@twoi I imagine you upgraded from a release to another, can you give us the version numbers? Was this a patch/minor release or a major release? Thanks.

We upgraded from 0.8.15 to 11.9 - finally, because we are using a native module that (pre-napi) needed to ported / rebuilt on every Node.js version upgrade, which include all the vast v8 refactorings.

But this change apparently made it into Node.js version 8.0.0. It's filed under Semver-patch commits, even though I'd argue it's a breaking change.

@twoi this PR is semver-patch. The removal of the prototype was done in #6092 which is marked as semver-major (v6). Is there anything actionable you ask for?

I highly recommend to never use object.hasOwnProperty('foobar'). Instead just use:
Object.prototoype.hasOwnProperty.call(object, 'foobar'). That is a significantly safer way to do the same.

@BridgeAR Thanks for the advice. Agree.

I don't have anything actionable, just wanted to make aware of the fact that this innocent looking change can (and in fact did) break someone's code.

Which, it seems, people noticed during testing: https://github.com/nodejs/node/pull/6092/files#r58880619
And then turned it semver-major.

I am adding more testing myself, so this won't catch me unawares anymore... Case closed.

@twoi

this innocent looking change can (and in fact did) break someone's code

The breaking change was done in #6092, not here. This PR is just some code refactoring.

@BridgeAR I see. Makes sense. Thanks