buffer: re-enable Fast API for Buffer.write by ronag · Pull Request #54526 · nodejs/node

ronag · 2024-08-23T14:34:31Z

Re-enables fast Fast API for Buffer.write after fixing UTF8 handling.

13:05:25 buffers/buffer-write-string-short.js n=1000000 len=1 encoding='ascii'                            ***    266.52 %       ±7.27%  ±9.75% ±12.84%
13:05:25 buffers/buffer-write-string-short.js n=1000000 len=1 encoding='latin1'                           ***    201.16 %       ±4.74%  ±6.33%  ±8.29%
13:05:25 buffers/buffer-write-string-short.js n=1000000 len=1 encoding='utf8'                             ***    242.07 %       ±6.29%  ±8.44% ±11.14%
13:05:25 buffers/buffer-write-string-short.js n=1000000 len=16 encoding='ascii'                           ***    264.81 %       ±5.88%  ±7.86% ±10.29%
13:05:25 buffers/buffer-write-string-short.js n=1000000 len=16 encoding='latin1'                          ***    197.28 %       ±6.96%  ±9.32% ±12.26%
13:05:25 buffers/buffer-write-string-short.js n=1000000 len=16 encoding='utf8'                            ***    349.02 %       ±8.80% ±11.84% ±15.68%
13:05:25 buffers/buffer-write-string-short.js n=1000000 len=32 encoding='ascii'                           ***    271.75 %       ±8.70% ±11.69% ±15.43%
13:05:25 buffers/buffer-write-string-short.js n=1000000 len=32 encoding='latin1'                          ***    214.58 %       ±5.85%  ±7.84% ±10.32%
13:05:25 buffers/buffer-write-string-short.js n=1000000 len=32 encoding='utf8'                            ***    351.17 %       ±7.64% ±10.28% ±13.60%
13:05:25 buffers/buffer-write-string-short.js n=1000000 len=8 encoding='ascii'                            ***    273.30 %       ±8.65% ±11.63% ±15.38%
13:05:25 buffers/buffer-write-string-short.js n=1000000 len=8 encoding='latin1'                           ***    191.62 %       ±5.62%  ±7.52%  ±9.88%
13:05:25 buffers/buffer-write-string-short.js n=1000000 len=8 encoding='utf8'                             ***    224.55 %       ±4.72%  ±6.33%  ±8.33%

src/node_buffer.cc

lemire · 2024-08-23T14:59:10Z

test/parallel/test-buffer-write.js

+{
+  let i = 0;
+
+  while (i < 1_000_000) {


Having a non-deterministic test is unpleasant.

You can trigger fast api call using the method @targos implemented in his previous pull requests.

lemire

This looks to me. I wish we could use a deterministic test. Having to loop a million times to trigger and issue is both slow and error prone.

codecov · 2024-08-23T16:05:22Z

Codecov Report

Attention: Patch coverage is 88.88889% with 8 lines in your changes missing coverage. Please review.

Project coverage is 87.60%. Comparing base (981c701) to head (0ae2ae1).
Report is 252 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_buffer.cc	88.88%	3 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54526      +/-   ##
==========================================
- Coverage   87.61%   87.60%   -0.01%     
==========================================
  Files         650      650              
  Lines      182835   182889      +54     
  Branches    35382    35389       +7     
==========================================
+ Hits       160185   160227      +42     
- Misses      15925    15927       +2     
- Partials     6725     6735      +10

Files with missing lines	Coverage Δ
src/node_buffer.cc	`71.41% <88.88%> (+1.46%)`	⬆️

... and 32 files with indirect coverage changes

jasnell

Some lint issues but otherwise LGTM

blexrob · 2024-08-24T07:38:59Z

With the current commits, the following test will fail:

let i = 0;
const testStr = "\xc2\x80"; // when stored in 1-byte chars, this is a valid UTF-8 encoding
const expected = Buffer.from(testStr).toString("hex");
for(; i < 1_000_000; i++) {
  const buf = Buffer.from(testStr);
  const ashex = buf.toString("hex");
  if (ashex !== expected) {
    console.log(`Decoding changed in iteration ${i} when changing to FastWriteStringUTF8, got ${ashex}, expected ${expected}`);
    break;
  }
}

if(i<1_000_000) {
  console.error("FAILED after %d iterations",i);
} else
  console.log("PASSED after %d iterations",i);

The source string is stored as UTF16 (compressed to 1 byte), but still needs to be converted to UTF-8. The fast path can only be implemented as memcpy when only characters from the range 0-127 are present.

ronag · 2024-08-24T14:23:39Z

The source string is stored as UTF16 (compressed to 1 byte), but still needs to be converted to UTF-8. The fast path can only be implemented as memcpy when only characters from the range 0-127 are present.

I thought FastOneByteString means that it's only one byte chars?

ronag · 2024-08-24T14:25:11Z

< when only characters from the range 0-127 are present

@lemira does simdutf have this? validate_ascii?

nodejs-github-bot · 2024-08-30T13:34:03Z

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

nodejs-github-bot · 2024-08-30T19:01:12Z

Commit Queue failed

- Loading data for nodejs/node/pull/54526
✔  Done loading data for nodejs/node/pull/54526
----------------------------------- PR info ------------------------------------
Title      buffer: validate UTF8 on fast path  (#54526)
Author     Robert Nagy <[email protected]> (@ronag)
Branch     ronag:fast-utf8-2 -> nodejs:main
Labels     buffer, c++, author ready, needs-ci
Commits    1
 - buffer: validate UTF8 on fast path
Committers 1
 - Robert Nagy <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/54526
Fixes: https://github.com/nodejs/node/issues/54521
Reviewed-By: Daniel Lemire <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/54526
Fixes: https://github.com/nodejs/node/issues/54521
Reviewed-By: Daniel Lemire <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 23 Aug 2024 14:34:31 GMT
   ✔  Approvals: 6
   ✔  - Daniel Lemire (@lemire): https://github.com/nodejs/node/pull/54526#pullrequestreview-2257470621
   ✔  - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/54526#pullrequestreview-2273156047
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/54526#pullrequestreview-2258976699
   ✔  - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/54526#pullrequestreview-2258958504
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/54526#pullrequestreview-2272343097
   ✔  - Paolo Insogna (@ShogunPanda) (TSC): https://github.com/nodejs/node/pull/54526#pullrequestreview-2260113366
   ✘  Last GitHub CI failed
   ℹ  Last Full PR CI on 2024-08-30T13:34:03Z: https://ci.nodejs.org/job/node-test-pull-request/61707/
   ℹ  Last Benchmark CI on 2024-08-30T10:28:48Z: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1626/
- Querying data for job/node-test-pull-request/61707/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

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

nodejs-github-bot · 2024-08-30T21:32:06Z

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

nodejs-github-bot · 2024-08-31T08:37:07Z

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

nodejs-github-bot · 2024-08-31T12:45:53Z

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

nodejs-github-bot · 2024-08-31T13:39:10Z

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

nodejs-github-bot · 2024-08-31T15:22:08Z

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

nodejs-github-bot · 2024-09-01T04:12:59Z

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

nodejs-github-bot · 2024-09-01T09:36:50Z

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

nodejs-github-bot · 2024-09-01T16:08:38Z

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

avivkeller · 2024-09-01T16:14:36Z

Given the repeated activity since the failed commit, I assume it is known that the commit-queue originally failed, so I removed the label. Feel free to re-add.

nodejs-github-bot · 2024-09-01T16:23:31Z

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

avivkeller · 2024-09-02T07:30:28Z

Correct me if I'm wrong, but shouldn't the "PR-URL" meta be added by the bot?

targos · 2024-09-02T07:38:47Z

Correct me if I'm wrong, but shouldn't the "PR-URL" meta be added by the bot?

You're right. @ronag please remove the PR-URL from the commit.

Re-enables fast Fast API for Buffer.write after fixing UTF8 handling. Fixes: nodejs#54521

nodejs-github-bot · 2024-09-02T14:24:58Z

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

nodejs-github-bot · 2024-09-03T04:27:28Z

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

nodejs-github-bot · 2024-09-03T10:01:31Z

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

nodejs-github-bot · 2024-09-03T10:52:44Z

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

nodejs-github-bot · 2024-09-03T11:18:41Z

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

nodejs-github-bot · 2024-09-03T14:41:45Z

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

nodejs-github-bot · 2024-09-04T06:57:17Z

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

nodejs-github-bot · 2024-09-04T07:50:54Z

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

nodejs-github-bot · 2024-09-04T10:12:03Z

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

nodejs-github-bot · 2024-09-04T15:14:51Z

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

nodejs-github-bot · 2024-09-04T22:34:30Z

Landed in dc74f17

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 23, 2024

ronag commented Aug 23, 2024

View reviewed changes

src/node_buffer.cc Outdated Show resolved Hide resolved

lemire mentioned this pull request Aug 23, 2024

Buffer.from() output breaks after optimizing in nodejs 22.7 #54521

Closed

ronag force-pushed the fast-utf8-2 branch 2 times, most recently from a930ad4 to 0fda693 Compare August 23, 2024 14:45

ronag mentioned this pull request Aug 23, 2024

buffer: don't use Fast API for utf8 #54525

Closed

ronag marked this pull request as ready for review August 23, 2024 14:46

ronag force-pushed the fast-utf8-2 branch 2 times, most recently from 23f1b23 to 8be5c43 Compare August 23, 2024 14:50

lemire reviewed Aug 23, 2024

View reviewed changes

lemire approved these changes Aug 23, 2024

View reviewed changes

benjamingr approved these changes Aug 23, 2024

View reviewed changes

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

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 23, 2024

This comment was marked as outdated.

Sign in to view

ronag force-pushed the fast-utf8-2 branch from 8be5c43 to 79573f4 Compare August 23, 2024 18:43

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

ronag mentioned this pull request Aug 23, 2024

copy_utf8 simdutf/simdutf#550

Closed

dlarocque mentioned this pull request Aug 23, 2024

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

Closed

jasnell approved these changes Aug 23, 2024

View reviewed changes

ronag force-pushed the fast-utf8-2 branch from 79573f4 to 8821991 Compare August 24, 2024 07:14

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

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 24, 2024

This comment was marked as outdated.

Sign in to view

anonrig approved these changes Aug 30, 2024

View reviewed changes

benjamingr approved these changes Aug 30, 2024

View reviewed changes

buffer: re-enable Fast API for Buffer.write

0ae2ae1

Re-enables fast Fast API for Buffer.write after fixing UTF8 handling. Fixes: nodejs#54521

anonrig approved these changes Sep 4, 2024

View reviewed changes

RafaelGSS mentioned this pull request Sep 16, 2024

v22.9.0 proposal #54966

Merged

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Uh oh!

lemire Aug 23, 2024

Choose a reason for hiding this comment

Uh oh!

anonrig Aug 23, 2024

Choose a reason for hiding this comment

Uh oh!

lemire left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

This comment was marked as outdated.

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

blexrob commented Aug 24, 2024 • edited by VoltrexKeyva Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Aug 24, 2024

Uh oh!

ronag commented Aug 24, 2024

Uh oh!

nodejs-github-bot commented Aug 30, 2024

Uh oh!

nodejs-github-bot commented Aug 30, 2024

Uh oh!

nodejs-github-bot commented Aug 30, 2024

Uh oh!

nodejs-github-bot commented Aug 31, 2024

Uh oh!

nodejs-github-bot commented Aug 31, 2024

Uh oh!

nodejs-github-bot commented Aug 31, 2024

Uh oh!

nodejs-github-bot commented Aug 31, 2024

Uh oh!

nodejs-github-bot commented Sep 1, 2024

Uh oh!

nodejs-github-bot commented Sep 1, 2024

Uh oh!

nodejs-github-bot commented Sep 1, 2024

Uh oh!

avivkeller commented Sep 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 1, 2024

Uh oh!

avivkeller commented Sep 2, 2024

Uh oh!

targos commented Sep 2, 2024

Uh oh!

nodejs-github-bot commented Sep 2, 2024

Uh oh!

nodejs-github-bot commented Sep 3, 2024

Uh oh!

nodejs-github-bot commented Sep 3, 2024

Uh oh!

nodejs-github-bot commented Sep 3, 2024

Uh oh!

nodejs-github-bot commented Sep 3, 2024

Uh oh!

nodejs-github-bot commented Sep 3, 2024

Uh oh!

nodejs-github-bot commented Sep 4, 2024

Uh oh!

nodejs-github-bot commented Sep 4, 2024

Uh oh!

nodejs-github-bot commented Sep 4, 2024

Uh oh!

nodejs-github-bot commented Sep 4, 2024

Uh oh!

ronag commented Aug 23, 2024 •

edited

Loading

codecov bot commented Aug 23, 2024 •

edited

Loading

blexrob commented Aug 24, 2024 •

edited by VoltrexKeyva

Loading

avivkeller commented Sep 1, 2024 •

edited

Loading