src: replace custom ASCII validation with simdutf one#46271
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added buffer 
tniessen left a comment
tniessen left a comment There was a problem hiding this comment.
I would have (naively) assumed that many compilers would vectorize contains_non_ascii_slow.
Uh oh!
There was an error while loading. Please reload this page.
addaleax commented Jan 19, 2023
@tniessen Me too! Fwiw, I’m not really trying to optimize anything here (who even uses |
richardlau commented Jan 19, 2023
The commit title is missing the "d" from "simdutf". |
addaleax changed the title addaleaxforce-pushed the simdutf-containsnonascii branch from 5e740f8 to 8df6b9dCompare
github-actionsbot removed the request-ci 
nodejs-github-bot commented Jan 19, 2023
| case ASCII: | ||
| if (contains_non_ascii(buf, buflen)){ | ||
| if (simdutf::validate_ascii_with_errors(buf, buflen).error){ |
There was a problem hiding this comment.
Any reason for using the error version? Is the common case to have invalid ASCII?
There was a problem hiding this comment.
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
nodejs-github-bot commented Jan 19, 2023
nodejs-github-bot commented Jan 19, 2023
anonrig added the author ready addaleax added the commit-queue 
| } | ||
| returnfalse; | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@bnoordhuis I double-checked out of interest (and also because it looks like code from you), and … you did! e325ace is all yours 🙂
There was a problem hiding this comment.
I feel vindicated now! Thanks for digging that up, Anna. :)
There was a problem hiding this comment.
Written almost ten years ago, and you still remember your code. I've nothing but respect for all of you.
nodejs-github-bot removed the commit-queue nodejs-github-bot commented Jan 21, 2023
Landed in 6913140 |
PR-URL: #46271 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Darshan Sen <[email protected]>
PR-URL: #46271 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Darshan Sen <[email protected]>
PR-URL: #46271 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Darshan Sen <[email protected]>
10 participants
No description provided.