buffer: consolidate encoding parsing #29217

mscdex · 2019-08-20T03:41:29Z

This consolidates encoding parsing for buffer at both the JS and C++ levels (only fill() still parses the encoding twice). Doing so not only cleans things up but also improves performance in some cases.

Various relevant benchmark results:

                                                                                                   confidence improvement accuracy (*)   (**)  (***)
 buffers/buffer-bytelength.js n=10000000 len=16 encoding='base64'                                          *      4.03 %       ±3.40% ±4.53% ±5.90%
 buffers/buffer-bytelength.js n=10000000 len=16 encoding='buffer'                                        ***     17.47 %       ±1.09% ±1.46% ±1.90%
 buffers/buffer-bytelength.js n=10000000 len=16 encoding='utf8'                                          ***     14.05 %       ±2.17% ±2.92% ±3.87%

 buffers/buffer-tostring.js n=10000000 len=64 args=0 encoding='ascii'                                    ***      1.05 %       ±0.58% ±0.78% ±1.03%
 buffers/buffer-tostring.js n=10000000 len=64 args=0 encoding='hex'                                      ***      2.09 %       ±0.76% ±1.02% ±1.34%
 buffers/buffer-tostring.js n=10000000 len=64 args=0 encoding='latin1'                                   ***      1.30 %       ±0.49% ±0.65% ±0.85%
 buffers/buffer-tostring.js n=10000000 len=64 args=0 encoding='utf8'                                     ***      2.31 %       ±0.98% ±1.31% ±1.72%
 buffers/buffer-tostring.js n=10000000 len=64 args=1 encoding='ascii'                                    ***      8.75 %       ±1.22% ±1.64% ±2.17%
 buffers/buffer-tostring.js n=10000000 len=64 args=1 encoding='hex'                                      ***      5.16 %       ±0.71% ±0.94% ±1.23%
 buffers/buffer-tostring.js n=10000000 len=64 args=1 encoding='latin1'                                   ***      7.52 %       ±1.41% ±1.88% ±2.46%
 buffers/buffer-tostring.js n=10000000 len=64 args=1 encoding='UCS-2'                                    ***      7.93 %       ±1.40% ±1.86% ±2.43%
 buffers/buffer-tostring.js n=10000000 len=64 args=1 encoding='utf8'                                     ***      9.29 %       ±1.19% ±1.60% ±2.11%
 buffers/buffer-tostring.js n=10000000 len=64 args=3 encoding='ascii'                                    ***      9.34 %       ±0.89% ±1.19% ±1.57%
 buffers/buffer-tostring.js n=10000000 len=64 args=3 encoding='hex'                                      ***      4.83 %       ±0.60% ±0.80% ±1.04%
 buffers/buffer-tostring.js n=10000000 len=64 args=3 encoding='latin1'                                   ***     10.68 %       ±1.50% ±2.01% ±2.66%
 buffers/buffer-tostring.js n=10000000 len=64 args=3 encoding='UCS-2'                                    ***      4.50 %       ±1.19% ±1.58% ±2.07%
 buffers/buffer-tostring.js n=10000000 len=64 args=3 encoding='utf8'                                     ***      9.86 %       ±0.84% ±1.12% ±1.48%

 buffers/buffer-write-string.js n=10000000 len=2048 args='' encoding=''                                   **      1.87 %       ±1.20% ±1.60% ±2.09%
 buffers/buffer-write-string.js n=10000000 len=2048 args='' encoding='ascii'                             ***      8.84 %       ±2.47% ±3.30% ±4.31%
 buffers/buffer-write-string.js n=10000000 len=2048 args='' encoding='hex'                               ***      1.24 %       ±0.56% ±0.75% ±0.98%
 buffers/buffer-write-string.js n=10000000 len=2048 args='' encoding='latin1'                            ***      8.56 %       ±1.02% ±1.36% ±1.77%
 buffers/buffer-write-string.js n=10000000 len=2048 args='' encoding='utf16le'                           ***      5.39 %       ±0.68% ±0.91% ±1.18%
 buffers/buffer-write-string.js n=10000000 len=2048 args='' encoding='utf8'                              ***      2.59 %       ±1.09% ±1.45% ±1.89%

 buffers/buffer-from.js n=4000000 len=100 source='string'                                                ***      3.36 %       ±1.69% ±2.24% ±2.92%

 buffers/buffer-indexof.js n=100000 type='buffer' encoding='ucs2' search='@'                             ***     13.54 %       ±4.29% ±5.71% ±7.43%
 buffers/buffer-indexof.js n=100000 type='buffer' encoding='ucs2' search='</i> to the Caterpillar'         *      0.57 %       ±0.48% ±0.63% ±0.82%
 buffers/buffer-indexof.js n=100000 type='buffer' encoding='ucs2' search='aaaaaaaaaaaaaaaaa'               *      0.44 %       ±0.43% ±0.58% ±0.75%
 buffers/buffer-indexof.js n=100000 type='buffer' encoding='ucs2' search='Alice'                         ***     18.63 %       ±4.71% ±6.27% ±8.16%
 buffers/buffer-indexof.js n=100000 type='buffer' encoding='ucs2' search='Gryphon'                       ***      3.35 %       ±1.91% ±2.55% ±3.31%
 buffers/buffer-indexof.js n=100000 type='buffer' encoding='ucs2' search='neighbouring pool'               *      0.48 %       ±0.46% ±0.61% ±0.80%
 buffers/buffer-indexof.js n=100000 type='buffer' encoding='ucs2' search='SQ'                            ***      3.65 %       ±1.69% ±2.25% ±2.92%
 buffers/buffer-indexof.js n=100000 type='buffer' encoding='utf8' search='@'                             ***     16.95 %       ±3.99% ±5.31% ±6.92%
 buffers/buffer-indexof.js n=100000 type='buffer' encoding='utf8' search='Alice'                         ***     18.55 %       ±5.48% ±7.30% ±9.52%
 buffers/buffer-indexof.js n=100000 type='buffer' encoding='utf8' search='Gryphon'                        **      3.03 %       ±1.78% ±2.36% ±3.08%
 buffers/buffer-indexof.js n=100000 type='buffer' encoding='utf8' search='Ou est ma chatte?'               *      0.78 %       ±0.66% ±0.88% ±1.14%
 buffers/buffer-indexof.js n=100000 type='string' encoding='ucs2' search='@'                             ***     14.38 %       ±5.38% ±7.16% ±9.32%
 buffers/buffer-indexof.js n=100000 type='string' encoding='ucs2' search='Alice'                         ***     27.09 %       ±5.71% ±7.60% ±9.91%
 buffers/buffer-indexof.js n=100000 type='string' encoding='ucs2' search='SQ'                            ***      3.63 %       ±2.07% ±2.75% ±3.58%
 buffers/buffer-indexof.js n=100000 type='string' encoding='utf8' search='@'                             ***     12.08 %       ±4.53% ±6.03% ±7.85%
 buffers/buffer-indexof.js n=100000 type='string' encoding='utf8' search='Alice'                         ***     15.36 %       ±5.71% ±7.60% ±9.90%
 buffers/buffer-indexof.js n=100000 type='string' encoding='utf8' search='Gryphon'                         *      1.88 %       ±1.87% ±2.48% ±3.23%
 buffers/buffer-indexof.js n=100000 type='string' encoding='utf8' search='Ou est ma chatte?'               *      0.88 %       ±0.78% ±1.04% ±1.36%
 buffers/buffer-indexof.js n=100000 type='string' encoding='utf8' search='SQ'                              *      3.60 %       ±3.04% ±4.04% ±5.27%

Checklist

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

nodejs-github-bot · 2019-08-20T03:43:21Z

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

Trott · 2019-08-22T04:41:00Z

@nodejs/buffer This could use some reviews.

nodejs-github-bot · 2019-08-22T04:41:43Z

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

bnoordhuis

Nice work, some impressive speedups!

bnoordhuis · 2019-08-22T08:31:41Z

lib/buffer.js

-    throw new ERR_INVALID_ARG_TYPE(
-      'string', ['string', 'Buffer', 'ArrayBuffer'], string
-    );
+const encodingOps = {


Maybe add a comment that for performance reasons it's imperative that all the entries have the same shape / hidden map? (I.e., users of an ops object shouldn't go poly- or megamorphic.)

nodejs-github-bot · 2019-08-23T22:19:46Z

CI: https://ci.nodejs.org/job/node-test-pull-request/25208/ ✔️

src/node_buffer.cc

nodejs-github-bot · 2019-08-26T05:25:17Z

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

PR-URL: nodejs#29217 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

PR-URL: #29217 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

The assignment to the encoding variable has no effect. Refs: nodejs#29217

The assignment to the encoding variable has no effect. Refs: #29217 PR-URL: #50199 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

The assignment to the encoding variable has no effect. Refs: nodejs#29217 PR-URL: nodejs#50199 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

The assignment to the encoding variable has no effect. Refs: #29217 PR-URL: #50199 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

mscdex added buffer Issues and PRs related to the buffer subsystem. performance Issues and PRs related to the performance of Node.js. labels Aug 20, 2019

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Aug 20, 2019

mscdex changed the title ~~Buffer consolidate encoding parsing~~ buffer: consolidate encoding parsing Aug 20, 2019

bnoordhuis mentioned this pull request Aug 22, 2019

aix: OOM during linking #29262

Closed

bnoordhuis approved these changes Aug 22, 2019

View reviewed changes

Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 23, 2019

This comment has been minimized.

Sign in to view

addaleax approved these changes Aug 25, 2019

View reviewed changes

src/node_buffer.cc Outdated Show resolved Hide resolved

src/node_buffer.cc Outdated Show resolved Hide resolved

src/node_buffer.cc Outdated Show resolved Hide resolved

mscdex force-pushed the buffer-consolidate-encoding-parsing branch from cb09531 to 9c7149a Compare August 26, 2019 05:21

mscdex added 2 commits August 28, 2019 15:13

buffer: consolidate encoding parsing

bb1af6c

PR-URL: nodejs#29217 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

buffer: correct indexOf() error message

5dc5cfb

PR-URL: nodejs#29217 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

mscdex force-pushed the buffer-consolidate-encoding-parsing branch from 9c7149a to 5dc5cfb Compare August 28, 2019 19:14

mscdex merged commit 5dc5cfb into nodejs:master Aug 28, 2019

mscdex deleted the buffer-consolidate-encoding-parsing branch August 28, 2019 19:21

BridgeAR pushed a commit that referenced this pull request Sep 3, 2019

buffer: consolidate encoding parsing

a007fe9

PR-URL: #29217 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

BridgeAR pushed a commit that referenced this pull request Sep 3, 2019

buffer: correct indexOf() error message

8da9e4d

PR-URL: #29217 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

BridgeAR mentioned this pull request Sep 3, 2019

v12.10.0 proposal #29429

Merged

BridgeAR pushed a commit that referenced this pull request Sep 4, 2019

buffer: consolidate encoding parsing

c900762

PR-URL: #29217 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

BridgeAR pushed a commit that referenced this pull request Sep 4, 2019

buffer: correct indexOf() error message

71aaf59

PR-URL: #29217 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

tniessen added a commit to tniessen/node that referenced this pull request Oct 16, 2023

buffer: remove unnecessary assignment in fromString

8bdd921

The assignment to the encoding variable has no effect. Refs: nodejs#29217

tniessen mentioned this pull request Oct 16, 2023

buffer: remove unnecessary assignment in fromString #50199

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

buffer: consolidate encoding parsing #29217

buffer: consolidate encoding parsing #29217

mscdex commented Aug 20, 2019 •

edited

Loading

nodejs-github-bot commented Aug 20, 2019

Trott commented Aug 22, 2019

nodejs-github-bot commented Aug 22, 2019

bnoordhuis left a comment

bnoordhuis Aug 22, 2019

This comment has been minimized.

nodejs-github-bot commented Aug 23, 2019 •

edited by Trott

Loading

nodejs-github-bot commented Aug 26, 2019

buffer: consolidate encoding parsing #29217

buffer: consolidate encoding parsing #29217

Conversation

mscdex commented Aug 20, 2019 • edited Loading

Checklist

nodejs-github-bot commented Aug 20, 2019

Trott commented Aug 22, 2019

nodejs-github-bot commented Aug 22, 2019

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Aug 22, 2019

Choose a reason for hiding this comment

This comment has been minimized.

nodejs-github-bot commented Aug 23, 2019 • edited by Trott Loading

nodejs-github-bot commented Aug 26, 2019

mscdex commented Aug 20, 2019 •

edited

Loading

nodejs-github-bot commented Aug 23, 2019 •

edited by Trott

Loading