Skip to content

Commit

Permalink
deadline-max-rAF.html fails
Browse files Browse the repository at this point in the history
https://bugs.webkit.org/show_bug.cgi?id=260183

Reviewed by Antti Koivisto.

This PR fixes a number of issues to make requestidlecallback/deadline-max-rAF.html pass.

1. Due to rounding errors / loss of precision, IdleDeadline::timeRemaining could return a time
that is off by 1ms or 20us, which ever is currently used for the time precision. We now ensure
this off-by-one-time-unit issue does not affect idle callbacks by always subtracting the same
amount from the remaining time. Since timeRemaining is defined to be an upper cap, this is fine.

2. didFinishRenderingUpdate was erroneously removing Page from m_pagesWithRenderingOpportunity
when the next rendering update has been scheduled within the current rendering update. Moved
the removal of the entry to at the beginning of Page::updateRendering so that subsequent
scheduling of rendering update within this rendering update does not end up getting cleared.

3. shouldEndIdlePeriod would return true whenever m_pagesWithRenderingOpportunity contains any
entry, which would effectively disable idle callbacks when there is any pending rendering update.
We now return true only if the next rendering update is happening within the next 4ms.

Unfortunately, this PR doesn't remove PASS FAIL expectation from deadline-max-rAF.html because
the test fails intermittently on the stress bots, and adding any logging to help debug the issue
will make the test no longer flaky, not providing means to debug it. Instead, this PR introduces
a new layout test to test the same scenario.

* LayoutTests/imported/w3c/web-platform-tests/requestidlecallback/deadline-max-rAF-dynamic.html:
* LayoutTests/imported/w3c/web-platform-tests/requestidlecallback/deadline-max-rAF-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/requestidlecallback/deadline-max-rAF.html:
* LayoutTests/platform/ios-wk2/TestExpectations:
* LayoutTests/platform/mac-wk2/TestExpectations:
* LayoutTests/requestidlecallback/requestidlecallback-deadline-cap-by-rendering-update-expected.txt: Added.
* LayoutTests/requestidlecallback/requestidlecallback-deadline-cap-by-rendering-update.html: Added.

* Source/WebCore/dom/IdleDeadline.cpp:
(WebCore::IdleDeadline::timeRemaining const): Fixed off-by-one error (1).

* Source/WebCore/dom/WindowEventLoop.cpp:
(WebCore::WindowEventLoop::didStartRenderingUpdate): Renamed from didFinishRenderingUpdate.
(WebCore::WindowEventLoop::opportunisticallyRunIdleCallbacks):
(WebCore::WindowEventLoop::shouldEndIdlePeriod): Now takes the current timestamp and only returns
true if the next rendering update is happening within the next 4ms instead of whenever (3).
(WebCore::WindowEventLoop::computeIdleDeadline):
(WebCore::WindowEventLoop::nextRenderingTime const): Added.

* Source/WebCore/dom/WindowEventLoop.h:

* Source/WebCore/page/Page.cpp:
(WebCore::Page::updateRendering): Moved the call to didStartRenderingUpdate (previously named
didFinishRenderingUpdate) here so that rendering update scheduled within this rendering update
doesn't get cleared at the end of the current rendering update (2).
(WebCore::Page::renderingUpdateCompleted):

* Source/WebCore/page/Performance.cpp:
(WebCore::Performance::timeResolution): Added.

* Source/WebCore/page/Performance.h:

Canonical link: https://commits.webkit.org/267008@main
  • Loading branch information
rniwa committed Aug 17, 2023
1 parent 3372a68 commit 7090728
Show file tree
Hide file tree
Showing 11 changed files with 92 additions and 25 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@ Test of requestIdleCallback deadline behavior
The test can pass accidentally as idle deadlines have a maximum but they can always be shorter. It runs multiple times to expose potential failures.


FAIL Check that the deadline is less than 16ms when there is a pending animation frame. assert_less_than_equal: expected a number less than or equal to 16.666666666666668 but got 115
PASS Check that the deadline is less than 16ms when there is a pending animation frame.

Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,16 @@
<script src="resources/ric-utils.js"></script>
<script>

function runRAFLoop()
{
requestAnimationFrame(runRAFLoop);
}

promise_test(async done => {
runRAFLoop();
for (let i = 0; i < getRICRetryCount(); ++i) {
requestAnimationFrame(() => {})
assert_less_than_equal(await getDeadlineForNextIdleCallback(), getPendingRenderDeadlineCap())
}

}, 'Check that the deadline is less than 16ms when there is a pending animation frame.');
</script>
<h1>Test of requestIdleCallback deadline behavior</h1>
Expand Down
2 changes: 1 addition & 1 deletion LayoutTests/platform/ios-wk2/TestExpectations
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ storage/filesystemaccess/ [ Pass ]
requestidlecallback [ Pass ]
imported/w3c/web-platform-tests/requestidlecallback [ Pass ]
imported/w3c/web-platform-tests/requestidlecallback/deadline-max-rAF.html [ Pass Failure ]
http/tests/eventloop/idle-callback-not-affected-by-timers-in-cross-origin.html [ Skip ]
http/tests/eventloop/idle-callback-not-affected-by-timers-in-cross-origin.html [ Pass ]

#//////////////////////////////////////////////////////////////////////////////////////////
# End platform-specific directories.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
This tests that the deadline can be reached.

On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".


PASS idleDeadline.timeRemaining() <= preferredRenderingUpdateInterval is true
PASS didRunIdleCallback is true
PASS successfullyParsed is true

TEST COMPLETE

Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
<!DOCTYPE html><!-- webkit-test-runner [ RequestIdleCallbackEnabled=true ] -->
<html>
<body>
<script src="../resources/js-test.js"></script>
<script>

description('This tests that the deadline can be reached.');

jsTestIsAsync = true;

let didRunIdleCallback = false;
onload = () => {
function runRAFLoop() {
requestAnimationFrame(runRAFLoop);
}
runRAFLoop();
requestIdleCallback((idleDeadline) => {
didRunIdleCallback = true;
window.idleDeadline = idleDeadline;
window.preferredRenderingUpdateInterval = internals.preferredRenderingUpdateInterval();
shouldBeTrue('idleDeadline.timeRemaining() <= preferredRenderingUpdateInterval');
if (idleDeadline.timeRemaining() > preferredRenderingUpdateInterval)
console.log(idleDeadline.timeRemaining() + ' >' + preferredRenderingUpdateInterval);
});
setTimeout(() => {
shouldBeTrue('didRunIdleCallback');
finishJSTest();
}, 200);
}

</script>
</body>
</html>
7 changes: 4 additions & 3 deletions Source/WebCore/dom/IdleDeadline.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,10 @@ DOMHighResTimeStamp IdleDeadline::timeRemaining(Document& document) const
RefPtr window { document.domWindow() };
if (!window || m_didTimeout == DidTimeout::Yes)
return 0;
auto duration = document.windowEventLoop().computeIdleDeadline() - MonotonicTime::now();
auto remainingTime = window->performance().reduceTimeResolution(duration);
return remainingTime < 0_s ? 0 : remainingTime.milliseconds();
auto& performance = window->performance();
auto now = performance.now();
auto deadline = performance.relativeTimeFromTimeOriginInReducedResolution(document.windowEventLoop().computeIdleDeadline() - performance.timeResolution());
return deadline < now ? 0 : deadline - now;
}

} // namespace WebCore
31 changes: 20 additions & 11 deletions Source/WebCore/dom/WindowEventLoop.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -125,17 +125,18 @@ void WindowEventLoop::didScheduleRenderingUpdate(Page& page, MonotonicTime nextR
m_pagesWithRenderingOpportunity.set(page, nextRenderingUpdate);
}

void WindowEventLoop::didFinishRenderingUpdate(Page& page)
void WindowEventLoop::didStartRenderingUpdate(Page& page)
{
m_pagesWithRenderingOpportunity.remove(page);
}

void WindowEventLoop::opportunisticallyRunIdleCallbacks()
{
if (shouldEndIdlePeriod())
auto now = MonotonicTime::now();
if (shouldEndIdlePeriod(now))
return;

m_lastIdlePeriodStartTime = MonotonicTime::now();
m_lastIdlePeriodStartTime = now;

forEachAssociatedContext([&](ScriptExecutionContext& context) {
RefPtr document = dynamicDowncast<Document>(context);
Expand All @@ -148,13 +149,14 @@ void WindowEventLoop::opportunisticallyRunIdleCallbacks()
});
}

bool WindowEventLoop::shouldEndIdlePeriod()
bool WindowEventLoop::shouldEndIdlePeriod(MonotonicTime now)
{
if (hasTasksForFullyActiveDocument())
return true;
if (!microtaskQueue().isEmpty())
return true;
if (!m_pagesWithRenderingOpportunity.isEmptyIgnoringNullReferences())
auto renderingTime = nextRenderingTime();
if (renderingTime && *renderingTime < now + IdleCallbackDurationExpectation)
return true;
return false;
}
Expand All @@ -167,16 +169,23 @@ MonotonicTime WindowEventLoop::computeIdleDeadline()
if (timerTime && *timerTime < minTime)
minTime = *timerTime;

if (!m_pagesWithRenderingOpportunity.isEmptyIgnoringNullReferences()) {
for (auto it : m_pagesWithRenderingOpportunity) {
if (it.value < minTime)
minTime = it.value;
}
}
auto renderingTime = nextRenderingTime();
if (renderingTime && *renderingTime < minTime)
minTime = *renderingTime;

return minTime;
}

std::optional<MonotonicTime> WindowEventLoop::nextRenderingTime() const
{
std::optional<MonotonicTime> result;
for (auto it : m_pagesWithRenderingOpportunity) {
if (!result || it.value < *result)
result = it.value;
}
return result;
}

void WindowEventLoop::didReachTimeToRun()
{
Ref protectedThis { *this }; // Executing tasks may remove the last reference to this WindowEventLoop.
Expand Down
5 changes: 3 additions & 2 deletions Source/WebCore/dom/WindowEventLoop.h
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,9 @@ class WindowEventLoop final : public EventLoop {
CustomElementQueue& backupElementQueue();

void didScheduleRenderingUpdate(Page&, MonotonicTime);
void didFinishRenderingUpdate(Page&);
void didStartRenderingUpdate(Page&);
void opportunisticallyRunIdleCallbacks();
bool shouldEndIdlePeriod();
bool shouldEndIdlePeriod(MonotonicTime);
MonotonicTime computeIdleDeadline();

WEBCORE_EXPORT static void breakToAllowRenderingUpdate();
Expand All @@ -71,6 +71,7 @@ class WindowEventLoop final : public EventLoop {
bool isContextThread() const final;
MicrotaskQueue& microtaskQueue() final;

std::optional<MonotonicTime> nextRenderingTime() const;
void didReachTimeToRun();

String m_agentClusterKey;
Expand Down
12 changes: 7 additions & 5 deletions Source/WebCore/page/Page.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1691,9 +1691,10 @@ void Page::scheduleRenderingUpdateInternal()

auto now = MonotonicTime::now();
forEachWindowEventLoop([&](WindowEventLoop& windowEventLoop) {
auto nextRenderingUpdateTime = m_lastRenderingUpdateTimestamp + preferredRenderingUpdateInterval();
auto interval = preferredRenderingUpdateInterval();
auto nextRenderingUpdateTime = m_lastRenderingUpdateTimestamp + interval;
if (nextRenderingUpdateTime < now)
nextRenderingUpdateTime = now + preferredRenderingUpdateInterval();
nextRenderingUpdateTime = now + interval;
windowEventLoop.didScheduleRenderingUpdate(*this, nextRenderingUpdateTime);
});
}
Expand Down Expand Up @@ -1752,6 +1753,10 @@ void Page::updateRendering()

m_lastRenderingUpdateTimestamp = MonotonicTime::now();

forEachWindowEventLoop([&](WindowEventLoop& eventLoop) {
eventLoop.didStartRenderingUpdate(*this);
});

bool isSVGImagePage = chrome().client().isSVGImageChromeClient();
if (!isSVGImagePage)
tracePoint(RenderingUpdateStart);
Expand Down Expand Up @@ -2044,9 +2049,6 @@ void Page::renderingUpdateCompleted()

if (!isUtilityPage()) {
auto nextRenderingUpdate = m_lastRenderingUpdateTimestamp + preferredRenderingUpdateInterval();
forEachWindowEventLoop([&](WindowEventLoop& eventLoop) {
eventLoop.didFinishRenderingUpdate(*this);
});
m_opportunisticTaskScheduler->reschedule(nextRenderingUpdate);
}
}
Expand Down
5 changes: 5 additions & 0 deletions Source/WebCore/page/Performance.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,11 @@ void Performance::allowHighPrecisionTime()
timePrecision = highTimePrecision;
}

Seconds Performance::timeResolution()
{
return timePrecision;
}

DOMHighResTimeStamp Performance::relativeTimeFromTimeOriginInReducedResolution(MonotonicTime timestamp) const
{
Seconds seconds = timestamp - m_timeOrigin;
Expand Down
1 change: 1 addition & 0 deletions Source/WebCore/page/Performance.h
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ class Performance final : public RefCounted<Performance>, public ContextDestruct
void unregisterPerformanceObserver(PerformanceObserver&);

static void allowHighPrecisionTime();
static Seconds timeResolution();
static Seconds reduceTimeResolution(Seconds);

DOMHighResTimeStamp relativeTimeFromTimeOriginInReducedResolution(MonotonicTime) const;
Expand Down

0 comments on commit 7090728

Please sign in to comment.