lib: add TypedArray to the primordials by targos · Pull Request #32127 · nodejs/node · GitHub

This also makes it possible to use Symbol methods and getters.

Checklist

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

For some reason this seems to have some regressions at least in the buffer-fill benchmarks here:

                                                                          confidence improvement accuracy (*)    (**)   (***)
buffers/buffer-fill.js n=20000 size=65536 type='fill("t", 0)'                     **     -4.21 %       ±3.09%  ±4.12%  ±5.37%
buffers/buffer-fill.js n=20000 size=8192 type='fill(0)'                           **     -8.96 %       ±6.07%  ±8.07% ±10.51%
buffers/buffer-fill.js n=20000 size=8192 type='fill(100)'                         **    -13.32 %       ±7.90% ±10.52% ±13.69%
buffers/buffer-fill.js n=20000 size=8192 type='fill(400)'                        ***    -10.50 %       ±5.74%  ±7.64%  ±9.97%
buffers/buffer-fill.js n=20000 size=8192 type='fill("t")'                        ***    -11.58 %       ±4.79%  ±6.40%  ±8.39%
buffers/buffer-fill.js n=20000 size=8192 type='fill("t", 0)'                      **     -7.43 %       ±4.58%  ±6.09%  ±7.94%

Let's revisit after V8 8.0+ lands on master. The overhead should be fixed

Let's revisit after V8 8.0+ lands on master. The overhead should be fixed

Is it currently possible to at least run these specific benchmarks with V8 8.x to verify the issues are resolved there?

you can try with this canary build (V8 8.1): https://nodejs.org/download/v8-canary/v14.0.0-v8-canary20200121fda488ae67/

canary-base vs. canary-base + this change:

                                                                           confidence improvement accuracy (*)    (**)   (***)
 buffers/buffer-fill.js n=20000 size=65536 type='fill("")'                                 0.56 %       ±3.33%  ±4.43%  ±5.76%
 buffers/buffer-fill.js n=20000 size=65536 type='fill("t", "utf8")'                       -1.56 %       ±3.37%  ±4.48%  ±5.83%
 buffers/buffer-fill.js n=20000 size=65536 type='fill("t", 0, "utf8")'                    -0.24 %       ±2.81%  ±3.75%  ±4.89%
 buffers/buffer-fill.js n=20000 size=65536 type='fill("t", 0)'                      *     -3.64 %       ±3.20%  ±4.26%  ±5.55%
 buffers/buffer-fill.js n=20000 size=65536 type='fill("t")'                               -1.10 %       ±3.50%  ±4.66%  ±6.08%
 buffers/buffer-fill.js n=20000 size=65536 type='fill("test")'                             0.55 %       ±2.87%  ±3.82%  ±4.97%
 buffers/buffer-fill.js n=20000 size=65536 type='fill(0)'                                 -2.26 %       ±2.91%  ±3.87%  ±5.04%
 buffers/buffer-fill.js n=20000 size=65536 type='fill(100)'                               -0.66 %       ±3.37%  ±4.49%  ±5.84%
 buffers/buffer-fill.js n=20000 size=65536 type='fill(400)'                               -2.57 %       ±2.97%  ±3.95%  ±5.14%
 buffers/buffer-fill.js n=20000 size=65536 type='fill(Buffer.alloc(1), 0)'                 1.28 %       ±2.70%  ±3.60%  ±4.69%
 buffers/buffer-fill.js n=20000 size=8192 type='fill("")'                                 -2.32 %       ±7.66% ±10.20% ±13.28%
 buffers/buffer-fill.js n=20000 size=8192 type='fill("t", "utf8")'                  *     -9.98 %       ±7.50%  ±9.99% ±13.00%
 buffers/buffer-fill.js n=20000 size=8192 type='fill("t", 0, "utf8")'                     -5.57 %       ±8.59% ±11.43% ±14.88%
 buffers/buffer-fill.js n=20000 size=8192 type='fill("t", 0)'                             -7.57 %       ±9.28% ±12.35% ±16.09%
 buffers/buffer-fill.js n=20000 size=8192 type='fill("t")'                                -0.69 %       ±7.65% ±10.18% ±13.27%
 buffers/buffer-fill.js n=20000 size=8192 type='fill("test")'                             -2.94 %       ±7.46%  ±9.93% ±12.93%
 buffers/buffer-fill.js n=20000 size=8192 type='fill(0)'                                  -6.50 %      ±11.14% ±14.83% ±19.30%
 buffers/buffer-fill.js n=20000 size=8192 type='fill(100)'                               -11.89 %      ±12.25% ±16.31% ±21.27%
 buffers/buffer-fill.js n=20000 size=8192 type='fill(400)'                          *    -12.45 %      ±10.89% ±14.48% ±18.85%
 buffers/buffer-fill.js n=20000 size=8192 type='fill(Buffer.alloc(1), 0)'                 -4.06 %       ±6.39%  ±8.50% ±11.07%

This also makes it possible to use Symbol methods and getters.

New results:

                                                                           confidence improvement accuracy (*)   (**)   (***)
 buffers/buffer-fill.js n=20000 size=65536 type='fill("")'                                -0.39 %       ±1.58% ±2.09%  ±2.68%
 buffers/buffer-fill.js n=20000 size=65536 type='fill("t", "utf8")'                       -0.96 %       ±1.47% ±1.94%  ±2.49%
 buffers/buffer-fill.js n=20000 size=65536 type='fill("t", 0, "utf8")'                     0.20 %       ±1.58% ±2.09%  ±2.68%
 buffers/buffer-fill.js n=20000 size=65536 type='fill("t", 0)'                     **     -2.25 %       ±1.65% ±2.17%  ±2.79%
 buffers/buffer-fill.js n=20000 size=65536 type='fill("t")'                               -0.68 %       ±1.51% ±1.99%  ±2.55%
 buffers/buffer-fill.js n=20000 size=65536 type='fill("test")'                             1.23 %       ±1.47% ±1.94%  ±2.49%
 buffers/buffer-fill.js n=20000 size=65536 type='fill(0)'                                 -0.47 %       ±1.64% ±2.16%  ±2.77%
 buffers/buffer-fill.js n=20000 size=65536 type='fill(100)'                         *     -1.72 %       ±1.59% ±2.10%  ±2.69%
 buffers/buffer-fill.js n=20000 size=65536 type='fill(400)'                                0.64 %       ±1.70% ±2.24%  ±2.88%
 buffers/buffer-fill.js n=20000 size=65536 type='fill(Buffer.alloc(1), 0)'                 0.36 %       ±1.35% ±1.78%  ±2.28%
 buffers/buffer-fill.js n=20000 size=8192 type='fill("")'                                 -1.47 %       ±3.97% ±5.23%  ±6.72%
 buffers/buffer-fill.js n=20000 size=8192 type='fill("t", "utf8")'                 **     -6.17 %       ±3.90% ±5.14%  ±6.60%
 buffers/buffer-fill.js n=20000 size=8192 type='fill("t", 0, "utf8")'               *     -4.80 %       ±4.18% ±5.51%  ±7.07%
 buffers/buffer-fill.js n=20000 size=8192 type='fill("t", 0)'                     ***     -7.77 %       ±4.14% ±5.46%  ±7.01%
 buffers/buffer-fill.js n=20000 size=8192 type='fill("t")'                                -3.28 %       ±3.99% ±5.26%  ±6.75%
 buffers/buffer-fill.js n=20000 size=8192 type='fill("test")'                              0.64 %       ±3.46% ±4.56%  ±5.86%
 buffers/buffer-fill.js n=20000 size=8192 type='fill(0)'                            *     -6.35 %       ±6.19% ±8.17% ±10.49%
 buffers/buffer-fill.js n=20000 size=8192 type='fill(100)'                         **     -8.87 %       ±5.81% ±7.66%  ±9.84%
 buffers/buffer-fill.js n=20000 size=8192 type='fill(400)'                        ***    -13.07 %       ±5.92% ±7.81% ±10.03%
 buffers/buffer-fill.js n=20000 size=8192 type='fill(Buffer.alloc(1), 0)'                  1.87 %       ±3.41% ±4.50%  ±5.78%

/cc @nodejs/v8

If I understood https://v8.dev/blog/v8-release-80#optimizing-higher-order-builtins correctly, this change shouldn't affect performance anymore. But we still see a significant difference between these two cases:

// 1
const TypedArrayPrototype = Object.getPrototypeOf(Uint8Array.prototype);
const TypedArrayFill = TypedArrayPrototype.fill;
TypedArrayFill.call(buffer, value);

// 2
const ReflectApply = Reflect.apply;
function uncurryThis(func) {
  return (thisArg, ...args) => ReflectApply(func, thisArg, args);
}
const TypedArrayPrototypeFill = uncurryThis(TypedArrayFill);
TypedArrayPrototypeFill(buffer, value);

What version of V8 are the newest results based on?

The one we have on master, 8.3.

We should probably also save the setter if it exists.

Suggested change

      
                const desc = Reflect.getOwnPropertyDescriptor(src, key);
          
                if (typeof desc.value === 'function') {
          
                  desc.value = uncurryThis(desc.value);
          
                } else if (typeof desc.get === 'function') {
          
                  desc.value = uncurryThis(desc.get);
          
                  delete desc.get;
          
                  delete desc.set;
          
                }
          
                Reflect.defineProperty(dest, newKey, desc);
          
                const desc = Reflect.getOwnPropertyDescriptor(src, key);
          
                let setterDesc;
          
                if (typeof desc.value === 'function') {
          
                  desc.value = uncurryThis(desc.value);
          
                } else if (typeof desc.get === 'function') {
          
                  desc.value = uncurryThis(desc.get);
          
                  if (typeof desc.set === 'function') {
          
                    setterDesc = { value: uncurryThis(desc.set), enumerable: desc.enumerable };
          
                  }
          
                  delete desc.get;
          
                  delete desc.set;
          
                }
          
                Reflect.defineProperty(dest, newKey, desc);
          
                if (setterDesc) {
          
                  Reflect.defineProperty(dest, `${newKey}Setter`, desc);
          
                }

I’ve extracted this change into #36329.

Related to #36016.

This needs a rebase.

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

Closes: nodejs#32127

Closes: #32127 PR-URL: #36329 Fixes: #32127 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. util Issues and PRs related to the built-in util module. labels Mar 6, 2020

targos added blocked PRs that are blocked by other issues or PRs. dont-land-on-v10.x labels Mar 7, 2020

BridgeAR removed the dont-land-on-v13.x label May 1, 2020

lib: add TypedArray to the primordials

95b07ea

This also makes it possible to use Symbol methods and getters.

targos force-pushed the primordial-typed-array branch from 0a11233 to 95b07ea Compare May 16, 2020 13:01

ExE-Boss reviewed May 17, 2020

View reviewed changes

BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18

aduh95 added the stalled Issues and PRs that are stalled. label Nov 8, 2020

ExE-Boss mentioned this pull request Nov 8, 2020

lib: add %TypedArray% abstract constructor to primordials #36016

Closed

2 tasks

targos removed the stalled Issues and PRs that are stalled. label Nov 9, 2020

ExE-Boss mentioned this pull request Nov 30, 2020

lib: add uncurried accessor properties to primordials #36329

Closed

3 tasks

targos closed this Nov 30, 2020

ExE-Boss added a commit to ExE-Boss/node that referenced this pull request Dec 5, 2020

lib: add uncurried accessor properties to primordials

4ec94c4

Closes: nodejs#32127

jasnell pushed a commit that referenced this pull request Dec 7, 2020

lib: add uncurried accessor properties to primordials

c83e599

Closes: #32127 PR-URL: #36329 Fixes: #32127 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

danielleadams pushed a commit that referenced this pull request Dec 7, 2020

lib: add uncurried accessor properties to primordials

c2938bd

Closes: #32127 PR-URL: #36329 Fixes: #32127 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

targos deleted the primordial-typed-array branch April 12, 2021 15:03

targos pushed a commit that referenced this pull request May 16, 2021

lib: add uncurried accessor properties to primordials

bc92cbd

Closes: #32127 PR-URL: #36329 Fixes: #32127 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

targos pushed a commit that referenced this pull request Jun 11, 2021

lib: add uncurried accessor properties to primordials

5daeac6

Closes: #32127 PR-URL: #36329 Fixes: #32127 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

lib: add TypedArray to the primordials by targos · Pull Request #32127 · nodejs/node · GitHub

Uh oh!

Uh oh!

lib: add TypedArray to the primordials - #32127

lib: add TypedArray to the primordials#32127targos wants to merge 1 commit intonodejs:masternodejs/node:masterfrom targos:primordial-typed-arraytargos/node:primordial-typed-arrayCopy head branch name to clipboard

Conversation

targos commented Mar 6, 2020

Checklist

Uh oh!

mscdex commented Mar 7, 2020

Uh oh!

targos commented Mar 7, 2020

Uh oh!

mscdex commented Mar 7, 2020

Uh oh!

targos commented Mar 7, 2020

Uh oh!

targos commented Mar 7, 2020

Uh oh!

targos commented May 16, 2020

Uh oh!

targos commented May 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex commented May 16, 2020

Uh oh!

targos commented May 16, 2020

Uh oh!

ExE-Boss May 17, 2020

Choose a reason for hiding this comment

Uh oh!

ExE-Boss Nov 30, 2020

Choose a reason for hiding this comment

Uh oh!

aduh95 commented Nov 8, 2020

Uh oh!

aduh95 commented Nov 8, 2020

Uh oh!

github-actions Bot commented Nov 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lib: add TypedArray to the primordials#32127
targos wants to merge 1 commit into
nodejs:masterfrom
targos:primordial-typed-array

targos commented May 16, 2020 •
edited
Loading