Skip to content

buffer: add fast api for isAscii & isUtf8#58058

Open
anonrig wants to merge 4 commits into
nodejs:mainfrom
anonrig:yagiz/add-fast-api-isascii
Open

buffer: add fast api for isAscii & isUtf8#58058
anonrig wants to merge 4 commits into
nodejs:mainfrom
anonrig:yagiz/add-fast-api-isascii

Conversation

@anonrig

@anonrig anonrig commented Apr 28, 2025

Copy link
Copy Markdown
Member

Adds v8 fast api for IsUtf8 and IsAscii methods

isAscii benchmark: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1706/

                                                                        confidence improvement accuracy (*)   (**)  (***)
buffers/buffer-isascii.js input='hello world' length='long' n=20000000         ***     37.08 %       ±0.87% ±1.17% ±1.52%
buffers/buffer-isascii.js input='hello world' length='short' n=20000000        ***     36.83 %       ±0.86% ±1.15% ±1.51%

isUtf8 benchmark: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1708/

buffers/buffer-isutf8.js input='∀x∈ℝ: ⌈x⌉ = −⌊−x⌋' length='long' n=20000000         ***      1.95 %       ±0.22% ±0.29% ±0.38%
buffers/buffer-isutf8.js input='∀x∈ℝ: ⌈x⌉ = −⌊−x⌋' length='short' n=20000000        ***     65.95 %       ±2.35% ±3.13% ±4.08%
buffers/buffer-isutf8.js input='regular string' length='long' n=20000000            ***     42.16 %       ±4.05% ±5.39% ±7.02%
buffers/buffer-isutf8.js input='regular string' length='short' n=20000000           ***    121.41 %       ±1.15% ±1.54% ±2.02%

@anonrig anonrig requested review from jasnell, lemire and ronag April 28, 2025 01:23
@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 Apr 28, 2025
@anonrig anonrig force-pushed the yagiz/add-fast-api-isascii branch from 6bb4020 to 8432a57 Compare April 28, 2025 01:23
@anonrig anonrig requested review from H4ad and mcollina April 28, 2025 01:34
Comment thread src/node_buffer.cc Outdated
@codecov

codecov Bot commented Apr 28, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 76.00000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.16%. Comparing base (723d7bb) to head (6b11f13).
⚠️ Report is 2976 commits behind head on main.

Files with missing lines Patch % Lines
src/node_buffer.cc 76.00% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #58058      +/-   ##
==========================================
- Coverage   90.17%   90.16%   -0.02%     
==========================================
  Files         630      630              
  Lines      186473   186496      +23     
  Branches    36613    36614       +1     
==========================================
+ Hits       168160   168162       +2     
- Misses      11128    11132       +4     
- Partials     7185     7202      +17     
Files with missing lines Coverage Δ
src/node_buffer.cc 69.20% <76.00%> (+0.10%) ⬆️

... and 24 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@lemire lemire left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great work.

@anonrig anonrig force-pushed the yagiz/add-fast-api-isascii branch 2 times, most recently from 2841143 to ab8d328 Compare April 29, 2025 17:00
@anonrig anonrig requested review from Renegade334, lemire and ronag April 29, 2025 17:00
Comment thread src/node_buffer.cc Outdated
@anonrig anonrig force-pushed the yagiz/add-fast-api-isascii branch from ab8d328 to d7ffbed Compare May 3, 2025 16:24
@anonrig anonrig requested review from Renegade334, addaleax and mcollina and removed request for Renegade334 May 3, 2025 16:24
@anonrig anonrig force-pushed the yagiz/add-fast-api-isascii branch from d7ffbed to 54410b2 Compare May 3, 2025 16:25
@anonrig anonrig force-pushed the yagiz/add-fast-api-isascii branch from 54410b2 to 7e84063 Compare May 3, 2025 16:26
@anonrig

anonrig commented May 3, 2025

Copy link
Copy Markdown
Member Author

@jasnell @Renegade334 can you review this pull-request one more time?

@anonrig anonrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels May 3, 2025
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 3, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@Renegade334 Renegade334 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Might be worth re-running benchmarks, but LGTM 👍

(Needs dont-land-on-v20.x/v22.x, as the old fast API did not provide access to the isolate.)

Comment thread src/node_buffer.cc
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label May 7, 2025
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 7, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 26, 2025
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 26, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@jasnell

jasnell commented Jun 29, 2025

Copy link
Copy Markdown
Member

The failure in CI here is not a flaky failure. It's relevant to the change being made here. That should be fixed before trying CI again.

@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 2, 2026
@slagiewka

Copy link
Copy Markdown

Looks like this was covered and released in #64169.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Development

Successfully merging this pull request may close these issues.