Bug 1513615 - part 3 - do more work in nsTimerEvent's constructor; r=glandium
authorNathan Froyd <froydnj@mozilla.com>
Tue, 08 Jan 2019 19:31:40 -0500
changeset 510107 9ea519d6b3c85c65646799b1fd85cbbaae054483
parent 510106 7adf8f3377828112210c2070055798e79c01f066
child 510108 f358ebdec0b4996d41f43da7493cbb055b3cbe42
push id10547
push userffxbld-merge
push dateMon, 21 Jan 2019 13:03:58 +0000
treeherdermozilla-beta@24ec1916bffe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersglandium
bugs1513615
milestone66.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1513615 - part 3 - do more work in nsTimerEvent's constructor; r=glandium nsTimerEvent goes through a multi-step initialization for reasons that are lost to time. We are also seeing peculiar crashes in `nsTimerEvent::SetTimer()` that are only explainable by `SetTimer` finding a non-null pointer where there should have been a null pointer. The compiler ought to have been able to optimize those bits away, but no matter: we can do the job ourselves and make the code clearer. Since we only call `SetTimer` once, we should just move its work into nsTimerEvent's constructor.
xpcom/threads/TimerThread.cpp
--- a/xpcom/threads/TimerThread.cpp
+++ b/xpcom/threads/TimerThread.cpp
@@ -129,58 +129,58 @@ class nsTimerEvent final : public Cancel
     mTimer->Cancel();
     return NS_OK;
   }
 
 #ifdef MOZ_COLLECTING_RUNNABLE_TELEMETRY
   NS_IMETHOD GetName(nsACString& aName) override;
 #endif
 
-  nsTimerEvent()
-      : mozilla::CancelableRunnable("nsTimerEvent"), mTimer(), mGeneration(0) {
+  explicit nsTimerEvent(already_AddRefed<nsTimerImpl> aTimer)
+      : mozilla::CancelableRunnable("nsTimerEvent"),
+        mTimer(aTimer),
+        mGeneration(mTimer->GetGeneration()) {
     // Note: We override operator new for this class, and the override is
     // fallible!
     sAllocatorUsers++;
+
+    if (MOZ_LOG_TEST(GetTimerLog(), LogLevel::Debug)) {
+      mInitTime = TimeStamp::Now();
+    }
   }
 
-  TimeStamp mInitTime;
-
   static void Init();
   static void Shutdown();
   static void DeleteAllocatorIfNeeded();
 
   static void* operator new(size_t aSize) CPP_THROW_NEW {
     return sAllocator->Alloc(aSize);
   }
   void operator delete(void* aPtr) {
     sAllocator->Free(aPtr);
     DeleteAllocatorIfNeeded();
   }
 
   already_AddRefed<nsTimerImpl> ForgetTimer() { return mTimer.forget(); }
 
-  void SetTimer(already_AddRefed<nsTimerImpl> aTimer) {
-    mTimer = aTimer;
-    mGeneration = mTimer->GetGeneration();
-  }
-
  private:
   nsTimerEvent(const nsTimerEvent&) = delete;
   nsTimerEvent& operator=(const nsTimerEvent&) = delete;
   nsTimerEvent& operator=(const nsTimerEvent&&) = delete;
 
   ~nsTimerEvent() {
     MOZ_ASSERT(!sCanDeleteAllocator || sAllocatorUsers > 0,
                "This will result in us attempting to deallocate the "
                "nsTimerEvent allocator twice");
     sAllocatorUsers--;
   }
 
+  TimeStamp mInitTime;
   RefPtr<nsTimerImpl> mTimer;
-  int32_t mGeneration;
+  const int32_t mGeneration;
 
   static TimerEventAllocator* sAllocator;
 
   // Timer thread state may be accessed during GC, so uses of this atomic are
   // not preserved when recording/replaying.
   static Atomic<int32_t, SequentiallyConsistent,
                 recordreplay::Behavior::DontPreserve>
       sAllocatorUsers;
@@ -724,23 +724,17 @@ already_AddRefed<nsTimerImpl> TimerThrea
 #endif
 
   nsCOMPtr<nsIEventTarget> target = timer->mEventTarget;
 
   void* p = nsTimerEvent::operator new(sizeof(nsTimerEvent));
   if (!p) {
     return timer.forget();
   }
-  RefPtr<nsTimerEvent> event = ::new (KnownNotNull, p) nsTimerEvent();
-
-  if (MOZ_LOG_TEST(GetTimerLog(), LogLevel::Debug)) {
-    event->mInitTime = TimeStamp::Now();
-  }
-
-  event->SetTimer(timer.forget());
+  RefPtr<nsTimerEvent> event = ::new (KnownNotNull, p) nsTimerEvent(timer.forget());
 
   nsresult rv;
   {
     // We release mMonitor around the Dispatch because if this timer is targeted
     // at the TimerThread we'll deadlock.
     MonitorAutoUnlock unlock(mMonitor);
     rv = target->Dispatch(event, NS_DISPATCH_NORMAL);
   }