src: replace icu with simdutf for char counts by anonrig · Pull Request #46472 · nodejs/node · GitHub

No description provided.

cc @nodejs/cpp-reviewers

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

Out of curiosity, what is the performance gain?

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)

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

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

@lpinca @addaleax Can you review it again?

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 ?

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

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>

Landed in 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>

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

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>

- nodejs/node#46471 - nodejs/node#46472

* 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>

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 requested a review from addaleax February 2, 2023 18:15

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

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 2, 2023

lpinca approved these changes Feb 2, 2023

View reviewed changes

anonrig added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 2, 2023

addaleax reviewed Feb 2, 2023

View reviewed changes

Comment thread src/inspector/node_string.cc Outdated

anonrig force-pushed the faster-string branch from e24eb3f to 58ce8e9 Compare February 2, 2023 21:35

anonrig requested a review from addaleax February 2, 2023 21:35

anonrig force-pushed the faster-string branch from 58ce8e9 to 43cb7a1 Compare February 2, 2023 21:37

src: provide faster string character counts

1a90cbb

anonrig force-pushed the faster-string branch from 43cb7a1 to 1a90cbb Compare February 2, 2023 21:42

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 3, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 3, 2023

This comment was marked as outdated.

Sign in to view

anonrig added the review wanted PRs that need reviews. label Feb 3, 2023

addaleax approved these changes Feb 3, 2023

View reviewed changes

tniessen approved these changes Feb 3, 2023

View reviewed changes

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

JungMinu approved these changes Feb 4, 2023

View reviewed changes

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 removed the performance Issues and PRs related to the performance of Node.js. label Feb 4, 2023

jasnell approved these changes Feb 4, 2023

View reviewed changes

RafaelGSS approved these changes Feb 4, 2023

View reviewed changes

This was referenced Feb 5, 2023

CI Reliability 2023-02-05 nodejs/reliability#495

Open

CI Reliability 2023-02-06 nodejs/reliability#497

Open

anonrig closed this Feb 6, 2023

anonrig deleted the faster-string branch February 6, 2023 13:41

tniessen changed the title src: provide faster string character counts src: replace icu with simdutf for char counts Feb 6, 2023

MylesBorins mentioned this pull request Feb 19, 2023

v19.7.0 proposal #46725

Merged

StefanStojanovic mentioned this pull request Feb 22, 2023

win,src: revert simdutf to icu for win arm64 #46780

Closed

danielleadams mentioned this pull request Apr 11, 2023

v18.16.0 release proposal #47502

Merged

codebytere added a commit to electron/electron that referenced this pull request Apr 15, 2023

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

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

fix: add simdutf dep to src/inspector BUILD.gn

8096620

- nodejs/node#46471 - nodejs/node#46472

src: replace icu with simdutf for char counts by anonrig · Pull Request #46472 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: replace icu with simdutf for char counts - #46472

src: replace icu with simdutf for char counts#46472
anonrig wants to merge 1 commit into
nodejs:mainfrom
anonrig:faster-string

anonrig commented Feb 2, 2023

Uh oh!

anonrig commented Feb 2, 2023

Uh oh!

nodejs-github-bot commented Feb 2, 2023

Uh oh!

lpinca commented Feb 2, 2023

Uh oh!

anonrig commented Feb 2, 2023

Uh oh!

addaleax left a comment

Uh oh!

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Feb 3, 2023

Uh oh!

anonrig commented Feb 3, 2023

Uh oh!

mscdex commented Feb 4, 2023

Uh oh!

anonrig commented Feb 4, 2023

Uh oh!

anonrig commented Feb 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

src: replace icu with simdutf for char counts by anonrig · Pull Request #46472 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: replace icu with simdutf for char counts - #46472

src: replace icu with simdutf for char counts#46472anonrig wants to merge 1 commit intonodejs:mainnodejs/node:mainfrom anonrig:faster-stringanonrig/node:faster-stringCopy head branch name to clipboard

Conversation

anonrig commented Feb 2, 2023

Uh oh!

anonrig commented Feb 2, 2023

Uh oh!

nodejs-github-bot commented Feb 2, 2023

Uh oh!

lpinca commented Feb 2, 2023

Uh oh!

anonrig commented Feb 2, 2023

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Feb 3, 2023

Uh oh!

anonrig commented Feb 3, 2023

Uh oh!

mscdex commented Feb 4, 2023

Uh oh!

anonrig commented Feb 4, 2023

Uh oh!

anonrig commented Feb 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

src: replace icu with simdutf for char counts#46472
anonrig wants to merge 1 commit into
nodejs:mainfrom
anonrig:faster-string