Backed out changeset (Bug 1615564) 1c58f39177c0 for bustages at TimeoutManager.cpp. CLOSED TREE
authorAtila Butkovits <abutkovits@mozilla.com>
Thu, 21 May 2020 13:06:15 +0300
changeset 531410 53445bcdff8e06da6f89621915599872736c029f
parent 531409 1c58f39177c0ea2e3bbd22fbb92fab39f8c9d2e0
child 531411 09a04d08423aab792d3e3d87ec8c5c0b3a9e8e3d
push id37439
push userbtara@mozilla.com
push dateThu, 21 May 2020 21:49:34 +0000
treeherdermozilla-central@92c11f0bf14b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1615564
milestone78.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
Backed out changeset (Bug 1615564) 1c58f39177c0 for bustages at TimeoutManager.cpp. CLOSED TREE
dom/base/Timeout.h
dom/base/TimeoutManager.cpp
dom/base/TimeoutManager.h
--- a/dom/base/Timeout.h
+++ b/dom/base/Timeout.h
@@ -10,121 +10,52 @@
 #include "mozilla/dom/PopupBlocker.h"
 #include "mozilla/dom/TimeoutHandler.h"
 #include "mozilla/LinkedList.h"
 #include "mozilla/TimeStamp.h"
 #include "nsCOMPtr.h"
 #include "nsGlobalWindowInner.h"
 #include "nsCycleCollectionParticipant.h"
 #include "GeckoProfiler.h"
-#include "nsDataHashtable.h"
 
 class nsIEventTarget;
 class nsIPrincipal;
 class nsIEventTarget;
 
 namespace mozilla {
 namespace dom {
 
 /*
  * Timeout struct that holds information about each script
  * timeout.  Holds a strong reference to an nsITimeoutHandler, which
  * abstracts the language specific cruft.
  */
-class Timeout final : protected LinkedListElement<RefPtr<Timeout>> {
+class Timeout final : public LinkedListElement<RefPtr<Timeout>> {
  public:
   Timeout();
 
   NS_DECL_CYCLE_COLLECTION_NATIVE_CLASS(Timeout)
   NS_INLINE_DECL_CYCLE_COLLECTING_NATIVE_REFCOUNTING(Timeout)
 
   enum class Reason : uint8_t {
     eTimeoutOrInterval,
     eIdleCallbackTimeout,
   };
 
-  struct TimeoutIdAndReason {
-    uint32_t mId;
-    Reason mReason;
-  };
-
-  class TimeoutHashKey : public PLDHashEntryHdr {
-   public:
-    typedef const TimeoutIdAndReason& KeyType;
-    typedef const TimeoutIdAndReason* KeyTypePointer;
-
-    explicit TimeoutHashKey(KeyTypePointer aKey) : mValue(*aKey) {}
-    TimeoutHashKey(TimeoutHashKey&& aOther)
-        : PLDHashEntryHdr(std::move(aOther)),
-          mValue(std::move(aOther.mValue)) {}
-    ~TimeoutHashKey() = default;
-
-    KeyType GetKey() const { return mValue; }
-    bool KeyEquals(KeyTypePointer aKey) const {
-      return aKey->mId == mValue.mId && aKey->mReason == mValue.mReason;
-    }
-
-    static KeyTypePointer KeyToPointer(KeyType aKey) { return &aKey; }
-    static PLDHashNumber HashKey(KeyTypePointer aKey) {
-      return aKey->mId | (static_cast<uint8_t>(aKey->mReason) << 31);
-    }
-    enum { ALLOW_MEMMOVE = true };
-
-   private:
-    const TimeoutIdAndReason mValue;
-  };
-
-  class TimeoutSet : public nsDataHashtable<TimeoutHashKey, Timeout*> {
-   public:
-    NS_INLINE_DECL_REFCOUNTING(TimeoutSet);
-
-   private:
-    ~TimeoutSet() = default;
-  };
-
   void SetWhenOrTimeRemaining(const TimeStamp& aBaseTime,
                               const TimeDuration& aDelay);
 
   // Can only be called when not frozen.
   const TimeStamp& When() const;
 
   const TimeStamp& SubmitTime() const;
 
   // Can only be called when frozen.
   const TimeDuration& TimeRemaining() const;
 
-  void SetTimeoutContainer(TimeoutSet* aTimeouts) {
-    MOZ_ASSERT(mTimeoutId != 0);
-    TimeoutIdAndReason key = {mTimeoutId, mReason};
-    if (mTimeouts) {
-      mTimeouts->Remove(key);
-    }
-    mTimeouts = aTimeouts;
-    if (mTimeouts) {
-      mTimeouts->Put(key, this);
-    }
-  }
-
-  // Override some LinkedListElement methods so that remove()
-  // calls can call SetTimeoutContainer.
-  Timeout* getNext() { return LinkedListElement<RefPtr<Timeout>>::getNext(); }
-
-  void setNext(Timeout* aNext) {
-    return LinkedListElement<RefPtr<Timeout>>::setNext(aNext);
-  }
-
-  Timeout* getPrevious() {
-    return LinkedListElement<RefPtr<Timeout>>::getPrevious();
-  }
-
-  void remove() {
-    SetTimeoutContainer(nullptr);
-    LinkedListElement<RefPtr<Timeout>>::remove();
-  }
-
 #ifdef MOZ_GECKO_PROFILER
   UniqueProfilerBacktrace TakeProfilerBacktrace() { return std::move(mCause); }
 #endif
 
  private:
   // mWhen and mTimeRemaining can't be in a union, sadly, because they
   // have constructors.
   // Nominal time to run this timeout.  Use only when timeouts are not
@@ -134,32 +65,30 @@ class Timeout final : protected LinkedLi
   // Remaining time to wait.  Used only when timeouts are frozen.
   TimeDuration mTimeRemaining;
 
   // Time that the timeout started, restarted, or was frozen.  Useful for
   // logging time from (virtual) start of a timer until the time it fires
   // (or is cancelled, etc)
   TimeStamp mSubmitTime;
 
-  ~Timeout() { SetTimeoutContainer(nullptr); }
+  ~Timeout() = default;
 
  public:
   // Public member variables in this section.  Please don't add to this list
   // or mix methods with these.  The interleaving public/private sections
   // is necessary as we migrate members to private while still trying to
   // keep decent binary packing.
 
   // Window for which this timeout fires
   RefPtr<nsGlobalWindowInner> mWindow;
 
   // The language-specific information about the callback.
   RefPtr<TimeoutHandler> mScriptHandler;
 
-  RefPtr<TimeoutSet> mTimeouts;
-
   // Interval
   TimeDuration mInterval;
 
 #ifdef MOZ_GECKO_PROFILER
   UniqueProfilerBacktrace mCause;
 #endif
 
   // Returned as value of setTimeout()
@@ -188,18 +117,14 @@ class Timeout final : protected LinkedLi
   // True if the timeout was cleared
   bool mCleared;
 
   // True if this is one of the timeouts that are currently running
   bool mRunning;
 
   // True if this is a repeating/interval timer
   bool mIsInterval;
-
- protected:
-  friend class LinkedList<RefPtr<Timeout>>;
-  friend class LinkedListElement<RefPtr<Timeout>>;
 };
 
 }  // namespace dom
 }  // namespace mozilla
 
 #endif  // mozilla_dom_timeout_h
--- a/dom/base/TimeoutManager.cpp
+++ b/dom/base/TimeoutManager.cpp
@@ -516,20 +516,19 @@ nsresult TimeoutManager::SetTimeout(Time
     // in some cases.
     if (interval <= StaticPrefs::dom_disable_open_click_delay()) {
       timeout->mPopupState = PopupBlocker::GetPopupControlState();
     }
   }
 
   Timeouts::SortBy sort(mWindow.IsFrozen() ? Timeouts::SortBy::TimeRemaining
                                            : Timeouts::SortBy::TimeWhen);
+  mTimeouts.Insert(timeout, sort);
 
   timeout->mTimeoutId = GetTimeoutId(aReason);
-  mTimeouts.Insert(timeout, sort);
-
   *aReturn = timeout->mTimeoutId;
 
   MOZ_LOG(
       gTimeoutLog, LogLevel::Debug,
       ("Set%s(TimeoutManager=%p, timeout=%p, delay=%i, "
        "minimum=%f, throttling=%s, state=%s(%s), realInterval=%f) "
        "returned timeout ID %u, budget=%d\n",
        aIsInterval ? "Interval" : "Timeout", this, timeout.get(), interval,
@@ -553,67 +552,71 @@ void TimeoutManager::ClearTimeout(int32_
 }
 
 bool TimeoutManager::ClearTimeoutInternal(int32_t aTimerId,
                                           Timeout::Reason aReason,
                                           bool aIsIdle) {
   uint32_t timerId = (uint32_t)aTimerId;
   Timeouts& timeouts = aIsIdle ? mIdleTimeouts : mTimeouts;
   RefPtr<TimeoutExecutor>& executor = aIsIdle ? mIdleExecutor : mExecutor;
+  bool firstTimeout = true;
   bool deferredDeletion = false;
+  bool cleared = false;
 
-  Timeout* timeout = timeouts.GetTimeout(aTimerId, aReason);
-  if (!timeout) {
-    return false;
-  }
-  bool firstTimeout = timeout == timeouts.GetFirst();
+  timeouts.ForEachAbortable([&](Timeout* aTimeout) {
+    MOZ_LOG(gTimeoutLog, LogLevel::Debug,
+            ("Clear%s(TimeoutManager=%p, timeout=%p, aTimerId=%u, ID=%u)\n",
+             aTimeout->mIsInterval ? "Interval" : "Timeout", this, aTimeout,
+             timerId, aTimeout->mTimeoutId));
 
-  MOZ_LOG(gTimeoutLog, LogLevel::Debug,
-          ("%s(TimeoutManager=%p, timeout=%p, ID=%u)\n",
-           timeout->mReason == Timeout::Reason::eIdleCallbackTimeout
-               ? "CancelIdleCallback"
-               : timeout->mIsInterval ? "ClearInterval" : "ClearTimeout",
-           this, timeout, timeout->mTimeoutId));
+    if (aTimeout->mTimeoutId == timerId && aTimeout->mReason == aReason) {
+      if (aTimeout->mRunning) {
+        /* We're running from inside the aTimeout. Mark this
+           aTimeout for deferred deletion by the code in
+           RunTimeout() */
+        aTimeout->mIsInterval = false;
+        deferredDeletion = true;
+      } else {
+        /* Delete the aTimeout from the pending aTimeout list */
+        aTimeout->remove();
+      }
+      cleared = true;
+      return true;  // abort!
+    }
 
-  if (timeout->mRunning) {
-    /* We're running from inside the timeout. Mark this
-       timeout for deferred deletion by the code in
-       RunTimeout() */
-    timeout->mIsInterval = false;
-    deferredDeletion = true;
-  } else {
-    /* Delete the aTimeout from the pending aTimeout list */
-    timeout->remove();
-  }
+    firstTimeout = false;
+
+    return false;
+  });
 
   // We don't need to reschedule the executor if any of the following are true:
   //  * If the we weren't cancelling the first timeout, then the executor's
   //    state doesn't need to change.  It will only reflect the next soonest
   //    Timeout.
   //  * If we did cancel the first Timeout, but its currently running, then
   //    RunTimeout() will handle rescheduling the executor.
   //  * If the window has become suspended then we should not start executing
   //    Timeouts.
   if (!firstTimeout || deferredDeletion || mWindow.IsSuspended()) {
-    return true;
+    return cleared;
   }
 
   // Stop the executor and restart it at the next soonest deadline.
   executor->Cancel();
 
   Timeout* nextTimeout = timeouts.GetFirst();
   if (nextTimeout) {
     if (aIsIdle) {
       MOZ_ALWAYS_SUCCEEDS(
           executor->MaybeSchedule(nextTimeout->When(), TimeDuration(0)));
     } else {
       MOZ_ALWAYS_SUCCEEDS(MaybeSchedule(nextTimeout->When()));
     }
   }
-  return true;
+  return cleared;
 }
 
 void TimeoutManager::RunTimeout(const TimeStamp& aNow,
                                 const TimeStamp& aTargetDeadline,
                                 bool aProcessIdle) {
   MOZ_DIAGNOSTIC_ASSERT(!aNow.IsNull());
   MOZ_DIAGNOSTIC_ASSERT(!aTargetDeadline.IsNull());
 
@@ -1080,17 +1083,16 @@ void TimeoutManager::Timeouts::Insert(Ti
        // majority of cases.
        mManager.IsInvalidFiringId(prevSibling->mFiringId);
        prevSibling = prevSibling->getPrevious()) {
     /* Do nothing; just searching */
   }
 
   // Now link in aTimeout after prevSibling.
   if (prevSibling) {
-    aTimeout->SetTimeoutContainer(mTimeouts);
     prevSibling->setNext(aTimeout);
   } else {
     InsertFront(aTimeout);
   }
 
   aTimeout->mFiringId = InvalidFiringId;
 }
 
--- a/dom/base/TimeoutManager.h
+++ b/dom/base/TimeoutManager.h
@@ -134,42 +134,32 @@ class TimeoutManager final {
 
   void UpdateBudget(const TimeStamp& aNow,
                     const TimeDuration& aDuration = TimeDuration());
 
   mozilla::PerformanceCounter* GetPerformanceCounter();
 
  private:
   struct Timeouts {
-    explicit Timeouts(const TimeoutManager& aManager)
-        : mManager(aManager), mTimeouts(new Timeout::TimeoutSet()) {}
+    explicit Timeouts(const TimeoutManager& aManager) : mManager(aManager) {}
 
     // Insert aTimeout into the list, before all timeouts that would
     // fire after it, but no earlier than the last Timeout with a
     // valid FiringId.
     enum class SortBy { TimeRemaining, TimeWhen };
     void Insert(mozilla::dom::Timeout* aTimeout, SortBy aSortBy);
 
     const Timeout* GetFirst() const { return mTimeoutList.getFirst(); }
     Timeout* GetFirst() { return mTimeoutList.getFirst(); }
     const Timeout* GetLast() const { return mTimeoutList.getLast(); }
     Timeout* GetLast() { return mTimeoutList.getLast(); }
     bool IsEmpty() const { return mTimeoutList.isEmpty(); }
-    void InsertFront(Timeout* aTimeout) {
-      aTimeout->SetTimeoutContainer(mTimeouts);
-      mTimeoutList.insertFront(aTimeout);
-    }
-    void InsertBack(Timeout* aTimeout) {
-      aTimeout->SetTimeoutContainer(mTimeouts);
-      mTimeoutList.insertBack(aTimeout);
-    }
-    void Clear() {
-      mTimeouts->Clear();
-      mTimeoutList.clear();
-    }
+    void InsertFront(Timeout* aTimeout) { mTimeoutList.insertFront(aTimeout); }
+    void InsertBack(Timeout* aTimeout) { mTimeoutList.insertBack(aTimeout); }
+    void Clear() { mTimeoutList.clear(); }
 
     template <class Callable>
     void ForEach(Callable c) {
       for (Timeout* timeout = GetFirst(); timeout;
            timeout = timeout->getNext()) {
         c(timeout);
       }
     }
@@ -181,36 +171,26 @@ class TimeoutManager final {
            timeout = timeout->getNext()) {
         if (c(timeout)) {
           return true;
         }
       }
       return false;
     }
 
-    Timeout* GetTimeout(uint32_t aTimeoutId, Timeout::Reason aReason) {
-      Timeout::TimeoutIdAndReason key = {aTimeoutId, aReason};
-      return mTimeouts->Get(key);
-    }
-
    private:
     // The TimeoutManager that owns this Timeouts structure.  This is
     // mainly used to call state inspecting methods like IsValidFiringId().
     const TimeoutManager& mManager;
 
     typedef mozilla::LinkedList<RefPtr<Timeout>> TimeoutList;
 
     // mTimeoutList is generally sorted by mWhen, but new values are always
     // inserted after any Timeouts with a valid FiringId.
     TimeoutList mTimeoutList;
-
-    // mTimeouts is a set of all the timeouts in the mTimeoutList.
-    // It let's one to have O(1) check whether a timeout id/reason is in the
-    // list.
-    RefPtr<Timeout::TimeoutSet> mTimeouts;
   };
 
   // Each nsGlobalWindowInner object has a TimeoutManager member.  This
   // reference points to that holder object.
   nsGlobalWindowInner& mWindow;
   // The executor is specific to the nsGlobalWindow/TimeoutManager, but it
   // can live past the destruction of the window if its scheduled.  Therefore
   // it must be a separate ref-counted object.