buffer: use fast API for writing one-byte strings by ronag · Pull Request #54311 · nodejs/node

ronag · 2024-08-10T19:59:03Z

                                                                        confidence improvement accuracy (*)    (**)   (***)
buffers/buffer-write-string-short.js n=1000000 len=0 encoding='ascii'          ***     65.81 %       ±5.23%  ±7.49% ±10.96%
buffers/buffer-write-string-short.js n=1000000 len=0 encoding='latin1'         ***     58.19 %      ±20.88% ±29.89% ±43.73%
buffers/buffer-write-string-short.js n=1000000 len=0 encoding='utf8'           ***     61.74 %      ±13.09% ±18.14% ±25.14%
buffers/buffer-write-string-short.js n=1000000 len=1 encoding='ascii'          ***    136.24 %      ±11.25% ±15.57% ±21.57%
buffers/buffer-write-string-short.js n=1000000 len=1 encoding='latin1'         ***    135.68 %      ±34.53% ±49.25% ±71.59%
buffers/buffer-write-string-short.js n=1000000 len=1 encoding='utf8'           ***    131.19 %      ±13.69% ±19.05% ±26.59%
buffers/buffer-write-string-short.js n=1000000 len=16 encoding='ascii'         ***    105.31 %      ±22.74% ±32.63% ±47.91%
buffers/buffer-write-string-short.js n=1000000 len=16 encoding='latin1'        ***    146.98 %      ±15.12% ±20.88% ±28.83%
buffers/buffer-write-string-short.js n=1000000 len=16 encoding='utf8'          ***    231.98 %      ±39.44% ±56.50% ±82.72%
buffers/buffer-write-string-short.js n=1000000 len=32 encoding='ascii'         ***    119.88 %      ±17.20% ±23.74% ±32.73%
buffers/buffer-write-string-short.js n=1000000 len=32 encoding='latin1'        ***    127.78 %      ±14.96% ±21.09% ±30.09%
buffers/buffer-write-string-short.js n=1000000 len=32 encoding='utf8'          ***    286.10 %      ±36.29% ±51.94% ±75.93%
buffers/buffer-write-string-short.js n=1000000 len=8 encoding='ascii'          ***    126.35 %      ±20.58% ±28.48% ±39.41%
buffers/buffer-write-string-short.js n=1000000 len=8 encoding='latin1'         ***    155.13 %       ±8.24% ±11.43% ±15.90%
buffers/buffer-write-string-short.js n=1000000 len=8 encoding='utf8'           ***    200.61 %      ±33.84% ±48.37% ±70.55%

anonrig · 2024-08-10T20:01:28Z

Probably because the benchmark has a flaw, and the input is not a one byte string. I recommend reading this amazing blog post: https://iliazeus.github.io/articles/js-string-optimizations-en/

ronag · 2024-08-10T20:38:49Z

@anonrig still only minimal difference:

buffers/buffer-write-string.js n=1000000 len=1                  9.21 %      ±12.42% ±17.25% ±24.00%
buffers/buffer-write-string.js n=1000000 len=16          *      5.12 %       ±4.58%  ±6.53%  ±9.52%
buffers/buffer-write-string.js n=1000000 len=32                 2.87 %       ±8.49% ±12.18% ±17.88%
buffers/buffer-write-string.js n=1000000 len=8          **      8.27 %       ±5.40%  ±7.68% ±11.10%

anonrig · 2024-08-10T20:40:00Z

Can you add a printf to fast api function and make sure that it runs? I've had several occasions that I couldn't trigger fast path for super simple calls.

ronag · 2024-08-11T06:08:12Z

@anonrig You are right. It's never called and I have no idea why. Do you know anyone that might know more?

ronag · 2024-08-11T06:31:06Z

Ah, it's because buffer is this and not the first arg. Do you know if fast API works with this args?

targos · 2024-08-11T06:35:20Z

The first arg (receiver) is the this arg.

ronag · 2024-08-11T06:39:03Z

Tried with no this and still no success... seems like a dead end for me

lib/buffer.js

santigimeno · 2024-08-11T10:32:03Z

This works for me and the fast api path is hit starting around 1e4 iterations in the benchmark:

diff --git a/lib/buffer.js b/lib/buffer.js
index 37051c94073..cd4f8f296a1 100644
--- a/lib/buffer.js
+++ b/lib/buffer.js
@@ -54,8 +54,9 @@ const {
   Uint8ArrayPrototype,
 } = primordials;
 
+const bufferBinding = internalBinding('buffer');
+
 const {
-  stringWrite,
   byteLengthUtf8,
   compare: _compare,
   compareOffset,
@@ -658,7 +659,7 @@ const encodingOps = {
     encoding: 'ascii',
     encodingVal: encodingsMap.ascii,
     byteLength: (string) => string.length,
-    write: (buf, string, offset, len) => stringWrite(buf, string, offset, len),
+    write: (buf, string, offset, len) => bufferBinding.stringWrite(buf, string, offset, len),
     slice: (buf, start, end) => buf.asciiSlice(start, end),
     indexOf: (buf, val, byteOffset, dir) =>
       indexOfBuffer(buf,
diff --git a/src/node_buffer.cc b/src/node_buffer.cc
index 3247797dec4..acf2677d65a 100644
--- a/src/node_buffer.cc
+++ b/src/node_buffer.cc
@@ -1422,6 +1422,7 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
   registry->Register(StringSlice<UCS2>);
   registry->Register(StringSlice<UTF8>);
 
+  registry->Register(SlowWriteString<ASCII>);
   registry->Register(fast_write_string.GetTypeInfo());
   registry->Register(FastWriteString);
   registry->Register(StringWrite<ASCII>);

ronag · 2024-08-11T14:06:12Z

Got it to work!

buffers/buffer-write-string.js n=1000000 str='a'           ***    306.67 %       ±7.42% ±10.17% ±13.87%
buffers/buffer-write-string.js n=1000000 str='aa'          ***    289.45 %       ±3.26%  ±4.59%  ±6.55%
buffers/buffer-write-string.js n=1000000 str='aaaa'        ***    271.79 %      ±12.09% ±16.96% ±24.00%

looks promising

nodejs-github-bot · 2024-08-14T08:39:47Z

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

nodejs-github-bot · 2024-08-14T10:02:31Z

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

nodejs-github-bot · 2024-08-14T11:58:45Z

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

nodejs-github-bot · 2024-08-14T14:27:38Z

Commit Queue failed

- Loading data for nodejs/node/pull/54311
✔  Done loading data for nodejs/node/pull/54311
----------------------------------- PR info ------------------------------------
Title      buffer: use fast API for writing one-byte strings (#54311)
Author     Robert Nagy <ronagy@icloud.com> (@ronag)
Branch     ronag:write-string-fast-api -> nodejs:main
Labels     buffer, c++, notable-change, author ready, needs-ci
Commits    1
 - buffer: use fast API for writing one-byte strings
Committers 1
 - Robert Nagy <ronagy@icloud.com>
PR-URL: https://github.com/nodejs/node/pull/54311
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/54311
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - buffer: use fast API for writing one-byte strings
   ℹ  This PR was created on Sat, 10 Aug 2024 19:59:03 GMT
   ✔  Approvals: 6
   ✔  - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/54311#pullrequestreview-2231748283
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/54311#pullrequestreview-2236268460
   ✔  - Santiago Gimeno (@santigimeno): https://github.com/nodejs/node/pull/54311#pullrequestreview-2231856267
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/54311#pullrequestreview-2231867115
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/54311#pullrequestreview-2232130355
   ✔  - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/54311#pullrequestreview-2233878460
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-08-14T11:58:45Z: https://ci.nodejs.org/job/node-test-pull-request/61103/
- Querying data for job/node-test-pull-request/61103/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/10389383045

nodejs-github-bot · 2024-08-15T01:01:07Z

Landed in ccf05ef

ShenHongFei · 2024-08-15T03:01:33Z

Access violation occurred in FastWriteString function, causing the process to crash and exit. @ronag

Refs: nodejs#54311 (comment) PR-URL: nodejs#54391

ronag · 2024-08-15T06:27:47Z

@ShenHongFei Thanks for reporting! Fix is ready.

Refs: nodejs#54311 (comment) PR-URL: nodejs#54391

ShenHongFei · 2024-08-15T06:42:25Z

I tried applying the patch https://github.com/nodejs/node/pull/54391/files and the problem was solved. Thank you.

Refs: #54311 (comment) PR-URL: #54391 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>

PR-URL: #54310 PR-URL: #54311 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Aug 10, 2024

targos approved these changes Aug 11, 2024

View reviewed changes

lib/buffer.js Outdated Show resolved Hide resolved

ronag force-pushed the write-string-fast-api branch 2 times, most recently from de22cf8 to 63e7cda Compare August 11, 2024 07:36

ronag force-pushed the write-string-fast-api branch 5 times, most recently from f3259aa to d1eb51b Compare August 11, 2024 14:25

ronag marked this pull request as ready for review August 11, 2024 14:26

ronag requested review from anonrig, benjamingr and mcollina August 11, 2024 14:26

ronag force-pushed the write-string-fast-api branch 2 times, most recently from 9c98f8f to 2596951 Compare August 11, 2024 14:29

ronag changed the title ~~Write string fast api~~ buffer: use fast API for writing one-byte strings Aug 11, 2024

anonrig approved these changes Aug 11, 2024

View reviewed changes

ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 11, 2024

ronag force-pushed the write-string-fast-api branch from 2596951 to 6594e2e Compare August 11, 2024 15:12

ronag added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Aug 14, 2024

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Aug 14, 2024

ronag requested review from BridgeAR, ShogunPanda and anonrig August 14, 2024 17:09

anonrig approved these changes Aug 14, 2024

View reviewed changes

jakecastelli added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Aug 15, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 15, 2024

nodejs-github-bot merged commit ccf05ef into nodejs:main Aug 15, 2024

This comment was marked as outdated.

Sign in to view

ronag mentioned this pull request Aug 15, 2024

buffer: properly apply dst offset and src length on fast path #54391

Merged

ronag added a commit to nxtedition/node that referenced this pull request Aug 15, 2024

buffer: properly apply offset on fast path

ea45c6c

Refs: nodejs#54311 (comment) PR-URL: nodejs#54391

ronag added a commit to nxtedition/node that referenced this pull request Aug 15, 2024

buffer: properly apply offset on fast path

9795353

Refs: nodejs#54311 (comment) PR-URL: nodejs#54391

ronag added a commit to nxtedition/node that referenced this pull request Aug 15, 2024

buffer: properly apply dst offset and src length on fast path

bbafef7

Refs: nodejs#54311 (comment) PR-URL: nodejs#54391

richardlau mentioned this pull request Aug 15, 2024

src: move more crypto code to ncrypto #54320

Closed

RafaelGSS mentioned this pull request Aug 19, 2024

v22.7.0 proposal #54452

Merged

dlarocque mentioned this pull request Aug 23, 2024

Compatibility error with Node 22.7 firebase/firebase-js-sdk#8455

Closed

Uh oh!

Conversation

ronag commented Aug 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anonrig commented Aug 10, 2024

Uh oh!

ronag commented Aug 10, 2024

Uh oh!

anonrig commented Aug 10, 2024

Uh oh!

ronag commented Aug 11, 2024

Uh oh!

ronag commented Aug 11, 2024

Uh oh!

targos commented Aug 11, 2024

Uh oh!

ronag commented Aug 11, 2024

Uh oh!

Uh oh!

santigimeno commented Aug 11, 2024

Uh oh!

ronag commented Aug 11, 2024

Uh oh!

nodejs-github-bot commented Aug 14, 2024

Uh oh!

nodejs-github-bot commented Aug 14, 2024

Uh oh!

nodejs-github-bot commented Aug 14, 2024 • edited by jakecastelli Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 14, 2024

Uh oh!

nodejs-github-bot commented Aug 15, 2024

Uh oh!

ShenHongFei commented Aug 15, 2024

Uh oh!

This comment was marked as outdated.

ronag commented Aug 15, 2024

Uh oh!

ShenHongFei commented Aug 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

ronag commented Aug 10, 2024 •

edited

Loading

nodejs-github-bot commented Aug 14, 2024 •

edited by jakecastelli

Loading