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 <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 · 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 <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>

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);
+}
+
+void BindingData::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 <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

src/encoding.h Outdated Show resolved Hide resolved

src/node_binding.cc Outdated Show resolved Hide resolved

lib/internal/encoding.js Show resolved Hide resolved

src/env-inl.h Outdated Show resolved Hide resolved

BridgeAR reviewed Feb 14, 2023

View reviewed changes

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

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 PRs awaiting manual backport to the v18.x-staging branch. 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

encoding: use AliasedUint32Array for encodeInto results #46658

encoding: use AliasedUint32Array for encodeInto results #46658

joyeecheung commented Feb 14, 2023

nodejs-github-bot commented Feb 14, 2023

nodejs-github-bot commented Feb 20, 2023

joyeecheung commented Feb 28, 2023

nodejs-github-bot commented Feb 28, 2023

joyeecheung commented Mar 3, 2023 •

edited

RaisinTen left a comment •

edited

joyeecheung commented Mar 7, 2023

joyeecheung commented Mar 7, 2023

nodejs-github-bot commented Mar 7, 2023

joyeecheung commented Mar 7, 2023

MoLow commented Mar 8, 2023

cjihrig commented Mar 8, 2023

joyeecheung commented Mar 8, 2023 •

edited

danielleadams commented Apr 3, 2023 •

edited

debadree25 May 5, 2023

encoding: use AliasedUint32Array for encodeInto results #46658

encoding: use AliasedUint32Array for encodeInto results #46658

Conversation

joyeecheung commented Feb 14, 2023

benchmark: stablize encode benchmark

src: move encoding bindings to a new binding

encoding: use AliasedUint32Array for encodeInto results

nodejs-github-bot commented Feb 14, 2023

nodejs-github-bot commented Feb 20, 2023

joyeecheung commented Feb 28, 2023

nodejs-github-bot commented Feb 28, 2023

joyeecheung commented Mar 3, 2023 • edited

RaisinTen left a comment • edited

Choose a reason for hiding this comment

joyeecheung commented Mar 7, 2023

joyeecheung commented Mar 7, 2023

nodejs-github-bot commented Mar 7, 2023

joyeecheung commented Mar 7, 2023

MoLow commented Mar 8, 2023

cjihrig commented Mar 8, 2023

joyeecheung commented Mar 8, 2023 • edited

danielleadams commented Apr 3, 2023 • edited

debadree25 May 5, 2023

Choose a reason for hiding this comment

joyeecheung commented Mar 3, 2023 •

edited

RaisinTen left a comment •

edited

joyeecheung commented Mar 8, 2023 •

edited

danielleadams commented Apr 3, 2023 •

edited