src: Fix inefficient usage of v8_inspector::StringView#52372
Merged
Uh oh!
There was an error while loading. Please reload this page.
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++ 
szuendforce-pushed the fix-inspector-string-view-usage branch from 0d43fab to a4087c9Compare
addaleax approved these changes Apr 5, 2024
Uh oh!
There was an error while loading. Please reload this page.
addaleax approved these changes Apr 8, 2024
github-actionsbot added request-ci-failed 
Contributor
Failed to start CI- Validating Jenkins credentials ✘ Jenkins credentials invalidhttps://github.com/nodejs/node/actions/runs/8605010252 |
ContributorAuthor
szuend commented Apr 9, 2024
@addaleax Anything I need to address from my side for the failed CI run? |
panva added request-ci 
addaleax added commit-queue github-actionsbot removed the request-ci 
Collaborator
nodejs-github-bot commented Apr 10, 2024
Collaborator
nodejs-github-bot commented Apr 11, 2024
ContributorAuthor
szuend commented Apr 15, 2024
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? |
Collaborator
nodejs-github-bot commented Apr 15, 2024
github-actionsbot removed the request-ci Collaborator
nodejs-github-bot commented Apr 16, 2024
szuendforce-pushed the fix-inspector-string-view-usage branch from 66345c0 to f79c3cbCompareContributorAuthor
szuend commented Apr 22, 2024
Rebased the PR after #51783 has landed. FYI @joyeecheung |
joyeecheung approved these changes Apr 22, 2024
github-actionsbot removed the request-ci Collaborator
nodejs-github-bot commented Apr 22, 2024
Collaborator
nodejs-github-bot commented Apr 23, 2024
Contributor
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-actionsbot removed the request-ci Collaborator
nodejs-github-bot commented Apr 3, 2025
Member
@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.
szuendforce-pushed the fix-inspector-string-view-usage branch from fe2ea75 to b144ac0CompareContributorAuthor
szuend commented Apr 3, 2025
@legendecas done! Thanks! |
legendecas approved these changes Apr 3, 2025
github-actionsbot removed the request-ci Collaborator
nodejs-github-bot commented Apr 3, 2025
legendecas added the commit-queue nodejs-github-bot removed the commit-queue nodejs-github-bot merged commit 264d031 into nodejs:main 59 checks passed
Uh oh!
There was an error while loading. Please reload this page.
Collaborator
nodejs-github-bot commented Apr 3, 2025
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]>
RafaelGSS pushed a commit that referenced this pull request May 1, 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: #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]>
RafaelGSS pushed a commit that referenced this pull request May 2, 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: #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]>
aduh95 pushed a commit that referenced this pull request May 6, 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: #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]>
RafaelGSS pushed a commit that referenced this pull request May 14, 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: #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]>
aduh95 pushed a commit that referenced this pull request May 17, 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: #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]>
aduh95 pushed a commit that referenced this pull request May 17, 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: #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]>
aduh95 pushed a commit that referenced this pull request May 19, 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: #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]>
ghost mentioned this pull request hubot pushed a commit to v8/v8 that referenced this pull request Jul 11, 2025
Upstream Node.js was missing a check for 'is8Bit' making it necessary to always use 16-bit wide chars when serializing CDP messages. We fixed this upstream (nodejs/node#52372) so now we can remove the widening copy. Bug: None Change-Id: I1f0b15e0ece7c0987dfcbeeaf913e9d76cda8ba8 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6478571 Reviewed-by: Eric Leese <[email protected]> Commit-Queue: Simon Zünd <[email protected]> Cr-Commit-Position: refs/heads/main@{#101360}
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
10 participants
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.