Skip to content

lib: replace public Map methods with primordials#36652

Closed
aduh95 wants to merge 3 commits intonodejs:masterfrom
aduh95:options-primordials
Closed

lib: replace public Map methods with primordials#36652
aduh95 wants to merge 3 commits intonodejs:masterfrom
aduh95:options-primordials

Conversation

@aduh95
Copy link
Contributor

@aduh95 aduh95 commented Dec 27, 2020 •
edited
Loading

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
@aduh95 aduh95 mentioned this pull request Jan 15, 2021
Merged
@RaisinTen
Copy link
Member

RaisinTen commented Jan 15, 2021

Does this need to be benchmarked against process?

@aduh95
Copy link
Contributor Author

aduh95 commented Jan 15, 2021 •
edited
Loading

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

ExE-Boss commented Jan 17, 2021

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 •
edited
Loading

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).

@ExE-Boss ExE-Boss mentioned this pull request Jan 19, 2021
Closed
jasnell pushed a commit that referenced this pull request Jan 23, 2021
@ExE-Boss @jasnell
lib: support returning Safe collections from C++
3ec7114 
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
@ExE-Boss @targos
lib: support returning Safe collections from C++
56377d6 
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
@ExE-Boss @targos
lib: support returning Safe collections from C++
bccabe3 
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
@ExE-Boss @targos
lib: support returning Safe collections from C++
26bd60e 
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
@ExE-Boss @targos
lib: support returning Safe collections from C++
a128997 
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
@ExE-Boss @targos
lib: support returning Safe collections from C++
346fc0a 
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

Reviewers

@Lxxyx Lxxyx Lxxyx approved these changes

Assignees

No one assigned

Labels

process Issues and PRs related to the process subsystem. review wanted PRs that need reviews.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@aduh95 @RaisinTen @ExE-Boss @Lxxyx @nodejs-github-bot @PoojaDurgad

Comments