FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

buffer: add fast api for isAscii & isUtf8 by anonrig · Pull Request #58058 · nodejs/node · GitHub

/ node Public

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

Closed
anonrig wants to merge 4 commits into
nodejs:mainfrom
anonrig:yagiz/add-fast-api-isascii
ConversationCommits (4)ChecksFiles changed
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 commented Apr 28, 2025 •
edited
Loading

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

codecov Bot commented Apr 28, 2025 •
edited
Loading

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 approved these changes Apr 28, 2025
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

lemire approved these changes Apr 28, 2025
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Great work.

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 requested review from Renegade334, lemire and ronag April 29, 2025 17:00
Renegade334 reviewed Apr 29, 2025
View reviewed changes
Comment thread src/node_buffer.cc Outdated
anonrig force-pushed the yagiz/add-fast-api-isascii branch from ab8d328 to d7ffbed Compare May 3, 2025 16:24
anonrig requested review from Renegade334, addaleax and mcollina and removed request for Renegade334 May 3, 2025 16:24
anonrig force-pushed the yagiz/add-fast-api-isascii branch from d7ffbed to 54410b2 Compare May 3, 2025 16:25
anonrig force-pushed the yagiz/add-fast-api-isascii branch from 54410b2 to 7e84063 Compare May 3, 2025 16:26

anonrig commented May 3, 2025

Copy link
Copy Markdown
Member Author

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

anonrig added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels May 3, 2025
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 commented May 3, 2025

Copy link
Copy Markdown
Collaborator

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

Renegade334 reviewed May 3, 2025
View reviewed changes

Renegade334 left a comment •
edited
Loading

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

jasnell reviewed May 7, 2025
View reviewed changes
Comment thread src/node_buffer.cc
anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label May 7, 2025
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 commented May 7, 2025

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented May 9, 2025

Copy link
Copy Markdown
Collaborator

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

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 26, 2025
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 commented Jun 26, 2025

Copy link
Copy Markdown
Collaborator

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

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

nodejs-github-bot commented Jun 29, 2025

Copy link
Copy Markdown
Collaborator

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

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 removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 2, 2026

slagiewka commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Looks like this was covered and released in #64169.

jasnell closed this Aug 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
Renegade334 Renegade334 left review comments
ronag ronag approved these changes
H4ad H4ad approved these changes
Ethan-Arrowood Ethan-Arrowood approved these changes
juanarbol juanarbol approved these changes
lemire Awaiting requested review from lemire
addaleax Awaiting requested review from addaleax
mcollina Awaiting requested review from mcollina

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Back | FazBrowse Home | New Git URL