Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.2k
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Uh oh!
There was an error while loading. Please reload this page.
aduh95 commented Apr 6, 2021
jasnell commented Apr 6, 2021
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.
marsonya commented Apr 16, 2021
Sorry for leaving a review instead of comment. |
aduh95 commented Apr 17, 2021
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.
Details
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%nodejs-github-bot commented Apr 17, 2021
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.
aduh95 commented Apr 17, 2021
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. |
marsonya commented May 18, 2021
mcollina commented May 18, 2021
We need to validate that http did not regress. |
aduh95 commented May 18, 2021
@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.
aduh95 commented May 19, 2021 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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.
nodejs-github-bot commented May 20, 2021
mcollina commented May 21, 2021
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
nodejs-github-bot commented May 21, 2021 • edited by jasnell
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by jasnell
Uh oh!
There was an error while loading. Please reload this page.
jasnell commented May 21, 2021
Landed in 13ec317 |
Replace code that's vulnerable to Prototype Pollution with Primordials. PR-URL: #38117 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Replace code that's vulnerable to Prototype Pollution with Primordials. PR-URL: #38117 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]>
richardlau commented Jul 16, 2021
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.