doc: add ReflectConstruct to known perf issues#50111
doc: add ReflectConstruct to known perf issues#50111nodejs-github-bot merged 5 commits intonodejs:mainfrom
Conversation
|
Review requested:
|
benjamingr
left a comment
benjamingr
left a comment
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Maybe: Primordials we should avoid for performance issuesBelow 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. |
|
Have measured if
Otherwise it seems not far-fetched this think the reader might conclude that |
|
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) |
|
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>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
aduh95
added
author ready
| @@ -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. | |||
There was a problem hiding this comment.
I know I am nitpicking myself.
Maybe should be "at runtime" and not "inside functions".
Sorry.
There was a problem hiding this comment.
What about:
ReflectConstructinstantiates a class with a custom constructor.
nodejs-github-bot
removed
the
commit-queue
|
Landed in 340b9a8 |
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>
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>
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>
8 participants
There are a lot of PRs created initially by nodejs/performance#109 to improve performance issues with this primordial.