Commit 70bb387
src: fix inefficient usage of v8_inspector::StringView
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>1 parent d767cbf commit 70bb387
3 files changed
Lines changed: 32 additions & 4 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -23,7 +23,6 @@ using v8::HandleScope; | |||
| 23 | 23 | using v8::Isolate; | |
| 24 | 24 | using v8::Local; | |
| 25 | 25 | using v8::MaybeLocal; | |
| 26 | - using v8::NewStringType; | ||
| 27 | 26 | using v8::Object; | |
| 28 | 27 | using v8::String; | |
| 29 | 28 | using v8::Uint32; | |
@@ -69,9 +68,8 @@ class JSBindingsConnection : public BaseObject { | |||
| 69 | 68 | HandleScope handle_scope(isolate); | |
| 70 | 69 | Context::Scope context_scope(env_->context()); | |
| 71 | 70 | Local<Value> argument; | |
| 72 | - if (!String::NewFromTwoByte(isolate, message.characters16(), | ||
| 73 | - NewStringType::kNormal, | ||
| 74 | - message.length()).ToLocal(&argument)) return; | ||
| 71 | + if (!ToV8Value(env_->context(), message, isolate).ToLocal(&argument)) | ||
| 72 | + return; | ||
| 75 | 73 | connection_->OnMessage(argument); | |
| 76 | 74 | } | |
| 77 | 75 | ||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -343,6 +343,32 @@ v8::MaybeLocal<v8::Value> ToV8Value(v8::Local<v8::Context> context, | |||
| 343 | 343 | .FromMaybe(v8::Local<v8::String>()); | |
| 344 | 344 | } | |
| 345 | 345 | ||
| 346 | + v8::MaybeLocal<v8::Value> ToV8Value(v8::Local<v8::Context> context, | ||
| 347 | + v8_inspector::StringView str, | ||
| 348 | + v8::Isolate* isolate) { | ||
| 349 | + if (isolate == nullptr) isolate = context->GetIsolate(); | ||
| 350 | + if (str.length() >= static_cast<size_t>(v8::String::kMaxLength)) | ||
| 351 | + [[unlikely]] { | ||
| 352 | + // V8 only has a TODO comment about adding an exception when the maximum | ||
| 353 | + // string size is exceeded. | ||
| 354 | + ThrowErrStringTooLong(isolate); | ||
| 355 | + return v8::MaybeLocal<v8::Value>(); | ||
| 356 | + } | ||
| 357 | + | ||
| 358 | + if (str.is8Bit()) { | ||
| 359 | + return v8::String::NewFromOneByte(isolate, | ||
| 360 | + str.characters8(), | ||
| 361 | + v8::NewStringType::kNormal, | ||
| 362 | + str.length()) | ||
| 363 | + .FromMaybe(v8::Local<v8::String>()); | ||
| 364 | + } | ||
| 365 | + return v8::String::NewFromTwoByte(isolate, | ||
| 366 | + str.characters16(), | ||
| 367 | + v8::NewStringType::kNormal, | ||
| 368 | + str.length()) | ||
| 369 | + .FromMaybe(v8::Local<v8::String>()); | ||
| 370 | + } | ||
| 371 | + | ||
| 346 | 372 | template <typename T> | |
| 347 | 373 | v8::MaybeLocal<v8::Value> ToV8Value(v8::Local<v8::Context> context, | |
| 348 | 374 | const std::vector<T>& vec, | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -25,6 +25,7 @@ | |||
| 25 | 25 | #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS | |
| 26 | 26 | ||
| 27 | 27 | #include "uv.h" | |
| 28 | + #include "v8-inspector.h" | ||
| 28 | 29 | #include "v8.h" | |
| 29 | 30 | ||
| 30 | 31 | #include "node.h" | |
@@ -720,6 +721,9 @@ std::vector<std::string_view> SplitString(const std::string_view in, | |||
| 720 | 721 | inline v8::MaybeLocal<v8::Value> ToV8Value(v8::Local<v8::Context> context, | |
| 721 | 722 | std::string_view str, | |
| 722 | 723 | v8::Isolate* isolate = nullptr); | |
| 724 | + inline v8::MaybeLocal<v8::Value> ToV8Value(v8::Local<v8::Context> context, | ||
| 725 | + v8_inspector::StringView str, | ||
| 726 | + v8::Isolate* isolate); | ||
| 723 | 727 | template <typename T, typename test_for_number = | |
| 724 | 728 | typename std::enable_if<std::numeric_limits<T>::is_specialized, bool>::type> | |
| 725 | 729 | inline v8::MaybeLocal<v8::Value> ToV8Value(v8::Local<v8::Context> context, | |
0 commit comments