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: replace public Map methods with primordials #36652

Closed
wants to merge 3 commits into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Dec 27, 2020

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the process Issues and PRs related to the process subsystem. label Dec 27, 2020
@PoojaDurgad PoojaDurgad added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 31, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 31, 2020
@aduh95 aduh95 added review wanted PRs that need reviews. dont-land-on-v10.x labels Jan 5, 2021
@RaisinTen
Copy link
Contributor

Does this need to be benchmarked against process?

@aduh95
Copy link
Contributor Author

aduh95 commented Jan 15, 2021

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

EDIT: Didn't show any perf regression or improvement 👍

Benchmark results
                                                      confidence improvement accuracy (*)    (**)   (***)
 process/bench-env.js operation='delete' n=1000000                   -0.19 %       ±4.42%  ±5.91%  ±7.75%
 process/bench-env.js operation='enumerate' n=1000000                 0.01 %       ±1.45%  ±1.93%  ±2.51%
 process/bench-env.js operation='get' n=1000000                      -2.35 %       ±5.17%  ±6.89%  ±8.99%
 process/bench-env.js operation='query' n=1000000                    -2.14 %       ±9.47% ±12.61% ±16.44%
 process/bench-env.js operation='set' n=1000000                      -1.77 %       ±6.70%  ±8.91% ±11.60%
 process/bench-hrtime.js type='bigint' n=1000000                      3.42 %       ±5.66%  ±7.53%  ±9.81%
 process/bench-hrtime.js type='diff' n=1000000                        2.72 %       ±3.48%  ±4.62%  ±6.02%
 process/bench-hrtime.js type='raw' n=1000000                         0.46 %       ±4.47%  ±5.95%  ±7.76%
 process/memoryUsage.js n=100000                                     -0.53 %       ±3.34%  ±4.46%  ±5.84%
 process/next-tick-breadth-args.js n=10000000                         2.98 %       ±4.18%  ±5.56%  ±7.24%
 process/next-tick-breadth.js n=10000000                              0.02 %       ±4.37%  ±5.82%  ±7.60%
 process/next-tick-depth-args.js n=7000000                            1.40 %       ±4.55%  ±6.07%  ±7.92%
 process/next-tick-depth.js n=7000000                                 0.88 %       ±3.53%  ±4.71%  ±6.18%
 process/next-tick-exec-args.js n=4000000                            -4.74 %      ±11.85% ±15.79% ±20.59%
 process/next-tick-exec.js n=4000000                                  4.45 %       ±6.18%  ±8.23% ±10.71%
 process/queue-microtask-breadth.js n=400000                         -4.26 %       ±7.29%  ±9.70% ±12.63%
 process/queue-microtask-depth.js n=1200000                          -1.21 %       ±1.53%  ±2.04%  ±2.66%
 process/resourceUsage.js n=100000                                    1.65 %       ±3.25%  ±4.33%  ±5.66%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case there are 18 comparisons, you can thus
expect the following amount of false-positive results:
  0.90 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.18 false positives, when considering a   1% risk acceptance (**, ***),
  0.02 false positives, when considering a 0.1% risk acceptance (***)

@ExE-Boss
Copy link
Contributor

Alternatively, it might be better to fix the C++ binding, so that getOptions() returns a SafeMap, just like how the C++ API handles creation of Buffer instances.

@aduh95
Copy link
Contributor Author

aduh95 commented Jan 18, 2021

Alternatively, it might be better to fix the C++ binding, so that getOptions() returns a SafeMap

Agreed, having to deal only with SafeMap objects rather than Map would be ideal. That being said, getOptionValue returns simple string, so most core developers shouldn't have to deal with the options object directly.

just like how the C++ API handles creation of Buffer instances.

I've been looking into that, and it seems all the C++ API does is create a Uint8Array and change the object's prototype:

node/src/node_buffer.cc

Lines 275 to 277 in 7efada6

Local<Uint8Array> ui = Uint8Array::New(ab, byte_offset, length);
Maybe<bool> mb =
ui->SetPrototype(env->context(), env->buffer_prototype_object());

This could be done in JS, no need to change the C++, but I wouldn't be surprised if it had worse performance (haven't measured it though).

jasnell pushed a commit that referenced this pull request Jan 23, 2021
Co-authored-by: Antoine du Hamel <[email protected]>

PR-URL: #36989
Refs: #36652
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@aduh95
Copy link
Contributor Author

aduh95 commented Jan 23, 2021

Superseded by #36989 and #37029.

@aduh95 aduh95 closed this Jan 23, 2021
@aduh95 aduh95 deleted the options-primordials branch January 23, 2021 08:55
targos pushed a commit that referenced this pull request Feb 2, 2021
Co-authored-by: Antoine du Hamel <[email protected]>

PR-URL: #36989
Refs: #36652
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request May 25, 2021
Co-authored-by: Antoine du Hamel <[email protected]>

PR-URL: #36989
Refs: #36652
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request May 30, 2021
Co-authored-by: Antoine du Hamel <[email protected]>

PR-URL: #36989
Refs: #36652
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Jun 5, 2021
Co-authored-by: Antoine du Hamel <[email protected]>

PR-URL: #36989
Refs: #36652
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Jun 11, 2021
Co-authored-by: Antoine du Hamel <[email protected]>

PR-URL: #36989
Refs: #36652
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
process Issues and PRs related to the process subsystem. review wanted PRs that need reviews.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants