encoding: use AliasedUint32Array for encodeInto results by joyeecheung · Pull Request #46658 · nodejs/node · GitHub

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%

Review requested:

@nodejs/gyp
@nodejs/http
@nodejs/http2
@nodejs/net
@nodejs/startup

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.

Rebased to fix merge conflict

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

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

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?

Where does the copy take place?

v8::ArrayBufferView::Buffer().

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

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 <raisinten@gmail.com>

Move the bindings used by TextEncoder to a new binding for more self-contained code. PR-URL: #46658 Reviewed-By: Darshan Sen <raisinten@gmail.com>

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 <raisinten@gmail.com>

Landed in 3b0c047...e5933c8

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

Linter issue is addressed in #47003.

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 <raisinten@gmail.com>

Move the bindings used by TextEncoder to a new binding for more self-contained code. PR-URL: #46658 Reviewed-By: Darshan Sen <raisinten@gmail.com>

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 <raisinten@gmail.com>

- 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 <raisinten@gmail.com>

Move the bindings used by TextEncoder to a new binding for more self-contained code. PR-URL: #46658 Reviewed-By: Darshan Sen <raisinten@gmail.com>

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 <raisinten@gmail.com>

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

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 <raisinten@gmail.com>

- 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 <raisinten@gmail.com>

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

Comment thread src/encoding.h Outdated

Comment thread src/node_binding.cc Outdated

Comment thread lib/internal/encoding.js Outdated

Comment thread src/env-inl.h Outdated

BridgeAR reviewed Feb 14, 2023

View reviewed changes

Comment thread benchmark/util/text-encoder.js Outdated

joyeecheung force-pushed the encode branch from 535e1bf to 86b9a0b Compare 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 06584cc Compare 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 9b54d80 Compare 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 4ea7f2e Compare 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 <raisinten@gmail.com>

joyeecheung closed this Mar 7, 2023

github-actions Bot 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 <raisinten@gmail.com>

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 <raisinten@gmail.com>

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

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

encoding: use AliasedUint32Array for encodeInto results by joyeecheung · Pull Request #46658 · nodejs/node · GitHub

Uh oh!

Uh oh!

encoding: use AliasedUint32Array for encodeInto results - #46658

encoding: use AliasedUint32Array for encodeInto results#46658joyeecheung wants to merge 5 commits intonodejs:mainnodejs/node:mainfrom joyeecheung:encodejoyeecheung/node:encodeCopy head branch name to clipboard

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!

debadree25 May 5, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

encoding: use AliasedUint32Array for encodeInto results#46658
joyeecheung wants to merge 5 commits into
nodejs:mainfrom
joyeecheung:encode

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