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

@anonriganonrig 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 ronagApril 28, 2025 01:23
@nodejs-github-botnodejs-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
anonrigforce-pushed the yagiz/add-fast-api-isascii branch from 6bb4020 to 8432a57CompareApril 28, 2025 01:23
@anonrig
anonrig requested review from H4ad and mcollinaApril 28, 2025 01:34
Comment threadsrc/node_buffer.cc Outdated
@codecov

codecovBot 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 linesPatch %Lines
src/node_buffer.cc76.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 linesCoverage Δ
src/node_buffer.cc69.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.

@mcollinamcollina 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

@lemirelemire 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
anonrigforce-pushed the yagiz/add-fast-api-isascii branch 2 times, most recently from 2841143 to ab8d328CompareApril 29, 2025 17:00
Comment threadsrc/node_buffer.cc Outdated
@anonrig
anonrigforce-pushed the yagiz/add-fast-api-isascii branch from ab8d328 to d7ffbedCompareMay 3, 2025 16:24
@anonrig
anonrig requested review from Renegade334, addaleax and mcollina and removed request for Renegade334May 3, 2025 16:24
@anonrig
anonrigforce-pushed the yagiz/add-fast-api-isascii branch from d7ffbed to 54410b2CompareMay 3, 2025 16:25
@anonrig
anonrigforce-pushed the yagiz/add-fast-api-isascii branch from 54410b2 to 7e84063CompareMay 3, 2025 16:26
@anonrig

Copy link
Copy Markdown
MemberAuthor

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

@anonriganonrig 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-actionsgithub-actionsBot 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

@Renegade334Renegade334 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 threadsrc/node_buffer.cc
@anonriganonrig added the request-ci Add this label to start a Jenkins CI on a PR. label May 7, 2025
@github-actionsgithub-actionsBot 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

@anonriganonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 26, 2025
@github-actionsgithub-actionsBot 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

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.

@addaleaxaddaleax 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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bufferIssues and PRs related to the buffer subsystem.c++Issues and PRs that require attention from people who are familiar with C++.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

12 participants

@anonrig@nodejs-github-bot@jasnell@slagiewka@mcollina@lemire@ronag@Renegade334@H4ad@Ethan-Arrowood@juanarbol@addaleax