-
-
Notifications
You must be signed in to change notification settings - Fork 33.9k
events: refactor to use primordials in lib/events #38117
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
|
Benchmarks should be checked on this before landing |
aduh95
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.
Some significant perf regressions:
confidence improvement accuracy (*) (**) (***)
events/ee-add-remove.jsn=1000000 *** -27.64 % ±3.09% ±4.13% ±5.40%
events/ee-emit.jslisteners=10 argc=0 n=2000000 0.28 % ±1.22% ±1.63% ±2.12%
events/ee-emit.jslisteners=10 argc=10 n=2000000 0.44 % ±1.47% ±1.96% ±2.58%
events/ee-emit.jslisteners=10 argc=2 n=2000000 0.37 % ±1.27% ±1.69% ±2.20%
events/ee-emit.jslisteners=10 argc=4 n=2000000 0.04 % ±1.07% ±1.43% ±1.86%
events/ee-emit.jslisteners=1 argc=0 n=2000000 -2.06 % ±2.67% ±3.55% ±4.62%
events/ee-emit.jslisteners=1 argc=10 n=2000000 -1.66 % ±3.41% ±4.54% ±5.91%
events/ee-emit.jslisteners=1 argc=2 n=2000000 -4.38 % ±4.59% ±6.11% ±7.96%
events/ee-emit.jslisteners=1 argc=4 n=2000000 -1.97 % ±3.47% ±4.63% ±6.03%
events/ee-emit.jslisteners=5 argc=0 n=2000000 -0.08 % ±2.29% ±3.04% ±3.96%
events/ee-emit.jslisteners=5 argc=10 n=2000000 -0.77 % ±1.88% ±2.51% ±3.26%
events/ee-emit.jslisteners=5 argc=2 n=2000000 -0.76 % ±1.97% ±2.63% ±3.43%
events/ee-emit.jslisteners=5 argc=4 n=2000000 -0.21 % ±2.09% ±2.78% ±3.62%
events/ee-listener-count-on-prototype.jsn=50000000 * -5.06 % ±4.62% ±6.21% ±8.20%
events/ee-listeners.jsraw='false' listeners=50 n=5000000 1.91 % ±3.18% ±4.26% ±5.58%
events/ee-listeners.jsraw='false' listeners=5 n=5000000 -1.18 % ±3.36% ±4.50% ±5.91%
events/ee-listeners.jsraw='true' listeners=50 n=5000000 1.67 % ±2.85% ±3.80% ±4.98%
events/ee-listeners.jsraw='true' listeners=5 n=5000000 2.24 % ±3.54% ±4.71% ±6.14%
events/ee-once.jsargc=0 n=20000000 *** -6.95 % ±1.40% ±1.86% ±2.43%
events/ee-once.jsargc=1 n=20000000 *** -9.55 % ±1.17% ±1.56% ±2.03%
events/ee-once.jsargc=4 n=20000000 *** -9.36 % ±4.11% ±5.47% ±7.12%
events/ee-once.jsargc=5 n=20000000 *** -8.47 % ±2.74% ±3.64% ±4.74%
events/eventtarget.jslisteners=10 n=1000000 * 3.56 % ±3.01% ±4.02% ±5.28%
events/eventtarget.jslisteners=1 n=1000000 -3.34 % ±4.28% ±5.74% ±7.54%
events/eventtarget.jslisteners=5 n=1000000 0.94 % ±2.38% ±3.16% ±4.12%
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.
I'm -1 to land this with regressions.
marsonya
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.
These are the primordials causing the Regressions in my previous commit
ArrayPrototypeUnshift and ArrayPrototypePush in _addListener()
FunctionPrototypeCall and ReflectApply in onceWrapper()
FunctionPrototypeBind in _onceWrap()
ArrayPrototypeShift in removeListener()
ArrayPrototypeShift in on()
I have removed these and benchmarked events again (locally on my computer).
The regressions are gone.
Kindly re-run benchmarks once again.
Sorry for leaving a review instead of comment. |
RaisinTen
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.
There does not seem to be any significant performance regressions in the bechmark run.
14:30:14 confidence improvement accuracy (*) (**) (***)
14:30:15 events/ee-add-remove.js n=1000000 -1.98 % ±5.20% ±6.92% ±9.01%
14:30:15 events/ee-emit.js listeners=10 argc=0 n=2000000 0.07 % ±1.19% ±1.58% ±2.06%
14:30:15 events/ee-emit.js listeners=10 argc=10 n=2000000 1.95 % ±3.77% ±5.05% ±6.64%
14:30:15 events/ee-emit.js listeners=10 argc=2 n=2000000 0.08 % ±0.81% ±1.08% ±1.40%
14:30:15 events/ee-emit.js listeners=10 argc=4 n=2000000 0.40 % ±0.98% ±1.30% ±1.69%
14:30:15 events/ee-emit.js listeners=1 argc=0 n=2000000 1.63 % ±4.24% ±5.64% ±7.34%
14:30:15 events/ee-emit.js listeners=1 argc=10 n=2000000 -0.60 % ±3.49% ±4.64% ±6.04%
14:30:15 events/ee-emit.js listeners=1 argc=2 n=2000000 -1.54 % ±4.00% ±5.32% ±6.93%
14:30:15 events/ee-emit.js listeners=1 argc=4 n=2000000 -5.19 % ±7.62% ±10.23% ±13.53%
14:30:15 events/ee-emit.js listeners=5 argc=0 n=2000000 -1.80 % ±2.15% ±2.86% ±3.73%
14:30:15 events/ee-emit.js listeners=5 argc=10 n=2000000 -0.44 % ±2.13% ±2.83% ±3.69%
14:30:15 events/ee-emit.js listeners=5 argc=2 n=2000000 0.56 % ±2.15% ±2.86% ±3.73%
14:30:15 events/ee-emit.js listeners=5 argc=4 n=2000000 -1.72 % ±2.54% ±3.38% ±4.40%
14:30:15 events/ee-listener-count-on-prototype.js n=50000000 0.19 % ±5.10% ±6.79% ±8.85%
14:30:15 events/ee-listeners.js raw='false' listeners=50 n=5000000 0.57 % ±1.29% ±1.72% ±2.23%
14:30:15 events/ee-listeners.js raw='false' listeners=5 n=5000000 0.65 % ±1.37% ±1.82% ±2.37%
14:30:15 events/ee-listeners.js raw='true' listeners=50 n=5000000 -2.61 % ±3.80% ±5.10% ±6.73%
14:30:15 events/ee-listeners.js raw='true' listeners=5 n=5000000 -1.70 % ±5.65% ±7.58% ±9.99%
14:30:15 events/ee-once.js argc=0 n=20000000 1.25 % ±1.45% ±1.94% ±2.54%
14:30:15 events/ee-once.js argc=1 n=20000000 -1.49 % ±2.15% ±2.88% ±3.79%
14:30:15 events/ee-once.js argc=4 n=20000000 -0.29 % ±1.31% ±1.75% ±2.28%
14:30:15 events/ee-once.js argc=5 n=20000000 0.09 % ±1.34% ±1.79% ±2.32%
14:30:15 events/eventtarget.js listeners=10 n=1000000 0.94 % ±2.36% ±3.15% ±4.10%
14:30:15 events/eventtarget.js listeners=1 n=1000000 0.07 % ±1.75% ±2.34% ±3.05%
14:30:15 events/eventtarget.js listeners=5 n=1000000 * 7.80 % ±6.65% ±8.95% ±11.85%
aduh95
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.
Benchmark results look promising! It's probably a good idea to wait for #38248 to land first as it's going to create conflicts and we want to include it in the v16.0.0 release.
|
Once this has been rebased on top of #38248 (after it has landed), we should also run HTTP benchmark to make sure we're not introducing hidden perf regressions. |
|
We need to validate that http did not regress. |
|
@marsonya if you want to rebase on top of master to fix the git conflict, we can spawn some benchmarks to see where this PR stands in term of perf regressions/improvment. |
Replace code that's vulnerable to Prototype Pollution with Primordials.
|
Benchmark CI (events): https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1019/ Results |
aduh95
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.
Benchmark results look OK.
|
I would like to do some checks of my own before landing. |
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.
Temporary -1 until I can test this as well.
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 on my end
|
Landed in 13ec317 |
Replace code that's vulnerable to Prototype Pollution with Primordials. PR-URL: #38117 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Replace code that's vulnerable to Prototype Pollution with Primordials. PR-URL: #38117 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
|
I'm going to mark this (and any other "use primordials" I come across) as requiring a manual backport due to the recent discussions around use of primordials in current and the fact that v14.x has an older version of V8 so if we did land these we would probably want to check benchmarks. |
Replace code that's vulnerable to Prototype Pollution with Primordials.