-
-
Notifications
You must be signed in to change notification settings - Fork 34.2k
lib: revert primordials in a hot path #38248
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
Conversation
benjamingr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if benchmarks look as good as the other one
|
Benchmark CI got stuck on Details |
|
I have done a direct comparison between the 2 PRs (rebased on top of master) for ~9 hours on my workstation, and after half a day of crunching I got this: DetailsAll the other HTTP benchmarks I could do confirm that those two PRs are roughly equivalent. |
mcollina
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
|
There are probably more to check/verify/revert in different hot paths, but this matches what I found (so far) for http. |
Evidence has shown that use of primordials have sometimes an impact of performance. This commit reverts the changes who are most likely to be responsible for performance regression in the HTTP response path.
39292b9 to
680ecc4
Compare
Evidence has shown that use of primordials have sometimes an impact of performance. This commit reverts the changes who are most likely to be responsible for performance regression in the HTTP response path. PR-URL: #38248 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Rich Trott <[email protected]>
|
Landed in ee9e2a2 |
Evidence has shown that use of primordials have sometimes an impact of performance. This commit reverts the changes who are most likely to be responsible for performance regression in the HTTP response path. PR-URL: nodejs#38248 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Evidence has shown that use of primordials have sometimes an impact of performance. This commit reverts the changes who are most likely to be responsible for performance regression in the HTTP response path. PR-URL: #38248 Backport-PR-URL: #38972 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Evidence has shown that use of primordials have sometimes an impact of
performance. This commit reverts the changes who are most likely to be
responsible for performance regression in the HTTP response path.
This is an alternative to #38246. My assumption would this PR has the same effect on benchmark results.
%Function.prototype%instead of() => {}sometimes decrease performance (maybe because V8 doesn't inline%Function.prototype%?).primordials.FunctionPrototypeCallorprimordials.ReflectApplyto call super constructor (rather than accessing.call()) has shown to sometimes decrease performance in previous PRs.primordials.FunctionPrototypeBindis less performant than accessing.bind().Buffer.prototype.slicecan be much faster than%TypedArrayPrototype.slice%.If this PR shows same performance as #38246, I'd like to run several benchmarks to identify what are the changes that actually have an impact on HTTP performance.
EDIT: Added changes:
FunctionPrototypeCallandReflectApplyon hot path have been replaced with.call()or.apply().ArrayPrototypePush,ArrayPrototypeSplice, etc.) have been replaced with the non-primordials equivalent.ArrayPrototypeForEachon hot path have been replaced withfor(;;)loops (which seems the most performance alternative).StringPrototypeToLowerCaseininternal/utilshave been replaced with.toLowerCase()based on lib: revert primordials in a hot path #38246 (comment).internal/util/debugloghave been refactored to avoid creatingSafeArrayIteratorin most cases.