Add V8 cross-thread profiling patches

V8's profiler does not expect isolates to be locked from different threads. Pull request #710 moved inspector protocol handling to a separate thread, meaning profiling was started and stopped on the inspector thread, not the Worker's request connection thread. If I remember right, the result of this is that the profiler ends up sampling the inspector thread, which doesn't normally run JavaScript, and causes a bunch of "(program)" placeholder spans to show up in the profile.

We previously discovered this issue with our internal runtime, which has always been heavily multi-threaded, and patched around the problem. This commit copies the two relevant patches from our internal repo.

Fixes #1754.

WORKSPACE

@@ -479,6 +479,8 @@ http_archive(
         "//:patches/v8/0015-Modify-where-to-look-for-fp16-dependency.-This-depen.patch",
         "//:patches/v8/0016-Expose-v8-Symbol-GetDispose.patch",
         "//:patches/v8/0017-Rename-V8_COMPRESS_POINTERS_IN_ISOLATE_CAGE-V8_COMPR.patch",
+        "//:patches/v8/0018-EW-3483-CPU-profiler-patches.patch",
+        "//:patches/v8/0019-EW-6624-Profiler-fix-for-V8-versions-10.0-and-above.patch",
     ],
     integrity = "sha256-5gW+N4R+oVxzMZexrqgp14gU42bsFBc5RHPN2oOrTTw=",
     strip_prefix = "v8-12.6.228.9",

patches/v8/0018-EW-3483-CPU-profiler-patches.patch

@@ -0,0 +1,178 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Bradley Thwaites <bradley@cloudflare.com>
+Date: Mon, 5 Oct 2020 10:38:59 -0500
+Subject: EW-3483: CPU profiler patches
+
+- Avoids use of pthread_self(), which uses an incorrect cached id.
+- Sampler::PlatformData now calls SYS_getpid and saves tgid.
+- SamplerManager calls SYS_getpid to set a thread_local tgid.
+- SamplerManager::DoSample uses tgid to look up in the sample map.
+- New samplers are added to SamplerManager using tgid, not vm_tid.
+- ThreadManager records the tid of the last thread to lock the
+isolate.
+- Sampler::DoSample sends SIGPROF to that tid.
+- Add libsampler dependencies to BUILD.gn
+
+diff --git a/src/execution/v8threads.cc b/src/execution/v8threads.cc
+index 4205817b73f3d954be4109d91721037b3aa2d4ee..326cb6bcd0769646575f8b30d5327dfa6f747fda 100644
+--- a/src/execution/v8threads.cc
++++ b/src/execution/v8threads.cc
+@@ -2,6 +2,9 @@
+ // Use of this source code is governed by a BSD-style license that can be
+ // found in the LICENSE file.
+
++#include <unistd.h>
++#include <sys/syscall.h>
++
+ #include "src/execution/v8threads.h"
+
+ #include "include/v8-locker.h"
+@@ -22,6 +25,11 @@ namespace {
+ // API code to verify that the lock is always held when V8 is being entered.
+ base::AtomicWord g_locker_was_ever_used_ = 0;
+
++static pid_t GetCurrentTid() {
++  static thread_local pid_t tid = static_cast<pid_t>(syscall(SYS_gettid));
++  return tid;
++}
++
+ }  // namespace
+
+ // Once the Locker is initialized, the current thread will be guaranteed to have
+@@ -149,6 +157,7 @@ bool ThreadManager::RestoreThread() {
+ void ThreadManager::Lock() {
+   mutex_.Lock();
+   mutex_owner_.store(ThreadId::Current(), std::memory_order_relaxed);
++  mutex_owner_tid_.store(GetCurrentTid(), std::memory_order_relaxed);
+   DCHECK(IsLockedByCurrentThread());
+ }
+
+@@ -219,6 +228,7 @@ ThreadState* ThreadState::Next() {
+ // defined as 0.)
+ ThreadManager::ThreadManager(Isolate* isolate)
+     : mutex_owner_(ThreadId::Invalid()),
++      mutex_owner_tid_(0),
+       lazily_archived_thread_(ThreadId::Invalid()),
+       lazily_archived_thread_state_(nullptr),
+       free_anchor_(nullptr),
+diff --git a/src/execution/v8threads.h b/src/execution/v8threads.h
+index 69fb91f91be7895aef14ac74635d1a1d985561d5..f87a6d11944895d7a6e7bf22ce154d695b809a5d 100644
+--- a/src/execution/v8threads.h
++++ b/src/execution/v8threads.h
+@@ -77,6 +77,11 @@ class ThreadManager {
+     return mutex_owner_.load(std::memory_order_relaxed) == id;
+   }
+
++  pid_t GetLockedByThreadId() const {
++    // Returns the tid of the thread that most recently locked the isolate.
++    return mutex_owner_tid_.load(std::memory_order_relaxed);
++  }
++
+   ThreadId CurrentId();
+
+   // Iterate over in-use states.
+@@ -95,6 +100,7 @@ class ThreadManager {
+   // {ThreadId} must be trivially copyable to be stored in {std::atomic}.
+   ASSERT_TRIVIALLY_COPYABLE(i::ThreadId);
+   std::atomic<ThreadId> mutex_owner_;
++  std::atomic<pid_t> mutex_owner_tid_;
+   ThreadId lazily_archived_thread_;
+   ThreadState* lazily_archived_thread_state_;
+
+diff --git a/src/libsampler/sampler.cc b/src/libsampler/sampler.cc
+index 0164cbbf5b8ef384afd927870342ca723583cea5..1ce6e6e5af90324eba01304dfde4e621f21af259 100644
+--- a/src/libsampler/sampler.cc
++++ b/src/libsampler/sampler.cc
+@@ -15,6 +15,8 @@
+ #include <signal.h>
+ #include <sys/time.h>
+ #include <atomic>
++#include "src/execution/isolate.h"
++#include "src/execution/v8threads.h"
+
+ #if !V8_OS_QNX && !V8_OS_AIX
+ #include <sys/syscall.h>
+@@ -197,17 +199,19 @@ bool AtomicGuard::is_success() const { return is_success_; }
+
+ class Sampler::PlatformData {
+  public:
+-  PlatformData() : vm_tid_(pthread_self()) {}
++  PlatformData() : vm_tid_(pthread_self()), tgid_((pid_t)syscall(SYS_getpid)) {}
+   pthread_t vm_tid() const { return vm_tid_; }
++  pid_t tgid() const { return tgid_; }
+
+  private:
+   pthread_t vm_tid_;
++  pid_t tgid_;
+ };
+
+ void SamplerManager::AddSampler(Sampler* sampler) {
+   AtomicGuard atomic_guard(&samplers_access_counter_);
+   DCHECK(sampler->IsActive());
+-  pthread_t thread_id = sampler->platform_data()->vm_tid();
++  pid_t thread_id = sampler->platform_data()->tgid();
+   auto it = sampler_map_.find(thread_id);
+   if (it == sampler_map_.end()) {
+     SamplerList samplers;
+@@ -223,7 +227,7 @@ void SamplerManager::AddSampler(Sampler* sampler) {
+ void SamplerManager::RemoveSampler(Sampler* sampler) {
+   AtomicGuard atomic_guard(&samplers_access_counter_);
+   DCHECK(sampler->IsActive());
+-  pthread_t thread_id = sampler->platform_data()->vm_tid();
++  pid_t thread_id = sampler->platform_data()->tgid();
+   auto it = sampler_map_.find(thread_id);
+   DCHECK_NE(it, sampler_map_.end());
+   SamplerList& samplers = it->second;
+@@ -234,12 +238,18 @@ void SamplerManager::RemoveSampler(Sampler* sampler) {
+   }
+ }
+
++namespace {
++pid_t GetTgid() {
++  static pid_t tgid = static_cast<pid_t>(syscall(SYS_getpid));
++  return tgid;
++}
++} // anonymous namespace
++
+ void SamplerManager::DoSample(const v8::RegisterState& state) {
+   AtomicGuard atomic_guard(&samplers_access_counter_, false);
+   // TODO(petermarshall): Add stat counters for the bailouts here.
+   if (!atomic_guard.is_success()) return;
+-  pthread_t thread_id = pthread_self();
+-  auto it = sampler_map_.find(thread_id);
++  auto it = sampler_map_.find(GetTgid());
+   if (it == sampler_map_.end()) return;
+   SamplerList& samplers = it->second;
+
+@@ -586,8 +596,12 @@ void Sampler::Stop() {
+ void Sampler::DoSample() {
+   base::RecursiveMutexGuard lock_guard(SignalHandler::mutex());
+   if (!SignalHandler::Installed()) return;
++  DCHECK(IsActive());
++  auto tm = reinterpret_cast<internal::Isolate*>(isolate_)->thread_manager();
++  pid_t lockedTid = tm->GetLockedByThreadId();
++  if (lockedTid == 0) return; // No thread is locking the isolate, so don't sample
+   SetShouldRecordSample();
+-  pthread_kill(platform_data()->vm_tid(), SIGPROF);
++  syscall(SYS_tgkill, platform_data()->tgid(), lockedTid, SIGPROF);
+ }
+
+ #elif V8_OS_WIN || V8_OS_CYGWIN
+diff --git a/src/libsampler/sampler.h b/src/libsampler/sampler.h
+index 98c0606151360b472ea4ed3016fae0e4e24614a4..88542f7459312dbcf5fd6a9bc1d6df1d31936d71 100644
+--- a/src/libsampler/sampler.h
++++ b/src/libsampler/sampler.h
+@@ -147,11 +147,12 @@ class V8_EXPORT_PRIVATE SamplerManager {
+
+  private:
+   SamplerManager() = default;
++
+   // Must be a friend so that it can access the private constructor for the
+   // global lazy instance.
+   friend class base::LeakyObject<SamplerManager>;
+
+-  std::unordered_map<pthread_t, SamplerList> sampler_map_;
++  std::unordered_map<pid_t, SamplerList> sampler_map_;
+   AtomicMutex samplers_access_counter_{false};
+ };
+

patches/v8/0019-EW-6624-Profiler-fix-for-V8-versions-10.0-and-above.patch

@@ -0,0 +1,52 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Samuel Warfield <swarfield@cloudflare.com>
+Date: Fri, 5 Aug 2022 09:43:13 -0600
+Subject: EW-6624 Profiler fix for V8 versions 10.0 and above.
+
+There was an update to the sampler that did thread lock lookups with
+the V8 Thread ID whereas the Runtime allocates each isolate to an
+individual thread, so the linux thread ID must be used.
+
+diff --git a/src/execution/v8threads.h b/src/execution/v8threads.h
+index f87a6d11944895d7a6e7bf22ce154d695b809a5d..00888520a8cb164dff7612f4d829c508176e35f5 100644
+--- a/src/execution/v8threads.h
++++ b/src/execution/v8threads.h
+@@ -73,8 +73,8 @@ class ThreadManager {
+   bool IsLockedByCurrentThread() const {
+     return mutex_owner_.load(std::memory_order_relaxed) == ThreadId::Current();
+   }
+-  bool IsLockedByThread(ThreadId id) const {
+-    return mutex_owner_.load(std::memory_order_relaxed) == id;
++  bool IsLockedByThread() const {
++    return GetLockedByThreadId() != 0;
+   }
+
+   pid_t GetLockedByThreadId() const {
+diff --git a/src/logging/log.cc b/src/logging/log.cc
+index d990a57783f1111c15467bd846b17e324a252b8d..9b491f51ce0073bb9997d6c81a94ab939c025722 100644
+--- a/src/logging/log.cc
++++ b/src/logging/log.cc
+@@ -1127,8 +1127,7 @@ class Ticker : public sampler::Sampler {
+     if (!profiler_) return;
+     Isolate* isolate = reinterpret_cast<Isolate*>(this->isolate());
+     if (isolate->was_locker_ever_used() &&
+-        (!isolate->thread_manager()->IsLockedByThread(
+-             perThreadData_->thread_id()) ||
++        (!isolate->thread_manager()->IsLockedByThread() ||
+          perThreadData_->thread_state() != nullptr))
+       return;
+ #if V8_HEAP_USE_PKU_JIT_WRITE_PROTECT
+diff --git a/src/profiler/cpu-profiler.cc b/src/profiler/cpu-profiler.cc
+index a6bff4732933b22ee3e68f3ba82bdf6bfa3e5e88..a27f9fc487b48aaa8d8377d1918fa2c7249864af 100644
+--- a/src/profiler/cpu-profiler.cc
++++ b/src/profiler/cpu-profiler.cc
+@@ -41,8 +41,7 @@ class CpuSampler : public sampler::Sampler {
+   void SampleStack(const v8::RegisterState& regs) override {
+     Isolate* isolate = reinterpret_cast<Isolate*>(this->isolate());
+     if (isolate->was_locker_ever_used() &&
+-        (!isolate->thread_manager()->IsLockedByThread(
+-             perThreadData_->thread_id()) ||
++        (!isolate->thread_manager()->IsLockedByThread() ||
+          perThreadData_->thread_state() != nullptr)) {
+       ProfilerStats::Instance()->AddReason(
+           ProfilerStats::Reason::kIsolateNotLocked);