Skip to content
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: make primordials Promise static methods safe #38843

Closed
wants to merge 2 commits into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented May 29, 2021

Promise static methods that iterate over the provided argument (%Promise.all%, %Promise.any%, ...) look up the then property over each promise to support promise subclassing. This PR is introducing SafePromiseAll, SafePromiseAny, etc. that take a array, safely iterate over it, and wrap each promise in a SafePromise whose prototype is accessible from userland.

@github-actions github-actions bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels May 29, 2021
@aduh95
Copy link
Contributor Author

aduh95 commented May 29, 2021

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1035/
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1036/

Results
                                                                   confidence improvement accuracy (*)    (**)   (***)
esm/cjs-parse.js n=100                                                            -0.16 %       ±2.51%  ±3.35%  ±4.36%
vm/create-context.js n=100                                                        -2.59 %       ±5.41%  ±7.20%  ±9.37%
vm/run-in-context.js withSigintListener=0 breakOnSigint=0 n=1                     -1.88 %       ±5.88%  ±7.86% ±10.30%
vm/run-in-context.js withSigintListener=0 breakOnSigint=1 n=1               *     -5.60 %       ±4.85%  ±6.49%  ±8.53%
vm/run-in-context.js withSigintListener=1 breakOnSigint=0 n=1                      0.56 %       ±5.97%  ±7.96% ±10.39%
vm/run-in-context.js withSigintListener=1 breakOnSigint=1 n=1                     -2.22 %       ±5.93%  ±7.92% ±10.37%
vm/run-in-this-context.js withSigintListener=0 breakOnSigint=0 n=1                 0.24 %       ±7.36%  ±9.80% ±12.77%
vm/run-in-this-context.js withSigintListener=0 breakOnSigint=1 n=1                 2.13 %       ±7.51% ±10.02% ±13.09%
vm/run-in-this-context.js withSigintListener=1 breakOnSigint=0 n=1                -1.83 %       ±7.34%  ±9.76% ±12.72%
vm/run-in-this-context.js withSigintListener=1 breakOnSigint=1 n=1                 1.20 %       ±8.42% ±11.21% ±14.60%
                                                                                  confidence improvement accuracy (*)    (**)   (***)
module/module-loader-circular.js n=10000                                                          0.04 %       ±3.49%  ±4.64%  ±6.04%
module/module-loader-deep.js cache='false' files=1000 ext=''                                     -2.08 %       ±3.24%  ±4.30%  ±5.60%
module/module-loader-deep.js cache='false' files=1000 ext='.js'                                   0.62 %       ±3.27%  ±4.35%  ±5.66%
module/module-loader-deep.js cache='true' files=1000 ext=''                                      -1.90 %      ±11.10% ±14.79% ±19.30%
module/module-loader-deep.js cache='true' files=1000 ext='.js'                                   -2.77 %       ±7.66% ±10.20% ±13.31%
module/module-loader.js cache='false' n=1000 files=500 dir='abs' name='/'                        -1.45 %       ±3.49%  ±4.66%  ±6.10%
module/module-loader.js cache='false' n=1000 files=500 dir='abs' name=''                          0.08 %       ±2.63%  ±3.50%  ±4.56%
module/module-loader.js cache='false' n=1000 files=500 dir='abs' name='/index.js'                 1.05 %       ±4.90%  ±6.54%  ±8.56%
module/module-loader.js cache='false' n=1000 files=500 dir='rel' name='/'                        -0.67 %       ±3.33%  ±4.43%  ±5.77%
module/module-loader.js cache='false' n=1000 files=500 dir='rel' name=''                          1.37 %       ±2.99%  ±3.98%  ±5.19%
module/module-loader.js cache='false' n=1000 files=500 dir='rel' name='/index.js'                -4.29 %       ±5.37%  ±7.18%  ±9.42%
module/module-loader.js cache='false' n=1 files=500 dir='abs' name='/'                            1.00 %       ±5.19%  ±6.90%  ±8.99%
module/module-loader.js cache='false' n=1 files=500 dir='abs' name=''                             2.36 %       ±3.76%  ±5.00%  ±6.51%
module/module-loader.js cache='false' n=1 files=500 dir='abs' name='/index.js'                   -2.73 %       ±8.41% ±11.21% ±14.65%
module/module-loader.js cache='false' n=1 files=500 dir='rel' name='/'                           -2.04 %       ±6.41%  ±8.55% ±11.18%
module/module-loader.js cache='false' n=1 files=500 dir='rel' name=''                             0.02 %       ±4.06%  ±5.41%  ±7.04%
module/module-loader.js cache='false' n=1 files=500 dir='rel' name='/index.js'                    0.71 %       ±4.59%  ±6.11%  ±7.96%
module/module-loader.js cache='true' n=1000 files=500 dir='abs' name='/'                          0.27 %       ±3.21%  ±4.27%  ±5.56%
module/module-loader.js cache='true' n=1000 files=500 dir='abs' name=''                           1.11 %       ±1.99%  ±2.66%  ±3.47%
module/module-loader.js cache='true' n=1000 files=500 dir='abs' name='/index.js'                 -0.10 %       ±1.47%  ±1.96%  ±2.56%
module/module-loader.js cache='true' n=1000 files=500 dir='rel' name='/'                   *      2.93 %       ±2.52%  ±3.36%  ±4.38%
module/module-loader.js cache='true' n=1000 files=500 dir='rel' name=''                           0.62 %       ±1.75%  ±2.34%  ±3.05%
module/module-loader.js cache='true' n=1000 files=500 dir='rel' name='/index.js'                 -0.94 %       ±3.29%  ±4.39%  ±5.77%
module/module-loader.js cache='true' n=1 files=500 dir='abs' name='/'                            -2.77 %       ±4.21%  ±5.61%  ±7.33%
module/module-loader.js cache='true' n=1 files=500 dir='abs' name=''                              1.73 %       ±3.97%  ±5.34%  ±7.04%
module/module-loader.js cache='true' n=1 files=500 dir='abs' name='/index.js'                    -1.19 %       ±5.13%  ±6.87%  ±9.04%
module/module-loader.js cache='true' n=1 files=500 dir='rel' name='/'                            -3.26 %       ±4.86%  ±6.47%  ±8.44%
module/module-loader.js cache='true' n=1 files=500 dir='rel' name=''                             -0.33 %       ±1.55%  ±2.06%  ±2.69%
module/module-loader.js cache='true' n=1 files=500 dir='rel' name='/index.js'                    -2.30 %       ±4.68%  ±6.25%  ±8.17%
module/module-require.js n=10000 type='dir'                                                       5.17 %       ±7.56% ±10.06% ±13.09%
module/module-require.js n=10000 type='.js'                                                       1.35 %       ±8.25% ±10.99% ±14.34%
module/module-require.js n=10000 type='.json'                                                     2.61 %       ±8.41% ±11.20% ±14.58%

@aduh95 aduh95 marked this pull request as ready for review May 29, 2021 20:37
@aduh95 aduh95 added the review wanted PRs that need reviews. label Jun 9, 2021
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am against changing further code to use primordials as outlined here #30697 (comment).

@aduh95
Copy link
Contributor Author

aduh95 commented Jun 11, 2021

I am against changing further code to use primordials as outlined here #30697 (comment).

Note that this change removes the SafeArrayIterator calls – which IMHO improves the readability of the code. I thought I should point it out, since it's one of the point outlined in the linked comment. If we leave the performance aspect on the side, would you say this PR is going in the right direction or quite the contrary?

@targos
Copy link
Member

targos commented Jun 12, 2021

I'm generally uncomfortable about reimplementing standard methods to avoid the fact that they call user-mutable things.

@aduh95 aduh95 mentioned this pull request Aug 26, 2021
3 tasks
@aduh95
Copy link
Contributor Author

aduh95 commented Jul 10, 2022

Superseded by #43728.

@aduh95 aduh95 closed this Jul 10, 2022
@aduh95 aduh95 deleted the safer-promise-statics branch July 10, 2022 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. review wanted PRs that need reviews.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants