Skip to content

Commit

Permalink
src: Fix inefficient usage of v8_inspector::StringView
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
szuend committed Apr 5, 2024
1 parent 433bd1b commit 0d43fab
Show file tree
Hide file tree
Showing 4 changed files with 21 additions and 8 deletions.
4 changes: 1 addition & 3 deletions src/inspector_js_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,7 @@ class JSBindingsConnection : public BaseObject {
HandleScope handle_scope(isolate);
Context::Scope context_scope(env_->context());
Local<Value> argument;
if (!String::NewFromTwoByte(isolate, message.characters16(),
NewStringType::kNormal,
message.length()).ToLocal(&argument)) return;
if (!StringViewToV8String(isolate, message).ToLocal(&argument)) return;
connection_->OnMessage(argument);
}

Expand Down
6 changes: 1 addition & 5 deletions src/inspector_profiler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -89,11 +89,7 @@ void V8ProfilerConnection::V8ProfilerSessionDelegate::SendMessageToFrontend(
const char* type = connection_->type();
// Convert StringView to a Local<String>.
Local<String> message_str;
if (!String::NewFromTwoByte(isolate,
message.characters16(),
NewStringType::kNormal,
message.length())
.ToLocal(&message_str)) {
if (!StringViewToV8String(isolate, message).ToLocal(&message_str)) {
fprintf(
stderr, "Failed to convert %s profile message to V8 string\n", type);
return;
Expand Down
14 changes: 14 additions & 0 deletions src/util-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,20 @@ inline v8::Local<v8::String> OneByteString(v8::Isolate* isolate,
.ToLocalChecked();
}

v8::MaybeLocal<v8::String> StringViewToV8String(
v8::Isolate* isolate,
v8_inspector::StringView string) {
if (string.is8Bit()) {
return v8::String::NewFromOneByte(isolate,
string.characters8(),
v8::NewStringType::kNormal,
string.length());
}
return v8::String::NewFromTwoByte(isolate, string.characters16(),
v8::NewStringType::kNormal,
string.length());
}

void SwapBytes16(char* data, size_t nbytes) {
CHECK_EQ(nbytes % 2, 0);

Expand Down
5 changes: 5 additions & 0 deletions src/util.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

#include "uv.h"
#include "v8.h"
#include "v8-inspector.h"

#include "node.h"
#include "node_exit_code.h"
Expand Down Expand Up @@ -353,6 +354,10 @@ inline v8::Local<v8::String> FIXED_ONE_BYTE_STRING(
return OneByteString(isolate, arr.data(), N - 1);
}

// Convenience wrapper to handle both one- and two-byte inspector strings.
inline v8::MaybeLocal<v8::String> StringViewToV8String(
v8::Isolate* isolate,
v8_inspector::StringView string);


// Swaps bytes in place. nbytes is the number of bytes to swap and must be a
Expand Down

0 comments on commit 0d43fab

Please sign in to comment.