Repository navigation
Performance regression in v12 caused by primordials #29766
Description
I have heard about performance regressions caused by transition to primordials in v12, but couldn't find a tracking issue for the regression, so opening this to make sure we are tracking it and can work with the upstream to get this handled, on way or another (feel free to close this if there is already an issue opened here).
The regressions seem to come from two types of code patterns:
- Calling functions through
Function.prototype.{call, apply}instead of just calling them directly. There is an issue opened in the upstream by @bmeck
https://bugs.chromium.org/p/v8/issues/detail?id=9702 - Looking up properties from fronzen objects - objects from our
primordialsnamespace are frozen so lookups likeconst { Reflect } = primordials; Reflect.apply(...);is slower than justReflect.applywhenReflectcomes from the global object. This can be mitigated by caching the lookup results upfront, e.g. event: improve performance of EventEmitter.emit #29633 by @mcollina There is also a fairly odd tracking issue for this in the upstream: https://bugs.chromium.org/p/v8/issues/detail?id=6831
cc @MylesBorins (https://twitter.com/MylesBorins/status/1173390304742785024)
Activity
mcollina commentedon Sep 30, 2019
From my tests, it seems to be related to inlining.
joyeecheung commentedon Sep 30, 2019
I just noticed that things like
primordials.Reflectare not frozen, but instead the slowdown probably comes fromObject.create(null)which turns the objects into dictionary mode.devsnek commentedon Sep 30, 2019
Can we have V8 migrate everything captured in the snapshot to fast properties?
And if all else fails...
try { eval('%ToFastProperties(primordials.Reflect)') } catch {}:)joyeecheung commentedon Sep 30, 2019
cc @nodejs/v8
joyeecheung commentedon Sep 30, 2019
For reference the primordials are baked in https://github.com/nodejs/node/blob/6ce87c0/lib/internal/per_context/primordials.js
joyeecheung commentedon Oct 10, 2019
One idea about the way forward: put the primordials beind a configure-time flag, and figure out how to fix the performance regression before turning it back on.
joyeecheung commentedon Oct 10, 2019
@nodejs/process ^ any opinions on the idea in #29766 (comment) ?
addaleax commentedon Oct 10, 2019
How much would we gain by not freezing the objects, for now? That could be put behind a flag pretty easily, right?
joyeecheung commentedon Oct 11, 2019
@addaleax I think we need to identify a suitable benchmark to compare the impact of removing certain bits in the
primorials.js...any suggestions? (from the tweet I think @mcollina @MylesBorins @bmeck have experience on this)mcollina commentedon Oct 11, 2019
I just did it manually, building two node versions and run some of our microbenchmarks.
mcollina commentedon Oct 15, 2019
The way how I fixed it is by just doing a simple assignment / destructuring:
node/lib/events.js
Line 24 in ed5eaa0
I would just do that everywhere, and we would be good I think.
joyeecheung commentedon Oct 22, 2019
@mcollina Thanks, that would probably be good for a code & learn task or a beginner task. I'll see if I can get this done through nodejs/code-and-learn#97 and if not, I'll spin off a separate issue about this specific task.
buffer: improve performance caused by primordials
19 remaining items
net: destructure primordials
events: improve performance caused by primordials
net: destructure primordials
events: improve performance caused by primordials
http: improve performance caused by primordials
lib: flatten access to primordials
http: improve performance caused by primordials
lib: flatten access to primordials
aduh95 commentedon Nov 19, 2020
Both upstream issues has been closed as fixed. Should we keep this open?
MylesBorins commentedon Nov 19, 2020
Closing. please reopen if things are not fixed