Skip to content

Commit

Permalink
Bug 1857618 - Ensure normal tasks get enough time to run, r=mstange
Browse files Browse the repository at this point in the history 
The basic idea is to check if there are pending tasks also right after RefreshDriver tick has been handled, not only before tick.
But in this new case we can't predict how much time is needed for other tasks, so just a bit of time is given.

The patch tweaks also other things: CanDoCatchUpTick (which is used by catch-up ticks and FinishedWaitingForTransaction) should
return false if the active timer is blocked because of mSuspendVsyncPriorityTicksUntil.
And to make it easier to check whether driver is blocked, mSuspendVsyncPriorityTicksUntil is changed to rely on local time, not the time stamp
from vsync.

The low priority notify is changed so that it uses member variables from the driver and in case it does trigger a tick, it uses the most recent skipped
values.

Differential Revision: https://phabricator.services.mozilla.com/D193409
  • Loading branch information
Olli Pettay authored and Olli Pettay committed Nov 20, 2023
1 parent 5b633e7 commit 6975460
Showing 1 changed file with 69 additions and 41 deletions.
110 changes: 69 additions & 41 deletions layout/base/nsRefreshDriver.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
#include "mozilla/InputTaskManager.h"
#include "mozilla/IntegerRange.h"
#include "mozilla/PresShell.h"
#include "mozilla/VsyncTaskManager.h"
#include "nsITimer.h"
#include "nsLayoutUtils.h"
#include "nsPresContext.h"
Expand Down Expand Up @@ -218,6 +219,7 @@ class RefreshDriverTimer {

TimeStamp MostRecentRefresh() const { return mLastFireTime; }
VsyncId MostRecentRefreshVsyncId() const { return mLastFireId; }
virtual bool IsBlocked() { return false; }

virtual TimeDuration GetTimerRate() = 0;

Expand Down Expand Up @@ -485,6 +487,12 @@ class VsyncRefreshDriverTimer : public RefreshDriverTimer {
: TimeDuration::FromMilliseconds(1000.0 / 60.0);
}

bool IsBlocked() override {
return !mSuspendVsyncPriorityTicksUntil.IsNull() &&
mSuspendVsyncPriorityTicksUntil > TimeStamp::Now() &&
ShouldGiveNonVsyncTasksMoreTime();
}

private:
// RefreshDriverVsyncObserver redirects vsync notifications to the main thread
// and calls VsyncRefreshDriverTimer::NotifyVsyncOnMainThread on it. It also
Expand Down Expand Up @@ -619,7 +627,8 @@ class VsyncRefreshDriverTimer : public RefreshDriverTimer {
mLastTickStart(TimeStamp::Now()),
mLastIdleTaskCount(0),
mLastRunOutOfMTTasksCount(0),
mProcessedVsync(true) {
mProcessedVsync(true),
mHasPendingLowPrioTask(false) {
mVsyncObserver = new RefreshDriverVsyncObserver(this);
}

Expand All @@ -638,15 +647,23 @@ class VsyncRefreshDriverTimer : public RefreshDriverTimer {
mVsyncObserver = nullptr;
}

bool ShouldGiveNonVsyncTasksMoreTime() {
bool ShouldGiveNonVsyncTasksMoreTime(bool aCheckOnlyNewPendingTasks = false) {
TaskController* taskController = TaskController::Get();
IdleTaskManager* idleTaskManager = taskController->GetIdleTaskManager();
VsyncTaskManager* vsyncTaskManager = VsyncTaskManager::Get();

// Note, pendingTaskCount includes also all the pending idle tasks.
// Note, pendingTaskCount includes also all the pending idle and vsync
// tasks.
uint64_t pendingTaskCount =
taskController->PendingMainthreadTaskCountIncludingSuspended();
uint64_t pendingIdleTaskCount = idleTaskManager->PendingTaskCount();
MOZ_ASSERT(pendingTaskCount >= pendingIdleTaskCount);
uint64_t pendingVsyncTaskCount = vsyncTaskManager->PendingTaskCount();
if (!(pendingTaskCount > (pendingIdleTaskCount + pendingVsyncTaskCount))) {
return false;
}
if (aCheckOnlyNewPendingTasks) {
return true;
}

uint64_t idleTaskCount = idleTaskManager->ProcessedTaskCount();

Expand All @@ -656,7 +673,6 @@ class VsyncRefreshDriverTimer : public RefreshDriverTimer {
// In the parent process RunOutOfMTTasksCount() is less meaningful
// because some of the tasks run through AppShell.
return mLastIdleTaskCount == idleTaskCount &&
pendingTaskCount > pendingIdleTaskCount &&
(taskController->RunOutOfMTTasksCount() ==
mLastRunOutOfMTTasksCount ||
XRE_IsParentProcess());
Expand All @@ -668,24 +684,28 @@ class VsyncRefreshDriverTimer : public RefreshDriverTimer {
mRecentVsync = aVsyncEvent.mTime;
mRecentVsyncId = aVsyncEvent.mId;
if (!mSuspendVsyncPriorityTicksUntil.IsNull() &&
mSuspendVsyncPriorityTicksUntil > aVsyncEvent.mTime) {
mSuspendVsyncPriorityTicksUntil > TimeStamp::Now()) {
if (ShouldGiveNonVsyncTasksMoreTime()) {
if (!IsAnyToplevelContentPageLoading()) {
// If pages aren't loading and there aren't other tasks to run,
// trigger the pending vsync notification.
static bool sHasPendingLowPrioTask = false;
if (!sHasPendingLowPrioTask) {
sHasPendingLowPrioTask = true;
mPendingVsync = mRecentVsync;
mPendingVsyncId = mRecentVsyncId;
if (!mHasPendingLowPrioTask) {
mHasPendingLowPrioTask = true;
NS_DispatchToMainThreadQueue(
NS_NewRunnableFunction(
"NotifyVsyncOnMainThread[low priority]",
[self = RefPtr{this}, event = aVsyncEvent]() {
sHasPendingLowPrioTask = false;
if (self->mRecentVsync == event.mTime &&
self->mRecentVsyncId == event.mId &&
[self = RefPtr{this}]() {
self->mHasPendingLowPrioTask = false;
if (self->mRecentVsync == self->mPendingVsync &&
self->mRecentVsyncId == self->mPendingVsyncId &&
!self->ShouldGiveNonVsyncTasksMoreTime()) {
self->mSuspendVsyncPriorityTicksUntil = TimeStamp();
self->NotifyVsyncOnMainThread(event);
self->NotifyVsyncOnMainThread({self->mPendingVsyncId,
self->mPendingVsync,
/* unused */
TimeStamp()});
}
}),
EventQueuePriority::Low);
Expand Down Expand Up @@ -850,38 +870,38 @@ class VsyncRefreshDriverTimer : public RefreshDriverTimer {

// Let also non-RefreshDriver code to run at least for awhile if we have
// a mVsyncRefreshDriverTimer.
// Always give a tiny bit, 1% of the vsync interval, time outside the
// Always give a tiny bit, 5% of the vsync interval, time outside the
// tick
// In case there are both normal tasks and RefreshDrivers are doing
// work, mSuspendVsyncPriorityTicksUntil will be set to a timestamp in the
// future where the period between the previous tick start
// (aVsyncTimestamp) and the next tick needs to be at least the amount of
// work normal tasks and RefreshDrivers did together (minus short grace
// (mostRecentTickStart) and the next tick needs to be at least the amount
// of work normal tasks and RefreshDrivers did together (minus short grace
// period).
TimeDuration gracePeriod = rate / int64_t(100);

if (shouldGiveNonVSyncTasksMoreTime) {
if (!mLastTickEnd.IsNull() && XRE_IsContentProcess() &&
// For RefreshDriver scheduling during page load there is currently
// idle priority based setup.
// XXX Consider to remove the page load specific code paths.
!IsAnyToplevelContentPageLoading()) {
// In case normal tasks are doing lots of work, we still want to paint
// every now and then, so only at maximum 4 * rate of work is counted
// here.
// If we're giving extra time for tasks outside a tick, try to
// ensure the next vsync after that period is handled, so subtract
// a grace period.
TimeDuration timeForOutsideTick = clamped(
tickStart - mLastTickEnd - gracePeriod, TimeDuration(), rate * 4);
mSuspendVsyncPriorityTicksUntil = aVsyncTimestamp + timeForOutsideTick +
(tickEnd - mostRecentTickStart);
} else {
mSuspendVsyncPriorityTicksUntil =
aVsyncTimestamp + gracePeriod + (tickEnd - mostRecentTickStart);
}
TimeDuration gracePeriod = rate / int64_t(20);

if (shouldGiveNonVSyncTasksMoreTime && !mLastTickEnd.IsNull() &&
XRE_IsContentProcess() &&
// For RefreshDriver scheduling during page load there is currently
// idle priority based setup.
// XXX Consider to remove the page load specific code paths.
!IsAnyToplevelContentPageLoading()) {
// In case normal tasks are doing lots of work, we still want to paint
// every now and then, so only at maximum 4 * rate of work is counted
// here.
// If we're giving extra time for tasks outside a tick, try to
// ensure the next vsync after that period is handled, so subtract
// a grace period.
TimeDuration timeForOutsideTick = clamped(
tickStart - mLastTickEnd - gracePeriod, gracePeriod, rate * 4);
mSuspendVsyncPriorityTicksUntil = tickEnd + timeForOutsideTick;
} else if (ShouldGiveNonVsyncTasksMoreTime(true)) {
// We've got some new tasks, give them some extra time.
// This handles also the case when mLastTickEnd.IsNull() above and we
// should give some more time for non-vsync tasks.
mSuspendVsyncPriorityTicksUntil = tickEnd + gracePeriod;
} else {
mSuspendVsyncPriorityTicksUntil = aVsyncTimestamp + gracePeriod;
mSuspendVsyncPriorityTicksUntil = mostRecentTickStart + gracePeriod;
}

mLastIdleTaskCount =
Expand Down Expand Up @@ -968,9 +988,13 @@ class VsyncRefreshDriverTimer : public RefreshDriverTimer {
TimeStamp mLastProcessedTick;
// mSuspendVsyncPriorityTicksUntil is used to block too high refresh rate in
// case the main thread has also other non-idle tasks to process.
// The timestamp is effectively mLastProcessedTick + some duration.
// The timestamp is effectively mLastTickEnd + some duration.
TimeStamp mSuspendVsyncPriorityTicksUntil;
bool mProcessedVsync;

TimeStamp mPendingVsync;
VsyncId mPendingVsyncId;
bool mHasPendingLowPrioTask;
}; // VsyncRefreshDriverTimer

/**
Expand Down Expand Up @@ -1661,6 +1685,10 @@ bool nsRefreshDriver::CanDoCatchUpTick() {
return false;
}

if (mActiveTimer->IsBlocked()) {
return false;
}

if (mTickVsyncTime.IsNull()) {
// Don't try to run a catch-up tick before there has been at least one
// normal tick. The catch-up tick could negatively affect page load
Expand Down

0 comments on commit 6975460

Please sign in to comment.