From 0492f0f7455ba10775b278be15c3f7221ac46ea8 Mon Sep 17 00:00:00 2001
From: Charles Kerr <charles@charleskerr.com>
Date: Tue, 1 Apr 2025 12:25:27 -0500
Subject: [PATCH] perf: have `ErrorThrower` lazily lookup the current isolate
 (#46388)

perf: have ErrorThrower lazy-lookup the current isolate

ErrorThrower's default constructor is marked as "should rarely if ever
be used" because it's expensive to call.

Unfortunately, nearly every instance of ErrorThrower comes as an argument
in gin_helper's JS-->C++ function marshalling where a thrower is
default-constructed and then populated in gin_helper::GetNextArgument()
with an assignment operator to a temporary ErrorThrower constructed
with the gin::Arguments' isolate.

tldr: most of the time we use the slow constructor first, then throw
that work away unused by overwriting with a fast-constructed one.

This refactor avoids that cost by deferring the expensive work to
`ErrorThrower::isolate()`, where it happens only as a fallback iff
isolate_ hasn't been set.
---
 shell/common/gin_helper/error_thrower.cc | 18 +++++++++---------
 shell/common/gin_helper/error_thrower.h  |  8 ++++----
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/shell/common/gin_helper/error_thrower.cc b/shell/common/gin_helper/error_thrower.cc
index 0dbfce5cdf81..842143cc1cb7 100644
--- a/shell/common/gin_helper/error_thrower.cc
+++ b/shell/common/gin_helper/error_thrower.cc
@@ -10,12 +10,11 @@
 
 namespace gin_helper {
 
-ErrorThrower::ErrorThrower(v8::Isolate* isolate) : isolate_(isolate) {}
-
-// This constructor should be rarely if ever used, since
-// v8::Isolate::GetCurrent() uses atomic loads and is thus a bit
-// costly to invoke
-ErrorThrower::ErrorThrower() : isolate_(v8::Isolate::GetCurrent()) {}
+v8::Isolate* ErrorThrower::isolate() const {
+  // Callers should prefer to specify the isolate in the constructor,
+  // since GetCurrent() uses atomic loads and is thus a bit costly to invoke
+  return isolate_ ? isolate_.get() : v8::Isolate::GetCurrent();
+}
 
 void ErrorThrower::ThrowError(const std::string_view err_msg) const {
   Throw(v8::Exception::Error, err_msg);
@@ -39,9 +38,10 @@ void ErrorThrower::ThrowSyntaxError(const std::string_view err_msg) const {
 
 void ErrorThrower::Throw(ErrorGenerator gen,
                          const std::string_view err_msg) const {
-  v8::Local<v8::Value> exception = gen(gin::StringToV8(isolate_, err_msg), {});
-  if (!isolate_->IsExecutionTerminating())
-    isolate_->ThrowException(exception);
+  v8::Isolate* isolate = this->isolate();
+
+  if (!isolate->IsExecutionTerminating())
+    isolate->ThrowException(gen(gin::StringToV8(isolate, err_msg), {}));
 }
 
 }  // namespace gin_helper
diff --git a/shell/common/gin_helper/error_thrower.h b/shell/common/gin_helper/error_thrower.h
index bd5981b57585..4e3c90d50b00 100644
--- a/shell/common/gin_helper/error_thrower.h
+++ b/shell/common/gin_helper/error_thrower.h
@@ -14,8 +14,8 @@ namespace gin_helper {
 
 class ErrorThrower {
  public:
-  explicit ErrorThrower(v8::Isolate* isolate);
-  ErrorThrower();
+  constexpr explicit ErrorThrower(v8::Isolate* isolate) : isolate_{isolate} {}
+  constexpr ErrorThrower() = default;
   ~ErrorThrower() = default;
 
   void ThrowError(std::string_view err_msg) const;
@@ -24,14 +24,14 @@ class ErrorThrower {
   void ThrowReferenceError(std::string_view err_msg) const;
   void ThrowSyntaxError(std::string_view err_msg) const;
 
-  v8::Isolate* isolate() const { return isolate_; }
+  v8::Isolate* isolate() const;
 
  private:
   using ErrorGenerator = v8::Local<v8::Value> (*)(v8::Local<v8::String> err_msg,
                                                   v8::Local<v8::Value> options);
   void Throw(ErrorGenerator gen, std::string_view err_msg) const;
 
-  raw_ptr<v8::Isolate> isolate_;
+  raw_ptr<v8::Isolate> isolate_ = {};
 };
 
 }  // namespace gin_helper