Skip to content

Commit

Permalink
Fix incorrect application of idle priority in RuntimeScheduler (#45408)
Browse files Browse the repository at this point in the history
Summary:
Pull Request resolved: #45408

Changelog: [General][Fixed] Fixed prioritization of idle priority tasks

We recently found out that idle priority tasks were never scheduled with the lowest priority possible. We didn't realize before because idle priority tasks weren't used, but now they are via `requestIdleCallback` and other mechanisms.

The problem was that the timeout for idle priority tasks was `std::chrono:milliseconds::max()`, and we compute the expiration time adding that to the current time. Doing that operation is always guaranteed to overflow, and the resulting expiration time was always in the past, resulting in the task having higher priority than any other tasks with any other priorities.

Instead of using `max()` we can just use a sensible value for idle priorities. In this case, 5 minutes should be more than enough.

Reviewed By: sammy-SC

Differential Revision: D59679513

fbshipit-source-id: 6c0f9e275818737ce804f05615c01f7ea6c126ab
  • Loading branch information
rubennorte authored and facebook-github-bot committed Jul 12, 2024
1 parent 9d8e52b commit 0ca3ed8
Show file tree
Hide file tree
Showing 4 changed files with 72 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ std::chrono::milliseconds getResolvedTimeoutForIdleTask(
timeoutForSchedulerPriority(SchedulerPriority::IdlePriority)
? timeoutForSchedulerPriority(SchedulerPriority::LowPriority) +
customTimeout
: timeoutForSchedulerPriority(SchedulerPriority::IdlePriority);
: customTimeout;
}
} // namespace

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,15 +41,15 @@ static inline std::chrono::milliseconds timeoutForSchedulerPriority(
SchedulerPriority schedulerPriority) noexcept {
switch (schedulerPriority) {
case SchedulerPriority::ImmediatePriority:
return std::chrono::milliseconds(-1);
return std::chrono::milliseconds(0);
case SchedulerPriority::UserBlockingPriority:
return std::chrono::milliseconds(250);
case SchedulerPriority::NormalPriority:
return std::chrono::milliseconds(5000);
return std::chrono::seconds(5);
case SchedulerPriority::LowPriority:
return std::chrono::milliseconds(10'000);
return std::chrono::seconds(10);
case SchedulerPriority::IdlePriority:
return std::chrono::milliseconds::max();
return std::chrono::minutes(5);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,71 @@ TEST_P(RuntimeSchedulerTest, scheduleTwoTasksWithDifferentPriorities) {
EXPECT_EQ(hostFunctionCallCount_, 2);
}

TEST_P(RuntimeSchedulerTest, scheduleTwoTasksWithAllPriorities) {
uint idlePriorityTaskCallOrder = 0;
auto idlePriTask = createHostFunctionFromLambda(
[this, &idlePriorityTaskCallOrder](bool /*unused*/) {
idlePriorityTaskCallOrder = hostFunctionCallCount_;
return jsi::Value::undefined();
});

uint lowPriorityTaskCallOrder = 0;
auto lowPriTask = createHostFunctionFromLambda(
[this, &lowPriorityTaskCallOrder](bool /*unused*/) {
lowPriorityTaskCallOrder = hostFunctionCallCount_;
return jsi::Value::undefined();
});

uint normalPriorityTaskCallOrder = 0;
auto normalPriTask = createHostFunctionFromLambda(
[this, &normalPriorityTaskCallOrder](bool /*unused*/) {
normalPriorityTaskCallOrder = hostFunctionCallCount_;
return jsi::Value::undefined();
});

uint userBlockingPriorityTaskCallOrder = 0;
auto userBlockingPriTask = createHostFunctionFromLambda(
[this, &userBlockingPriorityTaskCallOrder](bool /*unused*/) {
userBlockingPriorityTaskCallOrder = hostFunctionCallCount_;
return jsi::Value::undefined();
});

uint immediatePriorityTaskCallOrder = 0;
auto immediatePriTask = createHostFunctionFromLambda(
[this, &immediatePriorityTaskCallOrder](bool /*unused*/) {
immediatePriorityTaskCallOrder = hostFunctionCallCount_;
return jsi::Value::undefined();
});

runtimeScheduler_->scheduleTask(
SchedulerPriority::IdlePriority, std::move(idlePriTask));
runtimeScheduler_->scheduleTask(
SchedulerPriority::LowPriority, std::move(lowPriTask));
runtimeScheduler_->scheduleTask(
SchedulerPriority::NormalPriority, std::move(normalPriTask));
runtimeScheduler_->scheduleTask(
SchedulerPriority::UserBlockingPriority, std::move(userBlockingPriTask));
runtimeScheduler_->scheduleTask(
SchedulerPriority::ImmediatePriority, std::move(immediatePriTask));

EXPECT_EQ(idlePriorityTaskCallOrder, 0);
EXPECT_EQ(lowPriorityTaskCallOrder, 0);
EXPECT_EQ(normalPriorityTaskCallOrder, 0);
EXPECT_EQ(userBlockingPriorityTaskCallOrder, 0);
EXPECT_EQ(immediatePriorityTaskCallOrder, 0);
EXPECT_EQ(stubQueue_->size(), 1);

stubQueue_->tick();

EXPECT_EQ(idlePriorityTaskCallOrder, 5);
EXPECT_EQ(lowPriorityTaskCallOrder, 4);
EXPECT_EQ(normalPriorityTaskCallOrder, 3);
EXPECT_EQ(userBlockingPriorityTaskCallOrder, 2);
EXPECT_EQ(immediatePriorityTaskCallOrder, 1);
EXPECT_EQ(stubQueue_->size(), 0);
EXPECT_EQ(hostFunctionCallCount_, 5);
}

TEST_P(RuntimeSchedulerTest, cancelTask) {
bool didRunTask = false;
auto callback = createHostFunctionFromLambda([&didRunTask](bool /*unused*/) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ TEST(SchedulerPriorityTest, serialize) {
TEST(SchedulerPriorityTest, timeoutForSchedulerPriority) {
EXPECT_EQ(
timeoutForSchedulerPriority(SchedulerPriority::ImmediatePriority),
std::chrono::milliseconds(-1));
std::chrono::milliseconds(0));
EXPECT_EQ(
timeoutForSchedulerPriority(SchedulerPriority::UserBlockingPriority),
std::chrono::milliseconds(250));
Expand All @@ -43,5 +43,5 @@ TEST(SchedulerPriorityTest, timeoutForSchedulerPriority) {
std::chrono::seconds(10));
EXPECT_EQ(
timeoutForSchedulerPriority(SchedulerPriority::IdlePriority),
std::chrono::milliseconds::max());
std::chrono::minutes(5));
}

0 comments on commit 0ca3ed8

Please sign in to comment.