src: Fix inefficient usage of v8_inspector::StringView #52372
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nodejs-github-bot
added
c++
szuend
force-pushed
the
fix-inspector-string-view-usage
branch
from
0d43fab to
a4087c9
Compare
addaleax
approved these changes
Apr 5, 2024
addaleax
approved these changes
Apr 8, 2024
github-actions
bot
added
request-ci-failed
Failed to start CI- Validating Jenkins credentials ✘ Jenkins credentials invalidhttps://github.com./nodejs/node/actions/runs/8605010252 |
|
@addaleax Anything I need to address from my side for the failed CI run? |
panva
added
request-ci
addaleax
added
commit-queue
github-actions
bot
removed
the
request-ci
|
I'm not too familiar with the node CI. Are the failing tests from the two CI runs generally known to be flaky or should I investigate? |
github-actions
bot
removed
the
request-ci
szuend
force-pushed
the
fix-inspector-string-view-usage
branch
from
66345c0 to
f79c3cb
Compare
|
Rebased the PR after #51783 has landed. FYI @joyeecheung |
joyeecheung
approved these changes
Apr 22, 2024
github-actions
bot
removed
the
request-ci
github-actions
bot
removed
the
request-ci
JungMinu
approved these changes
Aug 8, 2024
|
@szuend could you please rebase the branch? We may not have merge commits for the CI and it has to run once more for this to land. I believe it's otherwise good to land. |
legendecas
reviewed
Apr 2, 2025
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #52372 +/- ##
==========================================
+ Coverage 90.23% 90.24% +0.01%
==========================================
Files 630 630
Lines 185017 185032 +15
Branches 36207 36222 +15
==========================================
+ Hits 166948 166985 +37
- Misses 11020 11029 +9
+ Partials 7049 7018 -31
🚀 New features to boost your workflow:
|
github-actions
bot
added
request-ci-failed
Failed to start CI⚠ Commits were pushed since the last approving review: ⚠ - src: fix inefficient usage of v8_inspector::StringView ⚠ - Apply linter ⚠ - Rename function to ToV8Value and mirror std::string_view overload ⚠ - Merge branch 'main' into fix-inspector-string-view-usage ⚠ - Fix usage of UNLIKELY ✘ Refusing to run CI on potentially unsafe PRhttps://github.com./nodejs/node/actions/runs/14238536161 |
legendecas
approved these changes
Apr 3, 2025
legendecas
added
request-ci
github-actions
bot
removed
the
request-ci
|
@szuend I think the CI failed to due merge commits. Would you mind rebase onto the tip of the main branch? thanks! |
v8_inspector::StringView can either be one-byte or two-byte strings. Node has currently two places where it's unconditionally assumed that it's a two-byte StringView. This requires the upstream V8 inspector to unnecessarily create a copy of the string: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/inspector/v8-inspector-session-impl.cc;l=192-199;drc=05bacd38e7a31e92afe0fd66081dfa2cc03b934a This is particularly slow, especially for large CDP messages, as the serialized JSON is iterated 8-bit char by 8-bit char and each one widened to a 16-bit char. This PR introduces a small helper that correctly converts a StringView to a v8::String instance honoring the "is8Bit" flag. This allows upstream V8 to remove the unnecessary widening copy.
szuend
force-pushed
the
fix-inspector-string-view-usage
branch
from
fe2ea75 to
b144ac0
Compare
|
@legendecas done! Thanks! |
legendecas
approved these changes
Apr 3, 2025
github-actions
bot
removed
the
request-ci
legendecas
added
the
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in 264d031 |
JonasBa
pushed a commit
to JonasBa/node
that referenced
this pull request
Apr 11, 2025
v8_inspector::StringView can either be one-byte or two-byte strings. Node has currently two places where it's unconditionally assumed that it's a two-byte StringView. This requires the upstream V8 inspector to unnecessarily create a copy of the string: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/inspector/v8-inspector-session-impl.cc;l=192-199;drc=05bacd38e7a31e92afe0fd66081dfa2cc03b934a This is particularly slow, especially for large CDP messages, as the serialized JSON is iterated 8-bit char by 8-bit char and each one widened to a 16-bit char. This PR introduces a small helper that correctly converts a StringView to a v8::String instance honoring the "is8Bit" flag. This allows upstream V8 to remove the unnecessary widening copy. PR-URL: nodejs#52372 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
v8_inspector::StringView can either be one-byte or two-byte strings. Node has currently two places where it's unconditionally assumed that it's a two-byte StringView.
This requires the upstream V8 inspector to unnecessarily create a copy of the string:
https://source.chromium.org/chromium/chromium/src/+/main:v8/src/inspector/v8-inspector-session-impl.cc;l=192-199;drc=05bacd38e7a31e92afe0fd66081dfa2cc03b934a
This is particularly slow, especially for large CDP messages, as the serialized JSON is iterated 8-bit char by 8-bit char and each one widened to a 16-bit char.
This PR introduces a small helper that correctly converts a StringView to a v8::String instance honoring the "is8Bit" flag. This allows upstream V8 to remove the unnecessary widening copy.
Tested locally with removing the widening copy in v8-inspector-session-impl.cc: Without the PR lots of tests will fail. But I'm happy to add unit tests for the
JSBindingsConnection/V8ProfilerConnectionif desired.