src: Fix inefficient usage of v8_inspector::StringView by szuend · Pull Request #52372 · nodejs/node

szuend · 2024-04-05T05:40:48Z

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/V8ProfilerConnection if desired.

src/util-inl.h

github-actions · 2024-04-08T18:35:28Z

Failed to start CI

- Validating Jenkins credentials
✘  Jenkins credentials invalid

https://github.com/nodejs/node/actions/runs/8605010252

szuend · 2024-04-09T04:14:59Z

@addaleax Anything I need to address from my side for the failed CI run?

nodejs-github-bot · 2024-04-10T18:51:18Z

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

nodejs-github-bot · 2024-04-11T09:42:42Z

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

szuend · 2024-04-15T05:17:53Z

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?

nodejs-github-bot · 2024-04-15T14:06:14Z

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

nodejs-github-bot · 2024-04-16T12:17:26Z

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

szuend · 2024-04-22T07:32:48Z

Rebased the PR after #51783 has landed.

FYI @joyeecheung

nodejs-github-bot · 2024-04-22T12:59:53Z

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

nodejs-github-bot · 2024-04-23T14:44:37Z

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

github-actions · 2025-04-03T08:33:41Z

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 PR

https://github.com/nodejs/node/actions/runs/14238536161

nodejs-github-bot · 2025-04-03T09:12:58Z

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

legendecas · 2025-04-03T09:40:26Z

@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 · 2025-04-03T10:31:06Z

@legendecas done! Thanks!

nodejs-github-bot · 2025-04-03T13:03:13Z

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

nodejs-github-bot · 2025-04-03T19:38:29Z

Landed in 264d031

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 <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

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 <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

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 <leese@chromium.org> Commit-Queue: Simon Zünd <szuend@chromium.org> Cr-Commit-Position: refs/heads/main@{#101360}

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Apr 5, 2024

szuend force-pushed the fix-inspector-string-view-usage branch from 0d43fab to a4087c9 Compare April 5, 2024 05:43

addaleax approved these changes Apr 5, 2024

View reviewed changes

src/util-inl.h Outdated Show resolved Hide resolved

addaleax approved these changes Apr 8, 2024

View reviewed changes

addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 8, 2024

github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Apr 8, 2024

panva added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Apr 9, 2024

addaleax added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Apr 10, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 10, 2024

cola119 added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 16, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 16, 2024

szuend force-pushed the fix-inspector-string-view-usage branch from 66345c0 to f79c3cb Compare April 22, 2024 07:31

joyeecheung approved these changes Apr 22, 2024

View reviewed changes

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 22, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 22, 2024

legendecas approved these changes Apr 3, 2025

View reviewed changes

legendecas added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Apr 3, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 3, 2025

szuend force-pushed the fix-inspector-string-view-usage branch from fe2ea75 to b144ac0 Compare April 3, 2025 10:26

legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 3, 2025

legendecas approved these changes Apr 3, 2025

View reviewed changes

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 3, 2025

legendecas added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 3, 2025

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 3, 2025

nodejs-github-bot merged commit 264d031 into nodejs:main Apr 3, 2025
59 checks passed

RafaelGSS mentioned this pull request May 1, 2025

2025-05-06, Version 24.0.0 (Current) #57609

Merged

github-actions bot mentioned this pull request May 19, 2025

2025-05-21, Version 22.16.0 'Jod' (LTS) #58388

Merged

ghost mentioned this pull request Jun 8, 2025

### Notable Changes autofix-ci/action#28

Closed

Uh oh!

Conversation

szuend commented Apr 5, 2024

Uh oh!

Uh oh!

github-actions bot commented Apr 8, 2024

Uh oh!

szuend commented Apr 9, 2024

Uh oh!

nodejs-github-bot commented Apr 10, 2024

Uh oh!

nodejs-github-bot commented Apr 11, 2024

Uh oh!

szuend commented Apr 15, 2024

Uh oh!

nodejs-github-bot commented Apr 15, 2024

Uh oh!

nodejs-github-bot commented Apr 16, 2024

Uh oh!

szuend commented Apr 22, 2024

Uh oh!

nodejs-github-bot commented Apr 22, 2024

Uh oh!

nodejs-github-bot commented Apr 23, 2024

Uh oh!

github-actions bot commented Apr 3, 2025

Uh oh!

nodejs-github-bot commented Apr 3, 2025

Uh oh!

legendecas commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szuend commented Apr 3, 2025

Uh oh!

nodejs-github-bot commented Apr 3, 2025

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

legendecas commented Apr 3, 2025 •

edited

Loading