encoding: use AliasedUint32Array for encodeInto results#46658

joyeecheung · 2023-02-14T21:03:17Z

The first commits comes from #46620

benchmark: stablize encode benchmark

Increase the number of iteration to 1e6 to reduce flakes. 1e4
can introduce flakes even when comparing the main branch
against itself
Replace the 1024 * 32 length test with 1024 * 8 since it would
otherwise take too long to complete.
Check the results of the encoding methods at the end.

src: move encoding bindings to a new binding

Move the bindings used by TextEncoder to a new binding for
more self-contained code.

encoding: use AliasedUint32Array for encodeInto results

Getting the buffer from a TypedArray created from the JS land
incurs a copy. For encodeInto() results we can just use an
AliasedArray and let the binding always own the store.

 confidence improvement accuracy (*) (**) (***) util/text-encoder.js op='encodeInto' type='ascii' n=1000000 len=1024 0.41 % ±1.33% ±1.78% ±2.33% util/text-encoder.js op='encodeInto' type='ascii' n=1000000 len=16 *** 3.66 % ±1.35% ±1.79% ±2.33% util/text-encoder.js op='encodeInto' type='ascii' n=1000000 len=256 0.29 % ±0.88% ±1.17% ±1.53% util/text-encoder.js op='encodeInto' type='ascii' n=1000000 len=32 *** 4.01 % ±1.34% ±1.79% ±2.34% util/text-encoder.js op='encodeInto' type='ascii' n=1000000 len=8192 -0.32 % ±0.66% ±0.88% ±1.15% util/text-encoder.js op='encodeInto' type='one-byte-string' n=1000000 len=1024 * -0.48 % ±0.37% ±0.50% ±0.65% util/text-encoder.js op='encodeInto' type='one-byte-string' n=1000000 len=16 *** 5.39 % ±2.49% ±3.34% ±4.41% util/text-encoder.js op='encodeInto' type='one-byte-string' n=1000000 len=256 *** 1.44 % ±0.48% ±0.63% ±0.83% util/text-encoder.js op='encodeInto' type='one-byte-string' n=1000000 len=32 *** 4.67 % ±1.01% ±1.34% ±1.74% util/text-encoder.js op='encodeInto' type='one-byte-string' n=1000000 len=8192 0.15 % ±0.24% ±0.32% ±0.42% util/text-encoder.js op='encodeInto' type='two-byte-string' n=1000000 len=1024 *** 1.29 % ±0.49% ±0.66% ±0.86% util/text-encoder.js op='encodeInto' type='two-byte-string' n=1000000 len=16 2.23 % ±2.90% ±3.90% ±5.16% util/text-encoder.js op='encodeInto' type='two-byte-string' n=1000000 len=256 *** 1.10 % ±0.48% ±0.64% ±0.84% util/text-encoder.js op='encodeInto' type='two-byte-string' n=1000000 len=32 *** 6.31 % ±1.60% ±2.14% ±2.82% util/text-encoder.js op='encodeInto' type='two-byte-string' n=1000000 len=8192 0.19 % ±0.21% ±0.28% ±0.37%

nodejs-github-bot · 2023-02-14T21:03:27Z

src/encoding.h

src/node_binding.cc

lib/internal/encoding.js

src/env-inl.h

benchmark/util/text-encoder.js

nodejs-github-bot · 2023-02-20T15:27:34Z

CI: https://ci.nodejs.org/job/node-test-pull-request/49791/

- Increase the number of iteration to 1e6 to reduce flakes. 1e4 can introduce flakes even when comparing the main branch against itself - Replace the 1024 * 32 length test with 1024 * 8 since it would otherwise take too long to complete. Remove the 16 length test since it's not too different from 32. - Check the results of the encoding methods at the end.

Move the bindings used by TextEncoder to a new binding for more self-contained code.

Getting the buffer from a TypedArray created from the JS land incurs a copy. For encodeInto() results we can just use an AliasedArray and let the binding always own the store.

joyeecheung · 2023-02-28T14:41:35Z

Rebased to fix merge conflict

nodejs-github-bot · 2023-02-28T14:41:52Z

CI: https://ci.nodejs.org/job/node-test-pull-request/50104/

joyeecheung · 2023-03-03T16:25:21Z

cc @nodejs/buffer @nodejs/cpp-reviewers can I have some reviews please? Thanks.

RaisinTen

The code change LGTM (% lint fixes) but I'm curious about this part:

Getting the buffer from a TypedArray created from the JS land
incurs a copy.

Where does the copy take place?

joyeecheung · 2023-03-07T12:05:43Z

Where does the copy take place?

v8::ArrayBufferView::Buffer().

joyeecheung · 2023-03-07T16:58:14Z

Fixed linter error (looks like the bot is not working again: https://ci.nodejs.org/job/node-test-pull-request/50249/)

nodejs-github-bot · 2023-03-07T19:55:18Z

CI: https://ci.nodejs.org/job/node-test-pull-request/50258/

- Increase the number of iteration to 1e6 to reduce flakes. 1e4 can introduce flakes even when comparing the main branch against itself - Replace the 1024 * 32 length test with 1024 * 8 since it would otherwise take too long to complete. Remove the 16 length test since it's not too different from 32. - Check the results of the encoding methods at the end. PR-URL: #46658 Reviewed-By: Darshan Sen <[email protected]>

Move the bindings used by TextEncoder to a new binding for more self-contained code. PR-URL: #46658 Reviewed-By: Darshan Sen <[email protected]>

Getting the buffer from a TypedArray created from the JS land incurs a copy. For encodeInto() results we can just use an AliasedArray and let the binding always own the store. PR-URL: #46658 Reviewed-By: Darshan Sen <[email protected]>

joyeecheung · 2023-03-07T21:55:07Z

Landed in 3b0c047...e5933c8

MoLow · 2023-03-08T00:45:23Z

this commit has landed without a passing CI/GH actions, introducing a lint error: #47007

cjihrig · 2023-03-08T00:51:56Z

Linter issue is addressed in #47003.

joyeecheung · 2023-03-08T01:46:59Z

The CI was actually passing in https://ci.nodejs.org/job/node-test-pull-request/50258/, which included the fixup commit that should remove the unused using statement causing the lint error: 4ea7f2e. But somehow git was only able to apply that fixup commit partially during landing, and I didn't see any warnings?

- Increase the number of iteration to 1e6 to reduce flakes. 1e4 can introduce flakes even when comparing the main branch against itself - Replace the 1024 * 32 length test with 1024 * 8 since it would otherwise take too long to complete. Remove the 16 length test since it's not too different from 32. - Check the results of the encoding methods at the end. PR-URL: #46658 Reviewed-By: Darshan Sen <[email protected]>

Move the bindings used by TextEncoder to a new binding for more self-contained code. PR-URL: #46658 Reviewed-By: Darshan Sen <[email protected]>

Getting the buffer from a TypedArray created from the JS land incurs a copy. For encodeInto() results we can just use an AliasedArray and let the binding always own the store. PR-URL: #46658 Reviewed-By: Darshan Sen <[email protected]>

- Increase the number of iteration to 1e6 to reduce flakes. 1e4 can introduce flakes even when comparing the main branch against itself - Replace the 1024 * 32 length test with 1024 * 8 since it would otherwise take too long to complete. Remove the 16 length test since it's not too different from 32. - Check the results of the encoding methods at the end. PR-URL: #46658 Reviewed-By: Darshan Sen <[email protected]>

Move the bindings used by TextEncoder to a new binding for more self-contained code. PR-URL: #46658 Reviewed-By: Darshan Sen <[email protected]>

Getting the buffer from a TypedArray created from the JS land incurs a copy. For encodeInto() results we can just use an AliasedArray and let the binding always own the store. PR-URL: #46658 Reviewed-By: Darshan Sen <[email protected]>

danielleadams · 2023-04-03T04:24:12Z

@joyeecheung This breaks the build when landing in v18.x - do you mind opening a backport PR?

debadree25 · 2023-05-05T09:47:45Z

src/encoding_binding.cc

+CHECK_NOT_NULL(binding);
+}
+
+voidBindingData::EncodeInto(const FunctionCallbackInfo<Value>& args){


Out of curiosity I exploring places where fast API could be applied, came across this function and PR, I experimented a bit it seems v8::FastApiTypedArray<uint8_t>& is indeed supported and it does get caught in fast call, but I am not sure how one could extract ->Buffer() from v8::FastApiTypedArray<uint8_t>&, is that even possible? just wondering do let me know your thoughts cc @anonrig @joyeecheung
Thank You!

- Increase the number of iteration to 1e6 to reduce flakes. 1e4 can introduce flakes even when comparing the main branch against itself - Replace the 1024 * 32 length test with 1024 * 8 since it would otherwise take too long to complete. Remove the 16 length test since it's not too different from 32. - Check the results of the encoding methods at the end. PR-URL: #46658 Reviewed-By: Darshan Sen <[email protected]>

- Increase the number of iteration to 1e6 to reduce flakes. 1e4 can introduce flakes even when comparing the main branch against itself - Replace the 1024 * 32 length test with 1024 * 8 since it would otherwise take too long to complete. Remove the 16 length test since it's not too different from 32. - Check the results of the encoding methods at the end. PR-URL: nodejs/node#46658 Reviewed-By: Darshan Sen <[email protected]>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. 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 Feb 14, 2023

anonrig added the performance Issues and PRs related to the performance of Node.js. label Feb 14, 2023

anonrig reviewed Feb 14, 2023
View reviewed changes

src/encoding.hShow resolvedHide resolved
src/node_binding.cc Outdated Show resolvedHide resolved
lib/internal/encoding.jsShow resolvedHide resolved
src/env-inl.h Outdated Show resolvedHide resolved

BridgeAR reviewed Feb 14, 2023
View reviewed changes

benchmark/util/text-encoder.js Outdated Show resolvedHide resolved

joyeecheung force-pushed the encode branch from 535e1bf to 86b9a0bCompare February 15, 2023 01:40

This was referenced Feb 21, 2023
CI Reliability 2023-02-21 nodejs/reliability#502
Open
CI Reliability 2023-02-22 nodejs/reliability#503
Open

joyeecheung force-pushed the encode branch from 86b9a0b to 06584ccCompare February 28, 2023 14:38

joyeecheung added 2 commits February 28, 2023 15:41

src: move encoding bindings to a new binding
57bb217
Move the bindings used by TextEncoder to a new binding for more self-contained code.

encoding: use AliasedUint32Array for encodeInto results
9b54d80
Getting the buffer from a TypedArray created from the JS land incurs a copy. For encodeInto() results we can just use an AliasedArray and let the binding always own the store.

joyeecheung force-pushed the encode branch from 06584cc to 9b54d80Compare February 28, 2023 14:41

This was referenced Mar 1, 2023
CI Reliability 2023-03-01 nodejs/reliability#510
Open
CI Reliability 2023-03-02 nodejs/reliability#511
Open
CI Reliability 2023-03-03 nodejs/reliability#512
Open

This was referenced Mar 4, 2023
CI Reliability 2023-03-04 nodejs/reliability#513
Open
CI Reliability 2023-03-05 nodejs/reliability#514
Open

RaisinTen approved these changes Mar 6, 2023
View reviewed changes

fixup! encoding: use AliasedUint32Array for encodeInto results
1233e7f

fixup! src: move encoding bindings to a new binding
4ea7f2e

joyeecheung force-pushed the encode branch from ea70ca8 to 4ea7f2eCompare March 7, 2023 16:59

joyeecheung added a commit that referenced this pull request Mar 7, 2023
src: move encoding bindings to a new binding
25c2186
Move the bindings used by TextEncoder to a new binding for more self-contained code. PR-URL: #46658 Reviewed-By: Darshan Sen <[email protected]>

joyeecheung closed this Mar 7, 2023

github-actionsbot mentioned this pull request Mar 8, 2023
CI Reliability 2023-03-08 nodejs/reliability#517
Open
19 tasks

targos pushed a commit that referenced this pull request Mar 13, 2023
src: move encoding bindings to a new binding
db4fa93
Move the bindings used by TextEncoder to a new binding for more self-contained code. PR-URL: #46658 Reviewed-By: Darshan Sen <[email protected]>

targos pushed a commit that referenced this pull request Mar 14, 2023
src: move encoding bindings to a new binding
dcba3a0
Move the bindings used by TextEncoder to a new binding for more self-contained code. PR-URL: #46658 Reviewed-By: Darshan Sen <[email protected]>

targos mentioned this pull request Mar 14, 2023
v19.8.0 release proposal #47087
Merged

danielleadams added the backport-requested-v18.x label Apr 3, 2023

debadree25 reviewed May 5, 2023
View reviewed changes

targos mentioned this pull request Nov 28, 2023
v18.19.0 release proposal #50953
Merged

Uh oh!

encoding: use AliasedUint32Array for encodeInto results#46658

encoding: use AliasedUint32Array for encodeInto results #46658

Uh oh!

Conversation

joyeecheung commented Feb 14, 2023

benchmark: stablize encode benchmark

src: move encoding bindings to a new binding

encoding: use AliasedUint32Array for encodeInto results

Uh oh!

nodejs-github-bot commented Feb 14, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 20, 2023

Uh oh!

joyeecheung commented Feb 28, 2023

Uh oh!

nodejs-github-bot commented Feb 28, 2023

Uh oh!

joyeecheung commented Mar 3, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RaisinTen left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Mar 7, 2023

Uh oh!

joyeecheung commented Mar 7, 2023

Uh oh!

nodejs-github-bot commented Mar 7, 2023

Uh oh!

joyeecheung commented Mar 7, 2023

Uh oh!

MoLow commented Mar 8, 2023

Uh oh!

cjihrig commented Mar 8, 2023

Uh oh!

joyeecheung commented Mar 8, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danielleadams commented Apr 3, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

debadree25May 5, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

joyeecheung commented Mar 3, 2023•
edited
Loading

RaisinTen left a comment •
edited
Loading

joyeecheung commented Mar 8, 2023•
edited
Loading

danielleadams commented Apr 3, 2023•
edited
Loading