From b144ac0ceecf2b94e07c54f5056c39255d5fe0bc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Simon=20Z=C3=BCnd?= <szuend@chromium.org>
Date: Fri, 5 Apr 2024 05:19:38 +0000
Subject: [PATCH] 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.
---
 src/inspector_js_api.cc |  6 ++----
 src/util-inl.h          | 26 ++++++++++++++++++++++++++
 src/util.h              |  4 ++++
 3 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/src/inspector_js_api.cc b/src/inspector_js_api.cc
index de771a30042c0e..69029247accf5b 100644
--- a/src/inspector_js_api.cc
+++ b/src/inspector_js_api.cc
@@ -23,7 +23,6 @@ using v8::HandleScope;
 using v8::Isolate;
 using v8::Local;
 using v8::MaybeLocal;
-using v8::NewStringType;
 using v8::Object;
 using v8::String;
 using v8::Uint32;
@@ -69,9 +68,8 @@ 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 (!ToV8Value(env_->context(), message, isolate).ToLocal(&argument))
+        return;
       connection_->OnMessage(argument);
     }
 
diff --git a/src/util-inl.h b/src/util-inl.h
index a63ed8328e29a2..d400d913be70f6 100644
--- a/src/util-inl.h
+++ b/src/util-inl.h
@@ -344,6 +344,32 @@ v8::MaybeLocal<v8::Value> ToV8Value(v8::Local<v8::Context> context,
       .FromMaybe(v8::Local<v8::String>());
 }
 
+v8::MaybeLocal<v8::Value> ToV8Value(v8::Local<v8::Context> context,
+                                    v8_inspector::StringView str,
+                                    v8::Isolate* isolate) {
+  if (isolate == nullptr) isolate = context->GetIsolate();
+  if (str.length() >= static_cast<size_t>(v8::String::kMaxLength))
+      [[unlikely]] {
+    // V8 only has a TODO comment about adding an exception when the maximum
+    // string size is exceeded.
+    ThrowErrStringTooLong(isolate);
+    return v8::MaybeLocal<v8::Value>();
+  }
+
+  if (str.is8Bit()) {
+    return v8::String::NewFromOneByte(isolate,
+                                      str.characters8(),
+                                      v8::NewStringType::kNormal,
+                                      str.length())
+        .FromMaybe(v8::Local<v8::String>());
+  }
+  return v8::String::NewFromTwoByte(isolate,
+                                    str.characters16(),
+                                    v8::NewStringType::kNormal,
+                                    str.length())
+      .FromMaybe(v8::Local<v8::String>());
+}
+
 template <typename T>
 v8::MaybeLocal<v8::Value> ToV8Value(v8::Local<v8::Context> context,
                                     const std::vector<T>& vec,
diff --git a/src/util.h b/src/util.h
index 6b414a79bbb613..a37293fa1e90ca 100644
--- a/src/util.h
+++ b/src/util.h
@@ -25,6 +25,7 @@
 #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
 
 #include "uv.h"
+#include "v8-inspector.h"
 #include "v8.h"
 
 #include "node.h"
@@ -719,6 +720,9 @@ inline v8::Maybe<void> FromV8Array(v8::Local<v8::Context> context,
 inline v8::MaybeLocal<v8::Value> ToV8Value(v8::Local<v8::Context> context,
                                            std::string_view str,
                                            v8::Isolate* isolate = nullptr);
+inline v8::MaybeLocal<v8::Value> ToV8Value(v8::Local<v8::Context> context,
+                                           v8_inspector::StringView str,
+                                           v8::Isolate* isolate);
 template <typename T, typename test_for_number =
     typename std::enable_if<std::numeric_limits<T>::is_specialized, bool>::type>
 inline v8::MaybeLocal<v8::Value> ToV8Value(v8::Local<v8::Context> context,