Skip to content

Commit 9dd90cc

Browse files
szuendRafaelGSS
authored andcommitted
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 <[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]>
1 parent adaed3a commit 9dd90cc

File tree

3 files changed

+32
-4
lines changed

3 files changed

+32
-4
lines changed

src/inspector_js_api.cc

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ using v8::HandleScope;
2323
using v8::Isolate;
2424
using v8::Local;
2525
using v8::MaybeLocal;
26-
using v8::NewStringType;
2726
using v8::Object;
2827
using v8::String;
2928
using v8::Uint32;
@@ -69,9 +68,8 @@ class JSBindingsConnection : public BaseObject {
6968
HandleScope handle_scope(isolate);
7069
Context::Scope context_scope(env_->context());
7170
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;
7573
connection_->OnMessage(argument);
7674
}
7775

src/util-inl.h

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -343,6 +343,32 @@ v8::MaybeLocal<v8::Value> ToV8Value(v8::Local<v8::Context> context,
343343
.FromMaybe(v8::Local<v8::String>());
344344
}
345345

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+
346372
template <typename T>
347373
v8::MaybeLocal<v8::Value> ToV8Value(v8::Local<v8::Context> context,
348374
const std::vector<T>& vec,

src/util.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
2626

2727
#include "uv.h"
28+
#include "v8-inspector.h"
2829
#include "v8.h"
2930

3031
#include "node.h"
@@ -720,6 +721,9 @@ std::vector<std::string_view> SplitString(const std::string_view in,
720721
inline v8::MaybeLocal<v8::Value> ToV8Value(v8::Local<v8::Context> context,
721722
std::string_view str,
722723
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);
723727
template <typename T, typename test_for_number =
724728
typename std::enable_if<std::numeric_limits<T>::is_specialized, bool>::type>
725729
inline v8::MaybeLocal<v8::Value> ToV8Value(v8::Local<v8::Context> context,

0 commit comments

Comments
 (0)