Skip to content

deps: V8: cherry-pick 67507b2a88f4 - #61825

Closed
rmagrin wants to merge 1 commit into
nodejs:mainfrom
rmagrin:v8-backport-highway-ascii
Closed

deps: V8: cherry-pick 67507b2a88f4#61825
rmagrin wants to merge 1 commit into
nodejs:mainfrom
rmagrin:v8-backport-highway-ascii

Conversation

@rmagrin

@rmagrinrmagrin commented Feb 14, 2026•
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Cherry-picks two V8 commits (squashed) to add Highway-based WriteLeadingAscii and fix its GCC build:

  • 67507b2a88f4 — Reland "use highway to check and copy leading ascii" (Dan Carney)
    Replaces IsAsciiOneByteString + memcpy with Highway WriteLeadingAscii in Utf8::Encode, providing a faster ASCII fast path for WriteUtf8V2.
    CL: https://chromium-review.googlesource.com/c/v8/v8/+/7184338

  • ee2873a6303d — Fix GCC Build (Abdirahim Musse)
    Moves WriteLeadingAscii explicit template specializations to namespace scope — C++ forbids them inside class scope, and GCC rejects it.
    CL: https://chromium-review.googlesource.com/c/v8/v8/+/7138905

    Note: only the unicode.h portion of this commit is included; the test file changes (heap-unittest.cc, module-decoder-unittest.cc) do not apply cleanly to Node.js's V8 copy and are unrelated to the WriteLeadingAscii fix.

This patch addresses part of the remaining ~30-40% performance gap in WriteUtf8V2 vs v22 after #61712 landed the initial simdutf + memcpy fast path.

Refs: #60719

Test plan

  • CI: V8 CI + Node.js CI

@nodejs-github-bot

nodejs-github-bot commented Feb 14, 2026

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/security-wg
  • @nodejs/v8-update

@nodejs-github-botnodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency. labels Feb 14, 2026
Renegade334
Renegade334 previously requested changes Feb 15, 2026
View reviewed changes

@Renegade334Renegade334 left a comment •
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.

This needs the unicode.h aspects of v8/v8@ee2873a to unblock gcc builds.

@rmagrin
rmagrinforce-pushed the v8-backport-highway-ascii branch from 9acf475 to eb0d45cCompareFebruary 15, 2026 16:23
@rmagrinrmagrin changed the title deps: V8: cherry-pick Highway string performance patchesdeps: V8: cherry-pick 67507b2a88f4Feb 15, 2026
@rmagrin
rmagrinforce-pushed the v8-backport-highway-ascii branch from eb0d45c to a215fe8CompareFebruary 15, 2026 16:48
@rmagrin
rmagrinforce-pushed the v8-backport-highway-ascii branch 2 times, most recently from 883bf5e to ad11612CompareFebruary 17, 2026 05:15
@rmagrin

rmagrin commented Feb 17, 2026•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

I decide to keep only one backport on this PR and the Fix GCC Build (ee2873a6303d).

I will open another PR once this is merged with the backport for https://chromium-review.googlesource.com/c/v8/v8/+/7159233

Renegade334
Renegade334 reviewed Feb 17, 2026
View reviewed changes

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

I believe this violates the commit-rebase policy, as each individual commit needs to (in principle) independently pass CI, which isn't the case here. I would just edit the backport commit to include the small change to the header, don't know what you reckon @targos?

@rmagrin

rmagrin commented Feb 18, 2026

Copy link
Copy Markdown
ContributorAuthor

I believe this violates the commit-rebase policy, as each individual commit needs to (in principle) independently pass CI, which isn't the case here. I would just edit the backport commit to include the small change to the header, don't know what you reckon @targos?

I missed that requirement. Happy to fix the PR to have a single commit. I just need a confirmation that it is the right approach for this case.

@targos

targos commented Feb 18, 2026

Copy link
Copy Markdown
Member

It's the right approach. git node v8 backport has a flag (--squash) to generate a single commit: https://github.com/nodejs/node-core-utils/blob/main/docs/git-node.md#git-node-v8-backport-sha

@rmagrin
deps: V8: cherry-pick 67507b2a88f4, ee2873a6303d
f9dd983
Original commit messages:
Reland "use highway to check and copy leading ascii"
This is a reland of commit a3e84e5f01540cec142f4d4f41f1921373c220e5
Original change's description:
> use highway to check and copy leading ascii
>
> Change-Id: I065532aeeee95273821aa1f25b5ffc5c5c23cbf1
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7172479
> Reviewed-by: Patrick Thier <pthier@chromium.org>
> Reviewed-by: Toon Verwaest <verwaest@chromium.org>
> Commit-Queue: Dan Carney <dcarney@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#103820}
Change-Id: I43b4ad18817eb52b701e112d2d0a5f685374ae1f
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7184338
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Reviewed-by: Patrick Thier <pthier@chromium.org>
Commit-Queue: Dan Carney <dcarney@chromium.org>
Cr-Commit-Position: refs/heads/main@{#103865}
Fix GCC Build
Move explicit specializations of Utf8::IsAsciiOneByteString to namespace
scope
C++ forbids explicit template specializations inside class scope.
The specializations for IsAsciiOneByteString<uint8_t> and
IsAsciiOneByteString<uint16_t> were defined inside class unibrow::Utf8,
riggering "explicit specialization in non-namespace scope"
compile errors.
Fix type mismatch in EXPECT_EQ comparisons for std::pair values
The tests failed because EXPECT_EQ was comparing pairs with mismatched
template parameters — std::pair<int, unsigned int> vs.
std::pair<unsigned int, unsigned char> — which have no valid operator==.
I corrected the expected values to use the same unsigned types as the
actual data, ensuring the pair comparison compiles cleanly.
Fix build failure on GCC 12 by replacing std::format with ostringstream
GCC 12's libstdc++ does not implement <format>, causing compile errors
when using std::format. Replaced all std::format calls with equivalent
Change-Id: I5c31f91065eccf6f4c14172902ffcd99863ebbb9
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7138905
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Reviewed-by: Clemens Backes <clemensb@chromium.org>
Commit-Queue: Jakob Kummerow <jkummerow@chromium.org>
Cr-Commit-Position: refs/heads/main@{#104280}
Refs: v8/v8@67507b2
Refs: v8/v8@ee2873a
@rmagrin
rmagrinforce-pushed the v8-backport-highway-ascii branch from ad11612 to f9dd983CompareFebruary 19, 2026 04:16
@targostargos added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 19, 2026
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 19, 2026
@nodejs-github-bot

nodejs-github-bot commented Feb 19, 2026

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Feb 19, 2026

Copy link
Copy Markdown
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-power9le,v8test=v8test/7027/

@nodejs-github-bot

nodejs-github-bot commented Feb 19, 2026

Copy link
Copy Markdown
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/7027/

@nodejs-github-bot

nodejs-github-bot commented Feb 19, 2026

Copy link
Copy Markdown
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/7027/

@rmagrin

rmagrin commented Mar 2, 2026

Copy link
Copy Markdown
ContributorAuthor

@Renegade334 is there anything pending from my side that allow this to be merged?

@rmagrin

rmagrin commented Apr 15, 2026

Copy link
Copy Markdown
ContributorAuthor

Closing since this was never merged and it has conflicts now.

@rmagrinrmagrin closed this Apr 15, 2026
@rmagrin
rmagrin deleted the v8-backport-highway-ascii branch April 15, 2026 17:20
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@targostargostargos approved these changes
@Renegade334Renegade334Renegade334 approved these changes

Assignees

No one assigned

Labels

buildIssues and PRs related to build files or the CI.needs-ciPRs that need a full CI run.v8 engineIssues and PRs related to the V8 dependency.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@rmagrin@nodejs-github-bot@targos@Renegade334