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

src: replace custom ASCII validation with simdutf one by addaleax · Pull Request #46271 · nodejs/node · GitHub

/ node Public

src: replace custom ASCII validation with simdutf one - #46271

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
addaleax:simdutf-containsnonascii
Jan 21, 2023
ConversationCommits (1)ChecksFiles changed
Merged

src: replace custom ASCII validation with simdutf one#46271
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
addaleax:simdutf-containsnonascii

Conversation

addaleax commented Jan 19, 2023

Copy link
Copy Markdown
Member

No description provided.

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 Jan 19, 2023
tniessen approved these changes Jan 19, 2023
View reviewed changes

tniessen 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

I would have (naively) assumed that many compilers would vectorize contains_non_ascii_slow.

Comment thread src/string_bytes.cc Outdated

addaleax commented Jan 19, 2023

Copy link
Copy Markdown
Member Author

@tniessen Me too! Fwiw, I’m not really trying to optimize anything here (who even uses 'ascii'…) but rather just get rid of code we don’t need anymore.

richardlau commented Jan 19, 2023

Copy link
Copy Markdown
Member

The commit title is missing the "d" from "simdutf".

addaleax changed the title src: replace custom ASCII validation with simutf one src: replace custom ASCII validation with simdutf one Jan 19, 2023
addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 19, 2023
addaleax force-pushed the simdutf-containsnonascii branch from 5e740f8 to 8df6b9d Compare January 19, 2023 16:13
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 19, 2023

nodejs-github-bot commented Jan 19, 2023

Copy link
Copy Markdown
Collaborator

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

lpinca approved these changes Jan 19, 2023
View reviewed changes
Comment thread src/string_bytes.cc

case ASCII:
if (contains_non_ascii(buf, buflen)) {
if (simdutf::validate_ascii_with_errors(buf, buflen).error) {

lpinca Jan 19, 2023 •
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

Any reason for using the error version? Is the common case to have invalid ASCII?

addaleax Jan 19, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

The with_errors variant bails out early if it detects invalid ASCII, instead of running the entire string, so my thought was that it would match the performance profile of the previous code here best.

The common case is to not use this branch (ASCII) at all :)

nodejs-github-bot commented Jan 19, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Jan 19, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Jan 19, 2023

Copy link
Copy Markdown
Collaborator

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

anonrig added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jan 19, 2023
addaleax added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 19, 2023
github-actions Bot mentioned this pull request Jan 20, 2023
Open
13 tasks
bnoordhuis approved these changes Jan 20, 2023
View reviewed changes
Comment thread src/string_bytes.cc
}

return false;
}

bnoordhuis Jan 20, 2023

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

Complete aside: I have a distinct memory of writing this code, it looks like how I would write such code, yet git blame attributes it to Isaac S... huh. The human mind is a fickle thing.

addaleax Jan 20, 2023

Copy link
Copy Markdown
Member Author

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

@bnoordhuis I double-checked out of interest (and also because it looks like code from you), and … you did! e325ace is all yours 🙂

bnoordhuis Jan 20, 2023

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

I feel vindicated now! Thanks for digging that up, Anna. :)

anonrig Jan 20, 2023

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

Written almost ten years ago, and you still remember your code. I've nothing but respect for all of you.

github-actions Bot mentioned this pull request Jan 21, 2023
Open
14 tasks
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 21, 2023
nodejs-github-bot merged commit 6913140 into nodejs:main Jan 21, 2023

nodejs-github-bot commented Jan 21, 2023

Copy link
Copy Markdown
Collaborator

Landed in 6913140

This was referenced Jan 22, 2023
Open
Open
addaleax deleted the simdutf-containsnonascii branch January 23, 2023 10:14
This was referenced Jan 24, 2023
Open
Open
ruyadorno pushed a commit that referenced this pull request Feb 1, 2023
src: replace custom ASCII validation with simdutf one
35bf93b 
PR-URL: #46271
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
ruyadorno mentioned this pull request Feb 1, 2023
Merged
juanarbol pushed a commit that referenced this pull request Mar 3, 2023
src: replace custom ASCII validation with simdutf one
b7bdf1d 
PR-URL: #46271
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
juanarbol mentioned this pull request Mar 3, 2023
Merged
juanarbol pushed a commit that referenced this pull request Mar 5, 2023
src: replace custom ASCII validation with simdutf one
18c4dd0 
PR-URL: #46271
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
This was referenced Dec 19, 2025
Merged
Merged
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

bnoordhuis bnoordhuis approved these changes
anonrig anonrig approved these changes
jasnell jasnell approved these changes
lpinca lpinca approved these changes
cjihrig cjihrig approved these changes
tniessen tniessen approved these changes
richardlau richardlau approved these changes
RaisinTen RaisinTen approved these changes

Assignees

No one assigned

Labels

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

10 participants

Back | FazBrowse Home | New Git URL