Skip to content

buffer: add fast api for isAscii & isUtf8 - #58058

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

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 3760 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.

@jasnell jasnell closed this Aug 17, 2026
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.