Eliminate some hot AreEventsDisallowed calls (#791)

replayio · Jul 2, 2023 · 645762a · 645762a
1 parent 5e71eeb
commit 645762a
Show file tree

Hide file tree

Showing 10 changed files with 33 additions and 54 deletions.
diff --git a/DEPS b/DEPS
@@ -311,7 +311,7 @@ vars = {
   # Three lines of non-changing comments so that
   # the commit queue can handle CLs rolling V8
   # and whatever else without interference from each other.
-  'v8_revision': '2f1673e26beb50c425ca043bd6ecdf3d9a51f00b',
+  'v8_revision': '1183bc3f8aea9d6055d4a76922163674861529e8',
   # Three lines of non-changing comments so that
   # the commit queue can handle CLs rolling swarming_client
   # and whatever else without interference from each other.

diff --git a/base/record_replay.cc b/base/record_replay.cc
@@ -38,13 +38,17 @@ namespace recordreplay {
         (const char* feature, const char* subfeature),                  \
         (feature, subfeature), bool, true)                              \
   Macro(V8RecordReplayHasDisabledFeatures, (), (), bool, false)         \
+  Macro(V8RecordReplayAreAssertsDisabled, (), (), bool, false)          \
   Macro(V8IsMainThread, (), (), bool, false)                            \
   Macro(V8RecordReplayHadMismatch, (), (), bool, false)
 
 #define ForEachV8APIVoid(Macro)                                         \
   Macro(V8RecordReplayAssertVA,                                         \
         (const char* format, va_list args),                             \
         (format, args))                                                 \
+  Macro(V8RecordReplayAssertMaybeEventsDisallowedVA,                    \
+        (const char* format, va_list args),                             \
+        (format, args))                                                 \
   Macro(V8RecordReplayAssertBytes,                                      \
         (const char* why, const void* buf, size_t size),                \
         (why, buf, size))                                               \
@@ -223,6 +227,15 @@ void Assert(const char* format, ...) {
 #endif
 }
 
+void AssertMaybeEventsDisallowed(const char* format, ...) {
+#ifndef NACL_TC_REV
+  va_list ap;
+  va_start(ap, format);
+  V8RecordReplayAssertMaybeEventsDisallowedVA(format, ap);
+  va_end(ap);
+#endif
+}
+
 void Diagnostic(const char* format, ...) {
 #ifndef NACL_TC_REV
   va_list ap;
@@ -254,6 +267,10 @@ void AssertBytes(const char* why, const void* buf, size_t size) {
   V8RecordReplayAssertBytes(why, buf, size);
 }
 
+bool AreAssertsDisabled() {
+  return V8RecordReplayAreAssertsDisabled();
+}
+
 void Print(const char* format, ...) {
 #ifndef NACL_TC_REV
   va_list ap;

diff --git a/base/record_replay.h b/base/record_replay.h
@@ -34,7 +34,9 @@ void Diagnostic(const char* format, ...);
 void Warning(const char* format, ...);
 void Trace(const char* format, ...);
 void Assert(const char* format, ...);
+void AssertMaybeEventsDisallowed(const char* format, ...);
 void AssertBytes(const char* why, const void* buf, size_t size);
+bool AreAssertsDisabled();
 
 uintptr_t RecordReplayValue(const char* why, uintptr_t v);
 void RecordReplayBytes(const char* why, void* buf, size_t size);

diff --git a/base/synchronization/waitable_event_posix.cc b/base/synchronization/waitable_event_posix.cc
@@ -99,10 +99,10 @@ bool WaitableEvent::IsSignaled() {
 // -----------------------------------------------------------------------------
 class SyncWaiter : public WaitableEvent::Waiter {
  public:
-  SyncWaiter()
+  SyncWaiter(bool record_replay_unordered)
      : fired_(false),
        signaling_event_(nullptr),
-       lock_(recordreplay::AreEventsDisallowed() ? nullptr : "SyncWaiter.lock_"),
+       lock_(record_replay_unordered ? nullptr : "SyncWaiter.lock_"),
        cv_(&lock_) {}
 
   ~SyncWaiter() override {}
@@ -203,7 +203,7 @@ bool WaitableEvent::TimedWait(const TimeDelta& wait_delta) {
     return true;
   }
 
-  SyncWaiter sw;
+  SyncWaiter sw(kernel_->record_replay_unordered_);
   if (!waiting_is_blocking_)
     sw.cv()->declare_only_used_while_idle();
   sw.lock()->Acquire();
@@ -275,8 +275,10 @@ cmp_fst_addr(const std::pair<WaitableEvent*, unsigned> &a,
 // NO_THREAD_SAFETY_ANALYSIS: Complex control flow.
 size_t WaitableEvent::WaitMany(WaitableEvent** raw_waitables,
                                size_t count) NO_THREAD_SAFETY_ANALYSIS {
+  bool record_replay_unordered = count && raw_waitables[0]->kernel_->record_replay_unordered_;
+
   absl::optional<recordreplay::AutoDisallowEvents> disallow;
-  if (count && raw_waitables[0]->kernel_->record_replay_unordered_)
+  if (record_replay_unordered)
     disallow.emplace("WaitableEvent::WaitMany");
 
   DCHECK(count) << "Cannot wait on no events";
@@ -304,7 +306,7 @@ size_t WaitableEvent::WaitMany(WaitableEvent** raw_waitables,
     DCHECK(waitables[i].first != waitables[i+1].first);
   }
 
-  SyncWaiter sw;
+  SyncWaiter sw(record_replay_unordered);
 
   const size_t r = EnqueueMany(&waitables[0], count, &sw);
   if (r < count) {

diff --git a/base/task/thread_pool/priority_queue.cc b/base/task/thread_pool/priority_queue.cc
@@ -134,12 +134,6 @@ RegisteredTaskSource PriorityQueue::PopTaskSource() {
       task_source_and_sort_key.take_task_source();
   container_.pop();
 
-  // https://linear.app/replay/issue/RUN-753
-  if (!recordreplay::AreEventsDisallowed()) {
-    recordreplay::Assert("PriorityQueue::PopTaskSource %d",
-                         recordreplay::PointerId(task_source.get()));
-  }
-
   return task_source;
 }
 

diff --git a/base/task/thread_pool/task_source.cc b/base/task/thread_pool/task_source.cc
@@ -72,17 +72,12 @@ TaskSource::TaskSource(const TaskTraits& traits,
       lock_(recordreplay::AreEventsDisallowed() ? nullptr : "TaskSource.lock_"),
       task_runner_(task_runner),
       execution_mode_(execution_mode) {
-  // https://linear.app/replay/issue/RUN-753
-  if (!recordreplay::AreEventsDisallowed())
-    recordreplay::RegisterPointer("TaskSource", this);
   DCHECK(task_runner_ ||
          execution_mode_ == TaskSourceExecutionMode::kParallel ||
          execution_mode_ == TaskSourceExecutionMode::kJob);
 }
 
-TaskSource::~TaskSource() {
-  recordreplay::UnregisterPointer(this);
-}
+TaskSource::~TaskSource() = default;
 
 TaskSource::Transaction TaskSource::BeginTransaction() {
   return Transaction(this);

diff --git a/base/task/thread_pool/thread_group_impl.cc b/base/task/thread_pool/thread_group_impl.cc
@@ -96,8 +96,7 @@ class ThreadGroupImpl::ScopedCommandsExecutor
     : public ThreadGroup::BaseScopedCommandsExecutor {
  public:
   explicit ScopedCommandsExecutor(ThreadGroupImpl* outer) : outer_(outer) {
-    if (recordreplay::AreEventsDisallowed())
-      CHECK(outer_->RecordReplayUnordered());
+    DCHECK(!recordreplay::AreEventsDisallowed() || outer_->RecordReplayUnordered());
   }
 
   ScopedCommandsExecutor(const ScopedCommandsExecutor&) = delete;
@@ -574,12 +573,8 @@ RegisteredTaskSource ThreadGroupImpl::WorkerThreadDelegateImpl::GetWork(
   outer_->EnsureEnoughWorkersLockRequired(&executor);
   executor.FlushWorkerCreation(&outer_->lock_);
 
-  if (!CanGetWorkLockRequired(&executor, worker)) {
-    // https://linear.app/replay/issue/RUN-753
-    if (!recordreplay::AreEventsDisallowed())
-      recordreplay::Assert("WorkerThreadDelegateImpl::GetWork #1");
+  if (!CanGetWorkLockRequired(&executor, worker))
     return nullptr;
-  }
 
   RegisteredTaskSource task_source;
   TaskPriority priority;
@@ -597,9 +592,6 @@ RegisteredTaskSource ThreadGroupImpl::WorkerThreadDelegateImpl::GetWork(
     task_source = outer_->TakeRegisteredTaskSource(&executor);
   }
   if (!task_source) {
-    // https://linear.app/replay/issue/RUN-753
-    if (!recordreplay::AreEventsDisallowed())
-      recordreplay::Assert("WorkerThreadDelegateImpl::GetWork #2");
     OnWorkerBecomesIdleLockRequired(&executor, worker);
     return nullptr;
   }
@@ -610,12 +602,6 @@ RegisteredTaskSource ThreadGroupImpl::WorkerThreadDelegateImpl::GetWork(
   write_worker().current_task_priority = priority;
   write_worker().current_shutdown_behavior = task_source->shutdown_behavior();
 
-  // https://linear.app/replay/issue/RUN-753
-  if (!recordreplay::AreEventsDisallowed()) {
-    recordreplay::Assert("WorkerThreadDelegateImpl::GetWork Done %d",
-                         recordreplay::PointerId(task_source.get()));
-  }
-
   return task_source;
 }
 

diff --git a/base/task/thread_pool/thread_group_native.cc b/base/task/thread_pool/thread_group_native.cc
@@ -127,23 +127,13 @@ RegisteredTaskSource ThreadGroupNative::GetWork() {
   while (!task_source && !priority_queue_.IsEmpty()) {
     priority = priority_queue_.PeekSortKey().priority();
     // Enforce the CanRunPolicy.
-    if (!task_tracker_->CanRunPriority(priority)) {
-      // https://linear.app/replay/issue/RUN-753
-      if (!recordreplay::AreEventsDisallowed())
-        recordreplay::Assert("ThreadGroupNative::GetWork #1");
+    if (!task_tracker_->CanRunPriority(priority))
       return nullptr;
-    }
 
     task_source = TakeRegisteredTaskSource(&workers_executor);
   }
   UpdateMinAllowedPriorityLockRequired();
 
-  // https://linear.app/replay/issue/RUN-753
-  if (!recordreplay::AreEventsDisallowed()) {
-    recordreplay::Assert("ThreadGroupNative::GetWork Done %d",
-                         recordreplay::PointerId(task_source.get()));
-  }
-
   return task_source;
 }
 

diff --git a/base/task/thread_pool/worker_thread_stack.cc b/base/task/thread_pool/worker_thread_stack.cc
@@ -19,11 +19,6 @@ WorkerThreadStack::~WorkerThreadStack() = default;
 
 void WorkerThreadStack::Push(WorkerThread* worker) {
   DCHECK(!Contains(worker)) << "WorkerThread already on stack";
-
-  if (!recordreplay::AreEventsDisallowed()) {
-    recordreplay::Assert("[RUN-597] WorkerThreadStack::Push %d", IsEmpty());
-  }
-
   if (!IsEmpty())
     stack_.back()->BeginUnusedPeriod();
   stack_.push_back(worker);

diff --git a/base/time/time_win.cc b/base/time/time_win.cc
@@ -608,10 +608,8 @@ TimeTicks::TickFunctionType TimeTicks::SetMockTickFunction(
 
 namespace subtle {
 TimeTicks TimeTicksNowIgnoringOverride() {
-  if (!recordreplay::AreEventsDisallowed()) {
-    recordreplay::Assert("[RUN-1916] TimeTicksNowIgnoringOverride %d",
-                         g_time_ticks_now_ignoring_override_function == &QPCNow);
-  }
+  recordreplay::AssertMaybeEventsDisallowed("[RUN-1916] TimeTicksNowIgnoringOverride %d",
+                                            g_time_ticks_now_ignoring_override_function == &QPCNow);
 
   return g_time_ticks_now_ignoring_override_function.load(
       std::memory_order_relaxed)();