GeezerVison

@nodejs/v8 is there an explanation to this large difference in speed?

I think Object.create(null) creates the object in "dictionary mode". With this change dictionary mode is not used until some operation determines the transition to it.

See also #11930.

The problem with this micro-benchmark is that it tests with only one event name. This is not representative of real use of event emitters.

We need at least to compare the other events benchmarks too.

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1047/

                                                                 confidence improvement accuracy (*)    (**)   (***)
events/ee-add-remove.js n=1000000 removeListener=0 newListener=0                -3.41 %       ±4.05%  ±5.39%  ±7.02%
events/ee-add-remove.js n=1000000 removeListener=0 newListener=1        ***     18.79 %       ±5.50%  ±7.40%  ±9.79%
events/ee-add-remove.js n=1000000 removeListener=1 newListener=0         **      5.02 %       ±2.99%  ±3.99%  ±5.23%
events/ee-add-remove.js n=1000000 removeListener=1 newListener=1        ***     11.48 %       ±2.75%  ±3.66%  ±4.78%
events/ee-emit.js listeners=0 argc=0 n=2000000                          ***    316.62 %      ±20.23% ±27.20% ±36.00%
events/ee-emit.js listeners=0 argc=10 n=2000000                         ***    289.75 %      ±18.03% ±24.24% ±32.03%
events/ee-emit.js listeners=0 argc=2 n=2000000                          ***    301.79 %      ±11.32% ±15.16% ±19.92%
events/ee-emit.js listeners=0 argc=4 n=2000000                          ***    290.69 %      ±10.78% ±14.46% ±19.06%
events/ee-emit.js listeners=10 argc=0 n=2000000                         ***      8.23 %       ±1.28%  ±1.70%  ±2.22%
events/ee-emit.js listeners=10 argc=10 n=2000000                        ***      6.52 %       ±1.30%  ±1.74%  ±2.27%
events/ee-emit.js listeners=10 argc=2 n=2000000                         ***      8.41 %       ±1.32%  ±1.76%  ±2.29%
events/ee-emit.js listeners=10 argc=4 n=2000000                         ***      9.18 %       ±1.30%  ±1.73%  ±2.26%
events/ee-emit.js listeners=1 argc=0 n=2000000                          ***    163.36 %      ±11.25% ±15.02% ±19.64%
events/ee-emit.js listeners=1 argc=10 n=2000000                         ***    163.33 %      ±13.79% ±18.51% ±24.42%
events/ee-emit.js listeners=1 argc=2 n=2000000                          ***    155.27 %      ±12.03% ±16.17% ±21.37%
events/ee-emit.js listeners=1 argc=4 n=2000000                          ***    164.21 %       ±9.38% ±12.60% ±16.63%
events/ee-emit.js listeners=5 argc=0 n=2000000                          ***     18.82 %       ±2.40%  ±3.20%  ±4.17%
events/ee-emit.js listeners=5 argc=10 n=2000000                         ***     11.39 %       ±2.36%  ±3.14%  ±4.10%
events/ee-emit.js listeners=5 argc=2 n=2000000                          ***     17.06 %       ±2.27%  ±3.03%  ±3.95%
events/ee-emit.js listeners=5 argc=4 n=2000000                          ***     18.17 %       ±2.53%  ±3.38%  ±4.43%
events/ee-listener-count-on-prototype.js n=50000000                      **     -7.75 %       ±4.80%  ±6.39%  ±8.33%
events/ee-listeners.js raw='false' listeners=50 n=5000000                       -1.54 %       ±1.68%  ±2.24%  ±2.92%
events/ee-listeners.js raw='false' listeners=5 n=5000000                ***     -3.72 %       ±1.69%  ±2.25%  ±2.92%
events/ee-listeners.js raw='true' listeners=50 n=5000000                        -1.24 %       ±3.07%  ±4.12%  ±5.44%
events/ee-listeners.js raw='true' listeners=5 n=5000000                  **     -5.18 %       ±3.07%  ±4.09%  ±5.33%
events/ee-once.js argc=0 n=20000000                                     ***    -29.54 %       ±0.77%  ±1.02%  ±1.33%
events/ee-once.js argc=1 n=20000000                                     ***    -28.67 %       ±1.44%  ±1.93%  ±2.54%
events/ee-once.js argc=4 n=20000000                                     ***    -25.95 %       ±1.62%  ±2.17%  ±2.84%
events/ee-once.js argc=5 n=20000000                                     ***    -27.45 %       ±0.75%  ±1.00%  ±1.30%
events/eventtarget.js listeners=10 n=1000000                                    -4.22 %       ±5.43%  ±7.29%  ±9.63%
events/eventtarget.js listeners=1 n=1000000                                      0.13 %       ±1.25%  ±1.67%  ±2.17%
events/eventtarget.js listeners=5 n=1000000                                     -1.53 %       ±4.50%  ±6.05%  ±8.01%

@targos here is the full benchmark result, once event is slower than before.
But maybe it's acceptable for there is a huge improvement in usual on event handle, and we won't emit an once event many times in usual case

                                                                confidence improvement accuracy (*)   (**)   (***)
events/ee-add-remove.js n=1000000 removeListener=0 newListener=0        ***     -6.31 %       ±1.00% ±1.34%  ±1.75%
events/ee-add-remove.js n=1000000 removeListener=0 newListener=1        ***     35.24 %       ±2.08% ±2.77%  ±3.61%
events/ee-add-remove.js n=1000000 removeListener=1 newListener=0        ***     19.70 %       ±2.03% ±2.71%  ±3.53%
events/ee-add-remove.js n=1000000 removeListener=1 newListener=1        ***     27.55 %       ±1.14% ±1.51%  ±1.97%
events/ee-emit.js listeners=0 argc=0 n=2000000                          ***    273.86 %       ±5.62% ±7.57% ±10.05%
events/ee-emit.js listeners=0 argc=10 n=2000000                         ***    261.12 %       ±4.78% ±6.43%  ±8.53%
events/ee-emit.js listeners=0 argc=2 n=2000000                          ***    271.77 %       ±4.57% ±6.14%  ±8.11%
events/ee-emit.js listeners=0 argc=4 n=2000000                          ***    272.22 %       ±6.62% ±8.91% ±11.80%
events/ee-emit.js listeners=1 argc=0 n=2000000                          ***    137.57 %       ±1.50% ±2.00%  ±2.61%
events/ee-emit.js listeners=1 argc=10 n=2000000                         ***    134.44 %       ±4.69% ±6.29%  ±8.31%
events/ee-emit.js listeners=1 argc=2 n=2000000                          ***    130.85 %       ±1.92% ±2.58%  ±3.41%
events/ee-emit.js listeners=1 argc=4 n=2000000                          ***    135.05 %       ±1.69% ±2.26%  ±2.97%
events/ee-emit.js listeners=10 argc=0 n=2000000                         ***     17.99 %       ±1.53% ±2.04%  ±2.67%
events/ee-emit.js listeners=10 argc=10 n=2000000                        ***     14.87 %       ±1.12% ±1.50%  ±1.96%
events/ee-emit.js listeners=10 argc=2 n=2000000                         ***     16.37 %       ±1.32% ±1.76%  ±2.31%
events/ee-emit.js listeners=10 argc=4 n=2000000                         ***     15.44 %       ±1.21% ±1.61%  ±2.11%
events/ee-emit.js listeners=5 argc=0 n=2000000                          ***     19.33 %       ±1.06% ±1.41%  ±1.84%
events/ee-emit.js listeners=5 argc=10 n=2000000                         ***     13.76 %       ±0.90% ±1.20%  ±1.57%
events/ee-emit.js listeners=5 argc=2 n=2000000                          ***     21.39 %       ±2.25% ±3.02%  ±4.00%
events/ee-emit.js listeners=5 argc=4 n=2000000                          ***     14.76 %       ±1.60% ±2.14%  ±2.82%
events/ee-error.js n=100000                                                     -0.06 %       ±0.36% ±0.48%  ±0.63%
events/ee-listener-count-on-prototype.js n=50000000                     ***    -11.98 %       ±1.58% ±2.10%  ±2.75%
events/ee-listeners.js raw='false' listeners=5 n=5000000                ***     -5.10 %       ±0.78% ±1.04%  ±1.36%
events/ee-listeners.js raw='false' listeners=50 n=5000000                       -0.87 %       ±1.18% ±1.57%  ±2.04%
events/ee-listeners.js raw='true' listeners=5 n=5000000                 ***     -6.92 %       ±1.34% ±1.78%  ±2.32%
events/ee-listeners.js raw='true' listeners=50 n=5000000                ***     -4.20 %       ±1.28% ±1.71%  ±2.23%
events/ee-once.js argc=0 n=20000000                                     ***    -29.26 %       ±0.52% ±0.70%  ±0.91%
events/ee-once.js argc=1 n=20000000                                     ***    -29.85 %       ±1.76% ±2.36%  ±3.09%
events/ee-once.js argc=4 n=20000000                                     ***    -28.23 %       ±0.57% ±0.76%  ±0.99%
events/ee-once.js argc=5 n=20000000                                     ***    -28.06 %       ±0.49% ±0.65%  ±0.86%

Looking at the code from what I understand - I'm more confused than when I started :)

https://source.chromium.org/chromium/chromium/src/+/main:v8/src/builtins/builtins-object-gen.cc;l=1104-1111?q=ObjectHasOwn&ss=chromium%2Fchromium%2Fsrc

This doesn't seem right, I'd assume that Object.create(null) literally calls NewJSObjectWithNullProto which in turn just does NewJSObject (similar to your = {} here) and then SetPrototype.

I'd like to understand this change better first.

@benjamingr Yes it's strange, here is a simple test:

const now = () => {
    const hrTime = process.hrtime();
    return hrTime[0] * 1000 + hrTime[1] / 1000000;
};
let a, start, end;

a = {};
Object.setPrototypeOf(a, null);
start = now();
for (let i = 0; i < 1e8; i++) {
    a['data'];
}
end = now();
console.log(end - start);

a = Object.create(null);
start = now();
for (let i = 0; i < 1e8; i++) {
    a['data'];
}
end = now();
console.log(end - start);

/*
64.12246084213257
837.3292689323425
*/

Looking at the code from what I understand - I'm more confused than when I started :)

https://source.chromium.org/chromium/chromium/src/+/main:v8/src/builtins/builtins-object-gen.cc;l=1104-1111?q=ObjectHasOwn&ss=chromium%2Fchromium%2Fsrc

@benjamingr I check the source code, looks like Object.create(null) method cant take benefit from fast properties?

https://v8.dev/blog/fast-properties

Looking at the code from what I understand - I'm more confused than when I started :)
https://source.chromium.org/chromium/chromium/src/+/main:v8/src/builtins/builtins-object-gen.cc;l=1104-1111?q=ObjectHasOwn&ss=chromium%2Fchromium%2Fsrc

@benjamingr I check the source code, looks like Object.create(null) method cant take benefit from fast properties?

https://v8.dev/blog/fast-properties

Yeah, but that's the problem of this benchmark. Fast properties work because the object always has the same shape. In the real world, many event emitters are created, with different events, and this is no longer true.

Looking at the code from what I understand - I'm more confused than when I started :)
https://source.chromium.org/chromium/chromium/src/+/main:v8/src/builtins/builtins-object-gen.cc;l=1104-1111?q=ObjectHasOwn&ss=chromium%2Fchromium%2Fsrc

@benjamingr I check the source code, looks like Object.create(null) method cant take benefit from fast properties?
https://v8.dev/blog/fast-properties

Yeah, but that's the problem of this benchmark. Fast properties work because the object always has the same shape. In the real world, many event emitters are created, with different events, and this is no longer true.

@targos I dont think so, different ee can have different event, but as some event such store as fast property, we can take benefit when getting its handler

Looking at the code from what I understand - I'm more confused than when I started :)
https://source.chromium.org/chromium/chromium/src/+/main:v8/src/builtins/builtins-object-gen.cc;l=1104-1111?q=ObjectHasOwn&ss=chromium%2Fchromium%2Fsrc

@benjamingr I check the source code, looks like Object.create(null) method cant take benefit from fast properties?
https://v8.dev/blog/fast-properties

Yeah, but that's the problem of this benchmark. Fast properties work because the object always has the same shape. In the real world, many event emitters are created, with different events, and this is no longer true.

Also most ee has similar shape such as stream all have data and close events

I don't understand why this would get fast properties in one way or not another - there are several heuristics for fast properties - being a prototype is one but I don't understand why it'd happen here in one case and not the other.

(And if it does, I doubt it's a good optimization).

To be clear: I am thankful for your contribution and am just confused about the behaviour we're seeing.

Can you run the code with v8 native syntax turned on require('v8').setFlagsFromString('--allow-natives-syntax') and then check %HasFastElements?

(Or run with --trace-opt --trace-deopt)

Oh and to illustrate the point take the code here:

@benjamingr Yes it's strange, here is a simple test:

And see what happens if you invert the order of the test :) (check .create first)

This is definitely a breaking change as there is code in the ecosystem that (rightly or wrongly) accesses _events directly, and switching to a null prototype could definitely break that code update: I forgot we already made the change to a null prototype before... still, out of an abundance of caution here, let's treat this as semver-major until we're definitely sure it's not. If we do decide to make changes here, we should absolutely also test the performance difference of using a Map here also. A long time ago when Map was tested it was definitely slower but I'm not sure if that would still be the case.

This is definitely a breaking change as there is code in the ecosystem that (rightly or wrongly) accesses _events directly, and switching to a null prototype could definitely break that code update: I forgot we already made the change to a null prototype before... still, out of an abundance of caution here, let's treat this as semver-major until we're definitely sure it's not. If we do decide to make changes here, we should absolutely also test the performance difference of using a Map here also. A long time ago when Map was tested it was definitely slower but I'm not sure if that would still be the case.

@jasnell Yes, the prototype is alwasy null before and after this change.

Map is faster than ObjectCreate(), but Map also has method like toString

I don't understand why this would get fast properties in one way or not another - there are several heuristics for fast properties - being a prototype is one but I don't understand why it'd happen here in one case and not the other.

(And if it does, I doubt it's a good optimization).

To be clear: I am thankful for your contribution and am just confused about the behaviour we're seeing.

Can you run the code with v8 native syntax turned on require('v8').setFlagsFromString('--allow-natives-syntax') and then check %HasFastElements?

(Or run with --trace-opt --trace-deopt)

I don't understand why this would get fast properties in one way or not another - there are several heuristics for fast properties - being a prototype is one but I don't understand why it'd happen here in one case and not the other.

(And if it does, I doubt it's a good optimization).

To be clear: I am thankful for your contribution and am just confused about the behaviour we're seeing.

Can you run the code with v8 native syntax turned on require('v8').setFlagsFromString('--allow-natives-syntax') and then check %HasFastElements?

(Or run with --trace-opt --trace-deopt)

@benjamingr
Thank for your advice, seems like my suppose is correct: Object.create wont have FastProperties:

Welcome to Node.js v14.17.3.
Type ".help" for more information.
> require('v8').setFlagsFromString('--allow-natives-syntax')
undefined
> %HasFastProperties(Object.create(null))
false
> %HasFastProperties({})
true
> let a = {};
undefined
> Object.setPrototypeOf(a, null);
[Object: null prototype] {}
> %HasFastProperties(a)
true

Oh and to illustrate the point take the code here:

@benjamingr Yes it's strange, here is a simple test:

And see what happens if you invert the order of the test :) (check .create first)

@benjamingr

Yes, when you switch the order, the result is different:
no longer 10x, but also has a 2x improvement (This time I run it 10 times for accuracy)

const now = () => {
    const hrTime = process.hrtime();
    return hrTime[0] * 1000 + hrTime[1] / 1000000;
};
let a, start, end;

for (let i = 0; i < 10; i++) {
    a = Object.create(null);
    start = now();
    for (let i = 0; i < 1e8; i++) {
        a['data'];
    }
    end = now();
    console.log(`Object.create`, end - start);

    a = {};
    Object.setPrototypeOf(a, null);
    start = now();
    for (let i = 0; i < 1e8; i++) {
        a['data'];
    }
    end = now();
    console.log(`Object.setPrototypeOf`, end - start);
}

/*
Object.create 775.9817728996277
Object.setPrototypeOf 488.72393321990967
Object.create 909.9279327392578
Object.setPrototypeOf 481.3809299468994
Object.create 895.326687335968
Object.setPrototypeOf 473.78826904296875
Object.create 899.6548957824707
Object.setPrototypeOf 474.5387659072876
Object.create 898.921669960022
Object.setPrototypeOf 477.34186267852783
Object.create 895.6246657371521
Object.setPrototypeOf 473.7106132507324
Object.create 894.2418832778931
Object.setPrototypeOf 473.29633617401123
Object.create 908.3455362319946
Object.setPrototypeOf 472.2995858192444
Object.create 895.0272059440613
Object.setPrototypeOf 474.7768030166626
Object.create 894.8622059822083
Object.setPrototypeOf 474.1221857070923
*/

I would be concerned that in real-world usage the backing object here will deoptimize to dictionary mode anyway. I'd appreciate seeing some more representative benchmarks.

@devsnek I think an argument @wwwzbwcom is making here is that in real-world usage many event emitters only emit 1-2 event types.

I see different results for %HasFastProperties after emitting 100 events on each (both are fast properties :))

I would be concerned that in real-world usage the backing object here will deoptimize to dictionary mode anyway. I'd appreciate seeing some more representative benchmarks.

Agree that, some realworld usage benchmark is need.

@devsnek slightly improvement in readable-from, more realworld benchmark later

node benchmark/compare.js --old ./node-master --new ./node-dev --filter "readable-from" streams > compare.csv
cat compare.csv | Rscript benchmark/compare.R                                    
confidence improvement accuracy (*)    (**)   (***)
streams/readable-from.js n=10000000         **     17.47 %      ±11.10% ±14.78% ±19.26%

I'll explain why I'm suspicious. 17.8% is a big improvement in streams/readable-from.js and I don't understand why/how this change explains it.

Late to the party. You're observation is correct, both {__proto__:null} and Object.create(null) will create dictionary/slow-mode objects in V8 (and if you're really performance sensitive, use the literal syntax as it avoids additional property loads and a method call).

This is based on the assumptions that folks use null-proto objects as dictionaries, in which case the fast-mode structure-tracking in V8 causes more harm than good. Especially all secondary effects with ICs are not represented in your micro benchmark, which is also one of the main motivations for "exotic" behavior in V8.

The reasoning behind this, is that if you have known property-names you're likely fine with non-null prototype, since you don't accidentally read those. However, with unknown property names (and that's the dictionary/Map use-case), you have to use a null-proto in order to get correct behavior. Thus the null-proto is quite a good indication for this behavior.

Let me know if you I can help with additional information.

@camillobruni thanks that's educational! and setting const o = {}; Object.setPrototypeOf(o, null); does not have the same optimisation of assuming it's used as a dictionary?

I think that's quite an uncommon case that's not widely adopted for this pattern.

Would a custom non-null prototype als be a solution for you?
I think that's going to be faster still than calling setPrototypeOf.

   const EventProto = {__proto__:null};
   ...
   this._events =  {__proto__: EventProto};
   ...

I think it makes sense to default to shapes/hidden-classes here since it'll fall back to "slow mode" anyway if you add many handlers.

Note that event lists aren't really dictionaries since often you sign up to multiple events once and then you dispatch events many times.

That said, I have no idea why the Readable.from benchmark changed 17%, Readable.from doesn't even subscribe to any events it just reads from an async iterator.

I think it makes sense to default to shapes/hidden-classes here since it'll fall back to "slow mode" anyway if you add many handlers.

Note that event lists aren't really dictionaries since often you sign up to multiple events once and then you dispatch events many times.

That said, I have no idea why the Readable.from benchmark changed 17%, Readable.from doesn't even subscribe to any events it just reads from an async iterator.

@benjamingr

in this benchmark, it does not just read from the stream, but create a handler on the stream "data" event, so as the ee is faster, this benchmark change 17%

@ronag

@ronag

I agree with this: #39939 (comment)

@ronag I meant - what do you think of this change (optimizing for subscribing to fewer static events treating event lists in emitters as more static than not - from the streams PoV)

@ronag I meant - what do you think of this change (optimizing for subscribing to fewer static events treating event lists in emitters as more static than not - from the streams PoV)

From stream point of view I think it's a good idea. Streams usually don't add/remove events and there is a limited number of events (data/drain/readable, end/finish, error, close). I think we've tried avoid dynamic patterns. So usually there is 4 static events + handlers per stream.

Ok, I think I'm fine with this :)

It:

• Would be cool if for streams (and other common emitters) the object shape would be pre-determined (that is, fast elements with the known event types though set to undefined) though that might be breaking if people check .hasOwn (well, since that literally made stage 4 yesterday - Object.prototype.hasOwnProperty.call).
• I'd prefer the syntax suggested by @camillobruni

Also ping @mscdex

I am going to close this due to the issues discussed and since we didn't get a conclusion until now. Please open a new PR, if you believe we should still do this.