Skip to content

lib: add TypedArray to the primordials#32127

Closed
targos wants to merge 1 commit into
nodejs:masterfrom
targos:primordial-typed-array
Closed

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

Conversation

@targos

@targos targos commented Mar 6, 2020

Copy link
Copy Markdown
Member

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

@nodejs-github-bot 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
@mscdex

mscdex commented Mar 7, 2020

Copy link
Copy Markdown
Contributor

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%

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

targos commented Mar 7, 2020

Copy link
Copy Markdown
Member Author

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

@mscdex

mscdex commented Mar 7, 2020

Copy link
Copy Markdown
Contributor

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?

@targos

targos commented Mar 7, 2020

Copy link
Copy Markdown
Member Author

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

@targos

targos commented Mar 7, 2020

Copy link
Copy Markdown
Member Author

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.
@targos targos force-pushed the primordial-typed-array branch from 0a11233 to 95b07ea Compare May 16, 2020 13:01
@targos

targos commented May 16, 2020

Copy link
Copy Markdown
Member Author

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%

@targos

targos commented May 16, 2020

Copy link
Copy Markdown
Member Author

/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);

@mscdex

mscdex commented May 16, 2020

Copy link
Copy Markdown
Contributor

What version of V8 are the newest results based on?

@targos

targos commented May 16, 2020

Copy link
Copy Markdown
Member Author

The one we have on master, 8.3.

Comment on lines +75 to +83
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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I’ve extracted this change into #36329.

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

aduh95 commented Nov 8, 2020

Copy link
Copy Markdown
Contributor

Related to #36016.

@aduh95

aduh95 commented Nov 8, 2020

Copy link
Copy Markdown
Contributor

This needs a rebase.

@aduh95 aduh95 added the stalled Issues and PRs that are stalled. label Nov 8, 2020
@github-actions

github-actions Bot commented Nov 8, 2020

Copy link
Copy Markdown
Contributor

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.

@targos targos removed the stalled Issues and PRs that are stalled. label Nov 9, 2020
@targos targos closed this Nov 30, 2020
ExE-Boss added a commit to ExE-Boss/node that referenced this pull request Dec 5, 2020
jasnell pushed a commit that referenced this pull request Dec 7, 2020
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
Closes: #32127

PR-URL: #36329
Fixes: #32127
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
cjihrig pushed a commit to cjihrig/node that referenced this pull request Dec 8, 2020
Closes: nodejs#32127

PR-URL: nodejs#36329
Fixes: nodejs#32127
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos deleted the primordial-typed-array branch April 12, 2021 15:03
targos pushed a commit that referenced this pull request May 16, 2021
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
Closes: #32127

PR-URL: #36329
Fixes: #32127
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blocked PRs that are blocked by other issues or PRs. buffer Issues and PRs related to the buffer subsystem. util Issues and PRs related to the built-in util module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants