-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
Performance regression in v12 caused by primordials #29766
Comments
From my tests, it seems to be related to inlining. |
I just noticed that things like |
Can we have V8 migrate everything captured in the snapshot to fast properties? And if all else fails... |
cc @nodejs/v8 |
For reference the primordials are baked in https://github.com/nodejs/node/blob/6ce87c0/lib/internal/per_context/primordials.js |
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. |
@nodejs/process ^ any opinions on the idea in #29766 (comment) ? |
How much would we gain by not freezing the objects, for now? That could be put behind a flag pretty easily, right? |
@addaleax I think we need to identify a suitable benchmark to compare the impact of removing certain bits in the |
I just did it manually, building two node versions and run some of our microbenchmarks. |
The way how I fixed it is by just doing a simple assignment / destructuring: Line 24 in ed5eaa0
I would just do that everywhere, and we would be good I think. |
@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. |
This is my first PR, and it's based on the code-and-learn guidances This restore some performance after introducing primordialias. Fixes: nodejs#29766 Refs: nodejs/code-and-learn#97 Refs: nodejs#29633
@joyeecheung Should i specify as |
@jizusun |
This is my first PR, and it's based on the code-and-learn guidances This restore some performance after introducing primordialias. Refs: #29766 Refs: nodejs/code-and-learn#97 Refs: #29633 PR-URL: #30235 Refs: #29766 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
This is my first PR, and it's based on the code-and-learn guidances This restore some performance after introducing primordialias. Refs: #29766 Refs: nodejs/code-and-learn#97 Refs: #29633 PR-URL: #30235 Refs: #29766 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
This is my first PR, and it's based on the code-and-learn guidances This restore some performance after introducing primordialias. Refs: #29766 Refs: nodejs/code-and-learn#97 Refs: #29633 PR-URL: #30235 Refs: #29766 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
This is my first PR, and it's based on the code-and-learn guidances This restore some performance after introducing primordialias. Refs: #29766 Refs: nodejs/code-and-learn#97 Refs: #29633 PR-URL: #30235 Refs: #29766 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
This is my first PR, and it's based on the code-and-learn guidances This restore some performance after introducing primordialias. Refs: #29766 Refs: nodejs/code-and-learn#97 Refs: #29633 PR-URL: #30235 Refs: #29766 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
This is my first PR, and it's based on the code-and-learn guidances This restore some performance after introducing primordialias. Refs: #29766 Refs: nodejs/code-and-learn#97 Refs: #29633 PR-URL: #30235 Refs: #29766 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Refs: nodejs#29766 This works on destructuring primordials whithin libs/_http_agent
Refs: #29766 This works on destructuring primordials whithin libs/_http_agent PR-URL: #30416 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
PR-URL: #30577 Refs: nodejs/code-and-learn#97 Refs: #29766 Refs: #29633 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Store all primordials as properties of the primordials object. Static functions are prefixed by the constructor's name and prototype methods are prefixed by the constructor's name followed by "Prototype". For example: primordials.Object.keys becomes primordials.ObjectKeys. PR-URL: #30610 Refs: #29766 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Refs: #29766 This works on destructuring primordials whithin libs/_http_agent PR-URL: #30416 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
PR-URL: #30577 Refs: nodejs/code-and-learn#97 Refs: #29766 Refs: #29633 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Store all primordials as properties of the primordials object. Static functions are prefixed by the constructor's name and prototype methods are prefixed by the constructor's name followed by "Prototype". For example: primordials.Object.keys becomes primordials.ObjectKeys. Backport-PR-URL: nodejs#30731 PR-URL: nodejs#30610 Refs: nodejs#29766 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Store all primordials as properties of the primordials object. Static functions are prefixed by the constructor's name and prototype methods are prefixed by the constructor's name followed by "Prototype". For example: primordials.Object.keys becomes primordials.ObjectKeys. Backport-PR-URL: #30731 PR-URL: #30610 Refs: #29766 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
PR-URL: #30577 Refs: nodejs/code-and-learn#97 Refs: #29766 Refs: #29633 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
PR-URL: #30577 Refs: nodejs/code-and-learn#97 Refs: #29766 Refs: #29633 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Refs: #29766 This works on destructuring primordials whithin libs/_http_agent PR-URL: #30416 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Store all primordials as properties of the primordials object. Static functions are prefixed by the constructor's name and prototype methods are prefixed by the constructor's name followed by "Prototype". For example: primordials.Object.keys becomes primordials.ObjectKeys. Backport-PR-URL: #30731 PR-URL: #30610 Refs: #29766 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Refs: #29766 This works on destructuring primordials whithin libs/_http_agent PR-URL: #30416 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Store all primordials as properties of the primordials object. Static functions are prefixed by the constructor's name and prototype methods are prefixed by the constructor's name followed by "Prototype". For example: primordials.Object.keys becomes primordials.ObjectKeys. Backport-PR-URL: #30731 PR-URL: #30610 Refs: #29766 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Both upstream issues has been closed as fixed. Should we keep this open? |
Closing. please reopen if things are not fixed |
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:
Function.prototype.{call, apply}
instead of just calling them directly. There is an issue opened in the upstream by @bmeckhttps://bugs.chromium.org/p/v8/issues/detail?id=9702
primordials
namespace are frozen so lookups likeconst { Reflect } = primordials; Reflect.apply(...);
is slower than justReflect.apply
whenReflect
comes 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=6831cc @MylesBorins (https://twitter.com/MylesBorins/status/1173390304742785024)
The text was updated successfully, but these errors were encountered: