Skip to content
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

src: replace icu with simdutf for char counts #46472

Closed
wants to merge 1 commit into from
Closed

src: replace icu with simdutf for char counts #46472

wants to merge 1 commit into from

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Feb 2, 2023

No description provided.

@nodejs-github-bot nodejs-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
@anonrig anonrig requested a review from addaleax February 2, 2023 18:15
@anonrig anonrig 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
Member Author

anonrig commented Feb 2, 2023

cc @nodejs/cpp-reviewers

@github-actions github-actions bot 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

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
Member Author

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)

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

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

src/inspector/node_string.cc Show resolved Hide resolved
@anonrig anonrig requested a review from addaleax February 2, 2023 21:35
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 3, 2023
@github-actions github-actions bot 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

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

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

anonrig commented Feb 3, 2023

@lpinca @addaleax Can you review it again?

@anonrig anonrig 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
Member Author

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

@tniessen tniessen 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
@anonrig anonrig 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 <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
@anonrig
Copy link
Member Author

anonrig commented Feb 6, 2023

Landed in 6a60ed8

@anonrig anonrig closed this Feb 6, 2023
@anonrig anonrig deleted the faster-string branch February 6, 2023 13:41
@tniessen tniessen changed the title src: provide faster string character counts src: replace icu with simdutf for char counts Feb 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 <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
@MylesBorins MylesBorins 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
@StefanStojanovic StefanStojanovic 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 <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
@danielleadams danielleadams 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 <shelley.vohr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RafaelGSS RafaelGSS RafaelGSS approved these changes

@JungMinu JungMinu JungMinu approved these changes

@tniessen tniessen tniessen approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs 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++. inspector Issues and PRs related to the V8 inspector protocol 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.
9 participants
@anonrig @nodejs-github-bot @lpinca @mscdex @jasnell @addaleax @tniessen @JungMinu @RafaelGSS