Skip to content
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

doc: add ReflectConstruct to known perf issues #50111

Merged

Conversation

H4ad
Copy link
Member

@H4ad H4ad commented Oct 10, 2023

There are a lot of PRs created initially by nodejs/performance#109 to improve performance issues with this primordial.

@H4ad H4ad marked this pull request as ready for review October 10, 2023 02:36
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/tsc

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Oct 10, 2023
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW the reason isn't ReflectConstruct per-se it's the usage of anonymous (new) classes each time with it that is the problem. Still I generally agree there be very little cases where it is required.

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'm not sure it makes sense to list it here, because using Reflect.construct would not be more performant. The list (currently) lists the primordials for which it's faster to call the non-primordial version (e.g., rewriting someArray.push(1) to ArrayPrototypePush(someArray, 1) can lead to a huge perf regression in hot path). I think it makes more sense to either have it as a separate list, or maybe even a different document since it's hardly related to primordials IIUC.

@H4ad
Copy link
Member Author

H4ad commented Oct 10, 2023

Maybe:

Primordials we should avoid for performance issues

Below are some primordials that we can avoid to be more performant:

In general, it's not a problem if you don't have an alternative but sometimes you can avoid it by writing the code in a different way and have less overhead.

@aduh95
Copy link
Contributor

aduh95 commented Oct 10, 2023

Have measured if Reflect.construct is faster than ReflectConstruct? My understanding is that both perform about the same, correct? If that's the case, you should say so:

ReflectConstruct: we avoid it for the same reason we avoid the non-primordial Reflect.construct, see the issue …

Otherwise it seems not far-fetched this think the reader might conclude that Reflect.construct would be a better alternative for faster code, when it is not.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@benjamingr
Copy link
Member

I agree with @aduh95 's comments. We should avoid Reflect.construct as well - otherwise people might see this and use Reflect.construct instead of ReflectConstruct which isn't the point (which is - they should avoid creating new types of classes inside a function instead of having a shared class like in those issues)

@H4ad
Copy link
Member Author

H4ad commented Oct 10, 2023

Any recommendations on where I should put this recommendation? Should I write something like #50111 (comment) or close this PR?

Co-authored-by: Uzlopak <aras.abbasi@googlemail.com>
@H4ad H4ad requested a review from aduh95 October 18, 2023 14:52
doc/contributing/primordials.md Outdated Show resolved Hide resolved
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Oct 18, 2023
@@ -124,7 +124,7 @@ performance of code in Node.js.
* `SafePromisePrototypeFinally`: use `try {} finally {}` block instead.
* `ReflectConstruct`: Also affects `Reflect.construct`.
`ReflectConstruct` creates new types of classes inside functions.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know I am nitpicking myself.

Maybe should be "at runtime" and not "inside functions".

Sorry.

Copy link
Member Author

@H4ad H4ad Oct 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about:

ReflectConstruct instantiates a class with a custom constructor.

@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 18, 2023
@nodejs-github-bot nodejs-github-bot merged commit 340b9a8 into nodejs:main Oct 18, 2023
22 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 340b9a8

@H4ad H4ad deleted the doc/reflect-construct-known-perf-issue branch October 18, 2023 19:32
targos pushed a commit that referenced this pull request Oct 23, 2023
PR-URL: #50111
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
PR-URL: nodejs#50111
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this pull request Nov 11, 2023
PR-URL: #50111
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants