GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — buffer: re-enable Fast API for Buffer.write by ronag · Pull Request #54526 · nodejs/node

Conversation

ronag commented Aug 23, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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%

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

theill, bricss, and mertcanaltin reacted with thumbs up emoji

bricss, avivkeller, ShenHongFei, jazelly, theill, mertcanaltin, and tomweptinstall reacted with rocket emoji

All reactions

👍 3 reactions
🚀 7 reactions

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

Comment thread

src/node_buffer.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.

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

Comment thread

test/parallel/test-buffer-write.js Outdated

lemire Aug 23, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a non-deterministic test is unpleasant.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

anonrig Aug 23, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

lemire approved these changes

Aug 23, 2024

View reviewed changes

lemire left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

codecov Bot commented Aug 23, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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. Only starts once the PR has an approving review. label

Aug 23, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Aug 23, 2024

This comment was marked as outdated.

ronag force-pushed the fast-utf8-2 branch from 8be5c43 to 79573f4 Compare

August 23, 2024 18:43

ronag added the author ready PRs with CI started, the required approvals, and no outstanding review comments. 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

jasnell left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some lint issues but otherwise LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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. Only starts once the PR has an approving review. label

Aug 24, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Aug 24, 2024

This comment was marked as outdated.

webharerob commented Aug 24, 2024 •

edited by VoltrexKeyva

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

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.

addaleax and yepelinux reacted with thumbs up emoji

All reactions

👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ronag commented Aug 24, 2024

Copy link

Copy Markdown

Member Author

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ronag commented Aug 24, 2024

Copy link

Copy Markdown

Member Author

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

@lemira does simdutf have this? validate_ascii?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

anonrig approved these changes

Aug 30, 2024

View reviewed changes

nodejs-github-bot commented Aug 30, 2024

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

benjamingr approved these changes

Aug 30, 2024

View reviewed changes

nodejs-github-bot commented Aug 30, 2024

Copy link

Copy Markdown

Collaborator

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

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

There was an error while loading. Please reload this page.

targos commented Sep 2, 2024

Copy link

Copy Markdown

Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          buffer: re-enable Fast API for Buffer.write

0ae2ae1

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

Fixes: nodejs#54521

nodejs-github-bot commented Sep 4, 2024

Copy link

Copy Markdown

Collaborator

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

ronag reacted with thumbs up emoji

ronag reacted with rocket emoji

All reactions

👍 1 reaction
🚀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

anonrig approved these changes

Sep 4, 2024

View reviewed changes

nodejs-github-bot commented Sep 4, 2024

Copy link

Copy Markdown

Collaborator

Landed in dc74f17

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

RafaelGSS mentioned this pull request

Sep 16, 2024

v22.9.0 proposal #54966

Merged

Reviewers

avivkeller

avivkeller left review comments

ShogunPanda

ShogunPanda approved these changes

benjamingr

benjamingr approved these changes

anonrig

anonrig approved these changes

lemire

Awaiting requested review from lemire

addaleax

Awaiting requested review from addaleax

webharerob

Awaiting requested review from webharerob

jasnell

Awaiting requested review from jasnell

+1 more reviewer

targos

targos left review comments

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready

PRs with CI started, the required approvals, and no outstanding review comments.

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

12 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

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.

webharerob 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

Uh oh!

There was an error while loading. Please reload this page.

codecov Bot commented Aug 23, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

webharerob commented Aug 24, 2024 •

edited by VoltrexKeyva

Loading

Uh oh!

There was an error while loading. Please reload this page.

avivkeller commented Sep 1, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.