Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.3k
lib: add primordials.SafeArrayIterator#36532
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
aduh95 commented Dec 15, 2020 • 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.
Uh oh!
There was an error while loading. Please reload this page.
mscdex left a comment • 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.
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's quite a few performance regressions here. I expect these will show up for other subsystem benchmarks as well due to the code that is being changed here, but here are the Buffer creation benchmark results for example:
confidence improvement accuracy (*) (**) (***) buffers/buffer-creation.js n=600000 len=1024 type='fast-alloc' *** -16.77 % ±3.11% ±4.15% ±5.42% buffers/buffer-creation.js n=600000 len=1024 type='fast-alloc-fill' *** -19.31 % ±4.12% ±5.50% ±7.18% buffers/buffer-creation.js n=600000 len=1024 type='fast-allocUnsafe' *** -60.95 % ±2.93% ±3.91% ±5.12% buffers/buffer-creation.js n=600000 len=1024 type='slow-allocUnsafe' *** -25.14 % ±3.70% ±4.93% ±6.43% buffers/buffer-creation.js n=600000 len=10 type='fast-alloc' *** -84.48 % ±4.53% ±6.09% ±8.07% buffers/buffer-creation.js n=600000 len=10 type='fast-alloc-fill' *** -80.82 % ±4.60% ±6.18% ±8.16% buffers/buffer-creation.js n=600000 len=10 type='fast-allocUnsafe' *** -87.77 % ±7.33% ±9.88% ±13.12% buffers/buffer-creation.js n=600000 len=10 type='slow-allocUnsafe' *** -83.53 % ±6.69% ±9.01% ±11.95% buffers/buffer-creation.js n=600000 len=4096 type='fast-alloc' *** -16.15 % ±2.99% ±3.98% ±5.18% buffers/buffer-creation.js n=600000 len=4096 type='fast-alloc-fill' *** -18.88 % ±3.98% ±5.31% ±6.92% buffers/buffer-creation.js n=600000 len=4096 type='fast-allocUnsafe' *** -28.35 % ±3.52% ±4.70% ±6.15% buffers/buffer-creation.js n=600000 len=4096 type='slow-allocUnsafe' *** -26.75 % ±3.74% ±4.98% ±6.49% buffers/buffer-creation.js n=600000 len=8192 type='fast-alloc' *** -11.73 % ±3.51% ±4.67% ±6.08% buffers/buffer-creation.js n=600000 len=8192 type='fast-alloc-fill' *** -14.17 % ±3.54% ±4.72% ±6.15% buffers/buffer-creation.js n=600000 len=8192 type='fast-allocUnsafe' *** -27.15 % ±3.93% ±5.25% ±6.87% buffers/buffer-creation.js n=600000 len=8192 type='slow-allocUnsafe' *** -26.28 % ±3.97% ±5.32% ±6.98% Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
aduh95 commented Dec 21, 2020 • 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.
@mscdex Please check out the last benchmark results, they seem much better now. Details |
9a728a4 to def229aCompareaduh95 commented Dec 22, 2020 • 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: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/789/ Details |
Trott 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 once unblocked (which presumably will make the failing tests pass)
Trott commented Dec 23, 2020
@nodejs/modules |
Trott 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 once unblocked (which presumably will make the failing tests pass)
Using an explicit constructor is necessary to avoid relying on `Array.prototype[Symbol.iterator]` and `%ArrayIteratorPrototype%.next`, which can be mutated by users. PR-URL: #36587 Refs: #36428 Refs: #36532 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
def229a to b975a4aComparenodejs-github-bot commented Dec 25, 2020
aduh95 commented Dec 26, 2020
Benchmark CI (events): https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/803/console Those don't show any perf regression either. Details |
Commit Queue failed- Loading data for nodejs/node/pull/36532 ✔ Done loading data for nodejs/node/pull/36532 ----------------------------------- PR info ------------------------------------ Title lib: add primordials.SafeArrayIterator (#36532) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch aduh95:safe-array-iterator -> nodejs:master Labels author ready, lib / src Commits 1 - lib: add primordials.SafeArrayIterator Committers 1 - Antoine du Hamel PR-URL: https://github.com/nodejs/node/pull/36532 Reviewed-By: Rich Trott ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/36532 Reviewed-By: Rich Trott -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last review: ⚠ - lib: add primordials.SafeArrayIterator ✔ Last GitHub Actions successful ℹ Last Benchmark CI on 2020-12-26T10:16:57Z: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/804/console ℹ Last Full PR CI on 2020-12-25T19:32:19Z: https://ci.nodejs.org/job/node-test-pull-request/35080/ - Querying data for job/node-test-pull-request/804/ ✔ Last Jenkins CI successful ℹ This PR was created on Tue, 15 Dec 2020 17:21:16 GMT ✔ Approvals: 1 ✔ - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/36532#pullrequestreview-557887739 -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/447379775 |
PR-URL: nodejs#36532 Reviewed-By: Rich Trott <[email protected]>
b975a4a to 0b6d307Compareaduh95 commented Dec 27, 2020
Landed in 0b6d307 |
Using an explicit constructor is necessary to avoid relying on `Array.prototype[Symbol.iterator]` and `%ArrayIteratorPrototype%.next`, which can be mutated by users. PR-URL: #36587 Refs: #36428 Refs: #36532 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: #36532 Reviewed-By: Rich Trott <[email protected]>
Using an explicit constructor is necessary to avoid relying on `Array.prototype[Symbol.iterator]` and `%ArrayIteratorPrototype%.next`, which can be mutated by users. PR-URL: #36587 Refs: #36428 Refs: #36532 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Using an explicit constructor is necessary to avoid relying on `Array.prototype[Symbol.iterator]` and `%ArrayIteratorPrototype%.next`, which can be mutated by users. PR-URL: #36587 Refs: #36428 Refs: #36532 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Using an explicit constructor is necessary to avoid relying on `Array.prototype[Symbol.iterator]` and `%ArrayIteratorPrototype%.next`, which can be mutated by users. PR-URL: #36587 Refs: #36428 Refs: #36532 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#36532 Reviewed-By: Rich Trott <[email protected]>
PR-URL: #36532 Backport-PR-URL: #39446 Reviewed-By: Rich Trott <[email protected]>
PR-URL: #36532 Backport-PR-URL: #39446 Reviewed-By: Rich Trott <[email protected]>
PR-URL: nodejs#36532 Backport-PR-URL: nodejs#39446 Reviewed-By: Rich Trott <[email protected]>
Make Node.js module loading resilient to prototype tempering from user-mode.
Blocked by #36428 and #36587.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes