Skip to content

src: replace icu with simdutf for char counts#46472

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
anonrig wants to merge 1 commit into from
Closed

src: replace icu with simdutf for char counts #46472

anonrig wants to merge 1 commit into from

Conversation

@anonrig
Copy link
Member

@anonriganonrig commented Feb 2, 2023

No description provided.

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Feb 2, 2023
@anonriganonrig requested a review from addaleaxFebruary 2, 2023 18:15
@anonriganonrig added request-ci Add this label to start a Jenkins CI on a PR. performance Issues and PRs related to the performance of Node.js. labels Feb 2, 2023
@anonrig
Copy link
MemberAuthor

anonrig commented Feb 2, 2023

cc @nodejs/cpp-reviewers

@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 2, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 2, 2023

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

@lpinca
Copy link
Member

lpinca commented Feb 2, 2023

Out of curiosity, what is the performance gain?

@anonrig
Copy link
MemberAuthor

anonrig commented Feb 2, 2023

Out of curiosity, what is the performance gain?

@lpinca I didn't run a benchmark (or can run a benchmark on CI, because the benchmark CI is down), but knowing the implementation (which is SIMD based), there should be a gain. (Long story short: I don't know lol)

@anonriganonrig added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 2, 2023
addaleax
addaleax reviewed Feb 2, 2023
View reviewed changes
Copy link
Member

@addaleaxaddaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lpinca I don't think we need to be terribly worried about performance properties of the inspector code either way 🙂 It's a nice step towards making the inspector work in --without-intl mode, I think that would be the main benefit here (assuming that that is possible on the V8 side).

@anonriganonrig requested a review from addaleaxFebruary 2, 2023 21:35
@anonriganonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 3, 2023
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 3, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 3, 2023

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

@anonriganonrig added the review wanted PRs that need reviews. label Feb 3, 2023
@anonrig
Copy link
MemberAuthor

anonrig commented Feb 3, 2023

@lpinca@addaleax Can you review it again?

@anonriganonrig added commit-queue Add this label to land a pull request using GitHub Actions. and removed review wanted PRs that need reviews. labels Feb 3, 2023
@mscdex
Copy link
Contributor

mscdex commented Feb 4, 2023

I don't think we need to be terribly worried about performance properties of the inspector code either way 🙂 It's a nice step towards making the inspector work in --without-intl mode, I think that would be the main benefit here (assuming that that is possible on the V8 side).

Maybe the PR could be re-framed then by dropping the performance tag and changing the commit message to be something like src: replace icu with simdutf for char counts ?

@anonrig
Copy link
MemberAuthor

anonrig commented Feb 4, 2023

I am fine with changing the commit name and removing a label, but I would kindly ask to do this through the CLI while merging so that we don't have to go through the flaky tests one more time. I appreciate if you can remove the commit-queue label and merge this when the time comes @mscdex

@tniessentniessen added inspector Issues and PRs related to the V8 inspector protocol and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Feb 4, 2023
@anonriganonrig removed the performance Issues and PRs related to the performance of Node.js. label Feb 4, 2023
This was referenced Feb 5, 2023
Open
Open
anonrig added a commit that referenced this pull request Feb 6, 2023
@anonrig
src: replace icu with simdutf for char counts
6a60ed8
PR-URL: #46472 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
@anonrig
Copy link
MemberAuthor

anonrig commented Feb 6, 2023

Landed in 6a60ed8

@anonriganonrig closed this Feb 6, 2023
@anonriganonrig deleted the faster-string branch February 6, 2023 13:41
@tniessentniessen changed the title src: provide faster string character countssrc: replace icu with simdutf for char countsFeb 6, 2023
MylesBorins pushed a commit that referenced this pull request Feb 18, 2023
@anonrig@MylesBorins
src: replace icu with simdutf for char counts
00b81c7
PR-URL: #46472 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Feb 19, 2023
Merged
StefanStojanovic added a commit to JaneaSystems/node that referenced this pull request Feb 22, 2023
@StefanStojanovic
win,src: revert simdutf to icu for win arm64
b7fbc9b
Many tests started failing on ARM64 Windows after migrating from icu to simdutf. This change reverts those changes for the problematic platform. Refs: nodejs#46471 Refs: nodejs#46472 Refs: nodejs#46548 Refs: simdutf/simdutf#216
@StefanStojanovicStefanStojanovic mentioned this pull request Feb 22, 2023
Closed
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
@anonrig@danielleadams
src: replace icu with simdutf for char counts
aaf04f4
PR-URL: #46472 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
@danielleadamsdanielleadams mentioned this pull request Apr 11, 2023
Merged
codebytere added a commit to electron/electron that referenced this pull request Apr 15, 2023
@codebytere
fix: add simdutf dep to src/inspector BUILD.gn
5305acb
- nodejs/node#46471 - nodejs/node#46472
codebytere added a commit to electron/electron that referenced this pull request Apr 16, 2023
@codebytere
fix: add simdutf dep to src/inspector BUILD.gn
b21ca56
- nodejs/node#46471 - nodejs/node#46472
codebytere added a commit to electron/electron that referenced this pull request Apr 17, 2023
@codebytere
fix: add simdutf dep to src/inspector BUILD.gn
8096620
- nodejs/node#46471 - nodejs/node#46472
codebytere added a commit to electron/electron that referenced this pull request Apr 18, 2023
@electron-roller@codebytere
chore: bump node to v18.16.0 (main) (#37973)
de192c2
* chore: bump node in DEPS to v18.16.0 * build,test: add proper support for IBM i nodejs/node#46739 * lib: enforce use of trailing commas nodejs/node#46881 * src: add initial support for single executable applications nodejs/node#45038 * lib: do not crash using workers with disabled shared array buffers nodejs/node#41023 * src: remove shadowed variable in OptionsParser::Parse nodejs/node#46672 * src: allow embedder control of code generation policy nodejs/node#46368 * src: allow optional Isolate termination in node::Stop() nodejs/node#46583 * lib: fix BroadcastChannel initialization location nodejs/node#46864 * chore: fixup patch indices * chore: sync filenames.json * fix: add simdutf dep to src/inspector BUILD.gn - nodejs/node#46471 - nodejs/node#46472 * deps: replace url parser with Ada nodejs/node#46410 * tls: support automatic DHE nodejs/node#46978 * fixup! src: add initial support for single executable applications * http: unify header treatment nodejs/node#46528 * fix: libc++ buffer overflow in string_view ctor nodejs/node#46410 * test: include strace openat test nodejs/node#46150 * fixup! fixup! src: add initial support for single executable applications --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes

@addaleaxaddaleaxaddaleax approved these changes

@lpincalpincalpinca approved these changes

@tniessentniessentniessen approved these changes

@JungMinuJungMinuJungMinu approved these changes

@RafaelGSSRafaelGSSRafaelGSS approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.c++Issues and PRs that require attention from people who are familiar with C++.inspectorIssues and PRs related to the V8 inspector protocolneeds-ciPRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@anonrig@nodejs-github-bot@lpinca@mscdex@jasnell@addaleax@tniessen@JungMinu@RafaelGSS