From 5e1e544d7bff1e2f9d43cf42f7041d1d57e1d652 Mon Sep 17 00:00:00 2001 From: Olli Pettay Date: Wed, 20 May 2020 14:08:59 +0000 Subject: [PATCH] Bug 1615564, clearing timeouts should be O(1) r=farre Differential Revision: https://phabricator.services.mozilla.com/D74566 --- dom/base/Timeout.h | 79 ++++++++++++++++++++++++++++++++++++- dom/base/TimeoutManager.cpp | 56 +++++++++++++------------- dom/base/TimeoutManager.h | 28 +++++++++++-- 3 files changed, 128 insertions(+), 35 deletions(-) diff --git a/dom/base/Timeout.h b/dom/base/Timeout.h index 8b2bc13f7efa..6ef0c85f6451 100644 --- a/dom/base/Timeout.h +++ b/dom/base/Timeout.h @@ -15,6 +15,7 @@ #include "nsGlobalWindowInner.h" #include "nsCycleCollectionParticipant.h" #include "GeckoProfiler.h" +#include "nsDataHashtable.h" class nsIEventTarget; class nsIPrincipal; @@ -28,7 +29,7 @@ namespace dom { * timeout. Holds a strong reference to an nsITimeoutHandler, which * abstracts the language specific cruft. */ -class Timeout final : public LinkedListElement> { +class Timeout final : protected LinkedListElement> { public: Timeout(); @@ -40,6 +41,45 @@ class Timeout final : public LinkedListElement> { 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(aKey->mReason) << 31); + } + enum { ALLOW_MEMMOVE = true }; + + private: + const TimeoutIdAndReason mValue; + }; + + class TimeoutSet : public nsDataHashtable { + public: + NS_INLINE_DECL_REFCOUNTING(TimeoutSet); + + private: + ~TimeoutSet() = default; + }; + void SetWhenOrTimeRemaining(const TimeStamp& aBaseTime, const TimeDuration& aDelay); @@ -51,6 +91,35 @@ class Timeout final : public LinkedListElement> { // 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>::getNext(); } + + void setNext(Timeout* aNext) { + return LinkedListElement>::setNext(aNext); + } + + Timeout* getPrevious() { + return LinkedListElement>::getPrevious(); + } + + void remove() { + SetTimeoutContainer(nullptr); + LinkedListElement>::remove(); + } + #ifdef MOZ_GECKO_PROFILER UniqueProfilerBacktrace TakeProfilerBacktrace() { return std::move(mCause); } #endif @@ -70,7 +139,7 @@ class Timeout final : public LinkedListElement> { // (or is cancelled, etc) TimeStamp mSubmitTime; - ~Timeout() = default; + ~Timeout() { SetTimeoutContainer(nullptr); } public: // Public member variables in this section. Please don't add to this list @@ -84,6 +153,8 @@ class Timeout final : public LinkedListElement> { // The language-specific information about the callback. RefPtr mScriptHandler; + RefPtr mTimeouts; + // Interval TimeDuration mInterval; @@ -122,6 +193,10 @@ class Timeout final : public LinkedListElement> { // True if this is a repeating/interval timer bool mIsInterval; + + protected: + friend class LinkedList>; + friend class LinkedListElement>; }; } // namespace dom diff --git a/dom/base/TimeoutManager.cpp b/dom/base/TimeoutManager.cpp index 993c53e937d4..05edc09cc164 100644 --- a/dom/base/TimeoutManager.cpp +++ b/dom/base/TimeoutManager.cpp @@ -521,9 +521,10 @@ nsresult TimeoutManager::SetTimeout(TimeoutHandler* aHandler, int32_t interval, 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( @@ -557,35 +558,31 @@ bool TimeoutManager::ClearTimeoutInternal(int32_t aTimerId, uint32_t timerId = (uint32_t)aTimerId; Timeouts& timeouts = aIsIdle ? mIdleTimeouts : mTimeouts; RefPtr& executor = aIsIdle ? mIdleExecutor : mExecutor; - bool firstTimeout = true; bool deferredDeletion = false; - bool cleared = false; - - 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)); - - 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! - } - - firstTimeout = false; + Timeout* timeout = timeouts.GetTimeout(aTimerId, aReason); + if (!timeout) { return false; - }); + } + bool firstTimeout = timeout == timeouts.GetFirst(); + + 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 (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(); + } // 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 @@ -596,7 +593,7 @@ bool TimeoutManager::ClearTimeoutInternal(int32_t aTimerId, // * If the window has become suspended then we should not start executing // Timeouts. if (!firstTimeout || deferredDeletion || mWindow.IsSuspended()) { - return cleared; + return true; } // Stop the executor and restart it at the next soonest deadline. @@ -611,7 +608,7 @@ bool TimeoutManager::ClearTimeoutInternal(int32_t aTimerId, MOZ_ALWAYS_SUCCEEDS(MaybeSchedule(nextTimeout->When())); } } - return cleared; + return true; } void TimeoutManager::RunTimeout(const TimeStamp& aNow, @@ -1088,6 +1085,7 @@ void TimeoutManager::Timeouts::Insert(Timeout* aTimeout, SortBy aSortBy) { // Now link in aTimeout after prevSibling. if (prevSibling) { + aTimeout->SetTimeoutContainer(mTimeouts); prevSibling->setNext(aTimeout); } else { InsertFront(aTimeout); diff --git a/dom/base/TimeoutManager.h b/dom/base/TimeoutManager.h index dbfeaa110d28..04b4767a442d 100644 --- a/dom/base/TimeoutManager.h +++ b/dom/base/TimeoutManager.h @@ -139,7 +139,8 @@ class TimeoutManager final { private: struct Timeouts { - explicit Timeouts(const TimeoutManager& aManager) : mManager(aManager) {} + explicit Timeouts(const TimeoutManager& aManager) + : mManager(aManager), mTimeouts(new Timeout::TimeoutSet()) {} // Insert aTimeout into the list, before all timeouts that would // fire after it, but no earlier than the last Timeout with a @@ -152,9 +153,18 @@ class TimeoutManager final { const Timeout* GetLast() const { return mTimeoutList.getLast(); } Timeout* GetLast() { return mTimeoutList.getLast(); } bool IsEmpty() const { return mTimeoutList.isEmpty(); } - void InsertFront(Timeout* aTimeout) { mTimeoutList.insertFront(aTimeout); } - void InsertBack(Timeout* aTimeout) { mTimeoutList.insertBack(aTimeout); } - void Clear() { mTimeoutList.clear(); } + 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(); + } template void ForEach(Callable c) { @@ -176,6 +186,11 @@ class TimeoutManager final { 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(). @@ -186,6 +201,11 @@ class TimeoutManager final { // 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 mTimeouts; }; // Each nsGlobalWindowInner object has a TimeoutManager member. This