Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

buffer: use fast API for writing one-byte strings #54311

Merged
merged 1 commit into from
Aug 15, 2024

Conversation

ronag
Copy link
Member

@ronag ronag commented Aug 10, 2024

                                                                        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%

@nodejs-github-bot 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
@anonrig
Copy link
Member

anonrig commented Aug 10, 2024

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
Copy link
Member Author

ronag commented Aug 10, 2024

@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
Copy link
Member

anonrig commented Aug 10, 2024

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
Copy link
Member Author

ronag commented Aug 11, 2024

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

@ronag
Copy link
Member Author

ronag commented Aug 11, 2024

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

@targos
Copy link
Member

targos commented Aug 11, 2024

The first arg (receiver) is the this arg.

@ronag
Copy link
Member Author

ronag commented Aug 11, 2024

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

lib/buffer.js Outdated Show resolved Hide resolved
@ronag ronag force-pushed the write-string-fast-api branch 2 times, most recently from de22cf8 to 63e7cda Compare August 11, 2024 07:36
@santigimeno
Copy link
Member

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
Copy link
Member Author

ronag commented Aug 11, 2024

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

@ronag ronag force-pushed the write-string-fast-api branch 5 times, most recently from f3259aa to d1eb51b Compare August 11, 2024 14:25
@ronag ronag marked this pull request as ready for review August 11, 2024 14:26
@ronag ronag force-pushed the write-string-fast-api branch 2 times, most recently from 9c98f8f to 2596951 Compare August 11, 2024 14:29
@ronag ronag changed the title Write string fast api buffer: use fast API for writing one-byte strings Aug 11, 2024
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 11, 2024
@jakecastelli 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 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 nodejs-github-bot merged commit ccf05ef into nodejs:main Aug 15, 2024
58 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in ccf05ef

@ShenHongFei
Copy link
Contributor

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

2024 08 15 10 54 12

@ronag

This comment was marked as outdated.

ronag added a commit to nxtedition/node that referenced this pull request Aug 15, 2024
ronag added a commit to nxtedition/node that referenced this pull request Aug 15, 2024
@ronag
Copy link
Member Author

ronag commented Aug 15, 2024

@ShenHongFei Thanks for reporting! Fix is ready.

ronag added a commit to nxtedition/node that referenced this pull request Aug 15, 2024
@ShenHongFei
Copy link
Contributor

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

nodejs-github-bot pushed a commit that referenced this pull request Aug 15, 2024
Refs: #54311 (comment)
PR-URL: #54391
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jake Yuesong Li <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Aug 19, 2024
PR-URL: #54310
PR-URL: #54311
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Aug 19, 2024
Refs: #54311 (comment)
PR-URL: #54391
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jake Yuesong Li <[email protected]>
RafaelGSS added a commit that referenced this pull request Aug 19, 2024
Notable changes:

buffer:
  * use fast API for writing one-byte strings (Robert Nagy) #54311
  * optimize createFromString (Robert Nagy) #54324
  * use native copy impl (Robert Nagy) #54087
inspector:
  * (SEMVER-MINOR) support `Network.loadingFailed` event (Kohei Ueno) #54246
lib:
  * (SEMVER-MINOR) rewrite AsyncLocalStorage without async_hooks (Stephen Belanger) #48528
module:
  * (SEMVER-MINOR) unflag detect-module (Geoffrey Booth) #53619

PR-URL: TODO
RafaelGSS added a commit that referenced this pull request Aug 19, 2024
Notable changes:

buffer:
  * use fast API for writing one-byte strings (Robert Nagy) #54311
  * optimize createFromString (Robert Nagy) #54324
  * use native copy impl (Robert Nagy) #54087
inspector:
  * (SEMVER-MINOR) support `Network.loadingFailed` event (Kohei Ueno) #54246
lib:
  * (SEMVER-MINOR) rewrite AsyncLocalStorage without async_hooks (Stephen Belanger) #48528
module:
  * (SEMVER-MINOR) unflag detect-module (Geoffrey Booth) #53619

PR-URL: #54452
@RafaelGSS RafaelGSS mentioned this pull request Aug 19, 2024
RafaelGSS added a commit that referenced this pull request Aug 20, 2024
Notable changes:

buffer:
  * use fast API for writing one-byte strings (Robert Nagy) #54311
  * optimize createFromString (Robert Nagy) #54324
  * use native copy impl (Robert Nagy) #54087
inspector:
  * (SEMVER-MINOR) support `Network.loadingFailed` event (Kohei Ueno) #54246
lib:
  * (SEMVER-MINOR) rewrite AsyncLocalStorage without async_hooks (Stephen Belanger) #48528
module:
  * (SEMVER-MINOR) add --experimental-transform-types flag (Marco Ippolito) #54283
  * (SEMVER-MINOR) unflag detect-module (Geoffrey Booth) #53619

PR-URL: #54452
RafaelGSS pushed a commit that referenced this pull request Aug 21, 2024
PR-URL: #54310
PR-URL: #54311
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Aug 21, 2024
Refs: #54311 (comment)
PR-URL: #54391
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jake Yuesong Li <[email protected]>
RafaelGSS added a commit that referenced this pull request Aug 21, 2024
Notable changes:

buffer:
  * use fast API for writing one-byte strings (Robert Nagy) #54311
  * optimize createFromString (Robert Nagy) #54324
  * use native copy impl (Robert Nagy) #54087
inspector:
  * (SEMVER-MINOR) support `Network.loadingFailed` event (Kohei Ueno) #54246
lib:
  * (SEMVER-MINOR) rewrite AsyncLocalStorage without async_hooks (Stephen Belanger) #48528
module:
  * (SEMVER-MINOR) add --experimental-transform-types flag (Marco Ippolito) #54283
  * (SEMVER-MINOR) unflag detect-module (Geoffrey Booth) #53619

PR-URL: #54452
RafaelGSS added a commit that referenced this pull request Aug 22, 2024
Notable changes:

buffer:
  * use fast API for writing one-byte strings (Robert Nagy) #54311
  * optimize createFromString (Robert Nagy) #54324
  * use native copy impl (Robert Nagy) #54087
inspector:
  * (SEMVER-MINOR) support `Network.loadingFailed` event (Kohei Ueno) #54246
lib:
  * (SEMVER-MINOR) rewrite AsyncLocalStorage without async_hooks (Stephen Belanger) #48528
module:
  * (SEMVER-MINOR) add --experimental-transform-types flag (Marco Ippolito) #54283
  * (SEMVER-MINOR) unflag detect-module (Geoffrey Booth) #53619

PR-URL: #54452
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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. notable-change PRs with changes that should be highlighted in changelogs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants