Skip to content

Commit

Permalink
makeOpportunisticTaskDeferralScopeIfPossible has a wrong early exit l…
Browse files Browse the repository at this point in the history
…ogic

https://bugs.webkit.org/show_bug.cgi?id=259857

Reviewed by Mark Lam, Yusuke Suzuki and Wenson Hsieh.

We want to defer opportunistic task whenever we have a timer firing in the next 1ms,
not whenever we have a timer firing in 1ms or later. But changing the condition to
that causes ~1% Speedometer 2.1 regression in macOS Sonoma so simply delete
OpportunisticTaskDeferralScope entirely out of DOMTimer. This seems perf neutral on
macOS Sonoma and around ~0.7% Speedometer 2.1 progression in macOS Ventura.

* Source/WebCore/page/DOMTimer.cpp:
(WebCore::DOMTimer::install):
(WebCore::DOMTimer::removeById):
(WebCore::DOMTimer::fired):
(WebCore::DOMTimer::didStop):
(WebCore::DOMTimer::makeOpportunisticTaskDeferralScopeIfPossible): Deleted.
(WebCore::DOMTimer::clearOpportunisticTaskDeferralScopeIfPossible): Deleted.
* Source/WebCore/page/DOMTimer.h:
* Source/WebCore/page/Page.h:

Canonical link: https://commits.webkit.org/266680@main
  • Loading branch information
rniwa committed Aug 8, 2023
1 parent 70e3b43 commit e9d28ce
Show file tree
Hide file tree
Showing 3 changed files with 2 additions and 36 deletions.
32 changes: 1 addition & 31 deletions Source/WebCore/page/DOMTimer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,6 @@ int DOMTimer::install(ScriptExecutionContext& context, Function<void(ScriptExecu
{
Ref<DOMTimer> timer = adoptRef(*new DOMTimer(context, WTFMove(action), timeout, type));
timer->suspendIfNeeded();
timer->makeOpportunisticTaskDeferralScopeIfPossible(context);

// Keep asking for the next id until we're given one that we don't already have.
do {
Expand Down Expand Up @@ -236,8 +235,7 @@ void DOMTimer::removeById(ScriptExecutionContext& context, int timeoutId)

InspectorInstrumentation::didRemoveTimer(context, timeoutId);

if (auto timer = context.takeTimeout(timeoutId))
timer->clearOpportunisticTaskDeferralScopeIfPossible();
context.takeTimeout(timeoutId);
}

inline bool DOMTimer::isDOMTimersThrottlingEnabled(const Document& document) const
Expand Down Expand Up @@ -333,7 +331,6 @@ void DOMTimer::fired()

updateThrottlingStateIfNecessary(fireState);

clearOpportunisticTaskDeferralScopeIfPossible();
return;
}

Expand All @@ -360,8 +357,6 @@ void DOMTimer::fired()
}
nestedTimers->stopTracking();
}

clearOpportunisticTaskDeferralScopeIfPossible();
}

void DOMTimer::didStop()
Expand All @@ -370,7 +365,6 @@ void DOMTimer::didStop()
// because they can form circular references back to the ScriptExecutionContext
// which will cause a memory leak.
m_action = nullptr;
clearOpportunisticTaskDeferralScopeIfPossible();
}

void DOMTimer::updateTimerIntervalIfNecessary()
Expand Down Expand Up @@ -425,30 +419,6 @@ std::optional<MonotonicTime> DOMTimer::alignedFireTime(MonotonicTime fireTime) c
return adjustedFireTime - (adjustedFireTime % alignmentInterval) + alignmentInterval + randomizedOffset;
}

void DOMTimer::makeOpportunisticTaskDeferralScopeIfPossible(ScriptExecutionContext& context)
{
if (!m_oneShot || m_currentTimerInterval <= 1_ms) {
// FIXME: This should eventually account for repeating timers and one-shot timers that were
// previously scheduled, and are about to fire.
return;
}

RefPtr document = dynamicDowncast<Document>(context);
if (!document)
return;

auto* page = document->page();
if (!page)
return;

m_opportunisticTaskDeferralScope = page->opportunisticTaskScheduler().makeDeferralScope();
}

void DOMTimer::clearOpportunisticTaskDeferralScopeIfPossible()
{
m_opportunisticTaskDeferralScope = nullptr;
}

const char* DOMTimer::activeDOMObjectName() const
{
return "DOMTimer";
Expand Down
5 changes: 0 additions & 5 deletions Source/WebCore/page/DOMTimer.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ namespace WebCore {

class DOMTimerFireState;
class Document;
class OpportunisticTaskDeferralScope;
class ScheduledAction;

class DOMTimer final : public RefCounted<DOMTimer>, public SuspendableTimerBase, public CanMakeWeakPtr<DOMTimer> {
Expand Down Expand Up @@ -70,9 +69,6 @@ class DOMTimer final : public RefCounted<DOMTimer>, public SuspendableTimerBase,

WEBCORE_EXPORT Seconds intervalClampedToMinimum() const;

void makeOpportunisticTaskDeferralScopeIfPossible(ScriptExecutionContext&);
void clearOpportunisticTaskDeferralScopeIfPossible();

bool isDOMTimersThrottlingEnabled(const Document&) const;
void updateThrottlingStateIfNecessary(const DOMTimerFireState&);

Expand All @@ -98,7 +94,6 @@ class DOMTimer final : public RefCounted<DOMTimer>, public SuspendableTimerBase,
bool m_oneShot;
Seconds m_currentTimerInterval;
RefPtr<UserGestureToken> m_userGestureTokenToForward;
std::unique_ptr<OpportunisticTaskDeferralScope> m_opportunisticTaskDeferralScope;
};

} // namespace WebCore
1 change: 1 addition & 0 deletions Source/WebCore/page/Page.h
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ class MediaPlaybackTarget;
class MediaRecorderProvider;
class MediaSessionCoordinatorPrivate;
class ModelPlayerProvider;
class OpportunisticTaskDeferralScope;
class PageConfiguration;
class PageConsoleClient;
class PageDebuggable;
Expand Down

0 comments on commit e9d28ce

Please sign in to comment.