events: speed up emit by create faster null prototype object #39939
Conversation
nodejs-github-bot
added
events
|
@nodejs/v8 is there an explanation to this large difference in speed? |
wwwzbwcom
force-pushed
the
speed-up-emit
branch
from
0884c86 to
66fcbef
Compare
|
I think |
|
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 |
|
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1047/ Results |
|
@targos here is the full benchmark result, |
|
Looking at the code from what I understand - I'm more confused than when I started :) |
@benjamingr Yes it's strange, here is a simple test: |
@benjamingr I check the source code, looks like |
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 |
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 (Or run with |
|
Oh and to illustrate the point take the code here:
And see what happens if you invert the order of the test :) (check .create first) |
jasnell
added
the
semver-major
|
This is definitely a breaking change as there is code in the ecosystem that (rightly or wrongly) accesses |
@jasnell Yes, the prototype is alwasy Map is faster than |
@benjamingr 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 |
Yes, when you switch the order, the result is different: 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 :)) |
Agree that, some realworld usage benchmark is need. |
|
@devsnek slightly improvement in readable-from, more realworld benchmark later |
|
I'll explain why I'm suspicious. 17.8% is a big improvement in |
|
Late to the party. You're observation is correct, both 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 |
|
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 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 |
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% |
|
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) |
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:
Also ping @mscdex |
We need to create null prototype to prevent internal method like
toStringmess with event handling.But in current version v8, Object create by
ObjectSetPrototypeOfis much more efficient thanObjectCreate(null)Here is benchmark result: