-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
lib: revert primordials in a hot path #38248
Conversation
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
|
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:
All the other HTTP benchmarks I could do confirm that those two PRs are roughly equivalent. |
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 <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
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 <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
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 <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
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.FunctionPrototypeCall
orprimordials.ReflectApply
to call super constructor (rather than accessing.call()
) has shown to sometimes decrease performance in previous PRs.primordials.FunctionPrototypeBind
is less performant than accessing.bind()
.Buffer.prototype.slice
can 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:
FunctionPrototypeCall
andReflectApply
on hot path have been replaced with.call()
or.apply()
.ArrayPrototypePush
,ArrayPrototypeSplice
, etc.) have been replaced with the non-primordials equivalent.ArrayPrototypeForEach
on hot path have been replaced withfor(;;)
loops (which seems the most performance alternative).StringPrototypeToLowerCase
ininternal/utils
have been replaced with.toLowerCase()
based on lib: revert primordials in a hot path #38246 (comment).internal/util/debuglog
have been refactored to avoid creatingSafeArrayIterator
in most cases.