-
-
Notifications
You must be signed in to change notification settings - Fork 35.1k
Conversation
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)
ReplacesIsAsciiOneByteString+memcpywith HighwayWriteLeadingAsciiinUtf8::Encode, providing a faster ASCII fast path forWriteUtf8V2.
CL: https://chromium-review.googlesource.com/c/v8/v8/+/7184338 -
ee2873a6303d-- Fix GCC Build (Abdirahim Musse)
MovesWriteLeadingAsciiexplicit template specializations to namespace scope -- C++ forbids them inside class scope, and GCC rejects it.
CL: https://chromium-review.googlesource.com/c/v8/v8/+/7138905Note: only the
unicode.hportion 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 theWriteLeadingAsciifix.
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
|
Review requested:
|
There was a problem hiding this comment.
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.
9acf475 to
eb0d45c
Compare
eb0d45c to
a215fe8
Compare
883bf5e to
ad11612
Compare
|
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
left a comment
There was a problem hiding this comment.
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?
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. |
|
It's the right approach. |
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
> Reviewed-by: Toon Verwaest
> Commit-Queue: Dan Carney
> 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
Reviewed-by: Patrick Thier
Commit-Queue: Dan Carney
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
IsAsciiOneByteString
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
std::pair
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
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
Reviewed-by: Jakob Kummerow
Reviewed-by: Clemens Backes
Commit-Queue: Jakob Kummerow
Cr-Commit-Position: refs/heads/main@{#104280}
Refs: v8/v8@67507b2
Refs: v8/v8@ee2873a
ad11612 to
f9dd983
Compare
|
@Renegade334 is there anything pending from my side that allow this to be merged? |