Skip to content

Commit 0ca3ed8

Browse files
rubennortefacebook-github-bot
authored andcommitted
Fix incorrect application of idle priority in RuntimeScheduler (facebook#45408)
Summary: Pull Request resolved: facebook#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
1 parent 9d8e52b commit 0ca3ed8

File tree

4 files changed

+72
-7
lines changed

4 files changed

+72
-7
lines changed

packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler_Modern.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ std::chrono::milliseconds getResolvedTimeoutForIdleTask(
2424
timeoutForSchedulerPriority(SchedulerPriority::IdlePriority)
2525
? timeoutForSchedulerPriority(SchedulerPriority::LowPriority) +
2626
customTimeout
27-
: timeoutForSchedulerPriority(SchedulerPriority::IdlePriority);
27+
: customTimeout;
2828
}
2929
} // namespace
3030

packages/react-native/ReactCommon/react/renderer/runtimescheduler/SchedulerPriorityUtils.h

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -41,15 +41,15 @@ static inline std::chrono::milliseconds timeoutForSchedulerPriority(
4141
SchedulerPriority schedulerPriority) noexcept {
4242
switch (schedulerPriority) {
4343
case SchedulerPriority::ImmediatePriority:
44-
return std::chrono::milliseconds(-1);
44+
return std::chrono::milliseconds(0);
4545
case SchedulerPriority::UserBlockingPriority:
4646
return std::chrono::milliseconds(250);
4747
case SchedulerPriority::NormalPriority:
48-
return std::chrono::milliseconds(5000);
48+
return std::chrono::seconds(5);
4949
case SchedulerPriority::LowPriority:
50-
return std::chrono::milliseconds(10'000);
50+
return std::chrono::seconds(10);
5151
case SchedulerPriority::IdlePriority:
52-
return std::chrono::milliseconds::max();
52+
return std::chrono::minutes(5);
5353
}
5454
}
5555

packages/react-native/ReactCommon/react/renderer/runtimescheduler/tests/RuntimeSchedulerTest.cpp

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -332,6 +332,71 @@ TEST_P(RuntimeSchedulerTest, scheduleTwoTasksWithDifferentPriorities) {
332332
EXPECT_EQ(hostFunctionCallCount_, 2);
333333
}
334334

335+
TEST_P(RuntimeSchedulerTest, scheduleTwoTasksWithAllPriorities) {
336+
uint idlePriorityTaskCallOrder = 0;
337+
auto idlePriTask = createHostFunctionFromLambda(
338+
[this, &idlePriorityTaskCallOrder](bool /*unused*/) {
339+
idlePriorityTaskCallOrder = hostFunctionCallCount_;
340+
return jsi::Value::undefined();
341+
});
342+
343+
uint lowPriorityTaskCallOrder = 0;
344+
auto lowPriTask = createHostFunctionFromLambda(
345+
[this, &lowPriorityTaskCallOrder](bool /*unused*/) {
346+
lowPriorityTaskCallOrder = hostFunctionCallCount_;
347+
return jsi::Value::undefined();
348+
});
349+
350+
uint normalPriorityTaskCallOrder = 0;
351+
auto normalPriTask = createHostFunctionFromLambda(
352+
[this, &normalPriorityTaskCallOrder](bool /*unused*/) {
353+
normalPriorityTaskCallOrder = hostFunctionCallCount_;
354+
return jsi::Value::undefined();
355+
});
356+
357+
uint userBlockingPriorityTaskCallOrder = 0;
358+
auto userBlockingPriTask = createHostFunctionFromLambda(
359+
[this, &userBlockingPriorityTaskCallOrder](bool /*unused*/) {
360+
userBlockingPriorityTaskCallOrder = hostFunctionCallCount_;
361+
return jsi::Value::undefined();
362+
});
363+
364+
uint immediatePriorityTaskCallOrder = 0;
365+
auto immediatePriTask = createHostFunctionFromLambda(
366+
[this, &immediatePriorityTaskCallOrder](bool /*unused*/) {
367+
immediatePriorityTaskCallOrder = hostFunctionCallCount_;
368+
return jsi::Value::undefined();
369+
});
370+
371+
runtimeScheduler_->scheduleTask(
372+
SchedulerPriority::IdlePriority, std::move(idlePriTask));
373+
runtimeScheduler_->scheduleTask(
374+
SchedulerPriority::LowPriority, std::move(lowPriTask));
375+
runtimeScheduler_->scheduleTask(
376+
SchedulerPriority::NormalPriority, std::move(normalPriTask));
377+
runtimeScheduler_->scheduleTask(
378+
SchedulerPriority::UserBlockingPriority, std::move(userBlockingPriTask));
379+
runtimeScheduler_->scheduleTask(
380+
SchedulerPriority::ImmediatePriority, std::move(immediatePriTask));
381+
382+
EXPECT_EQ(idlePriorityTaskCallOrder, 0);
383+
EXPECT_EQ(lowPriorityTaskCallOrder, 0);
384+
EXPECT_EQ(normalPriorityTaskCallOrder, 0);
385+
EXPECT_EQ(userBlockingPriorityTaskCallOrder, 0);
386+
EXPECT_EQ(immediatePriorityTaskCallOrder, 0);
387+
EXPECT_EQ(stubQueue_->size(), 1);
388+
389+
stubQueue_->tick();
390+
391+
EXPECT_EQ(idlePriorityTaskCallOrder, 5);
392+
EXPECT_EQ(lowPriorityTaskCallOrder, 4);
393+
EXPECT_EQ(normalPriorityTaskCallOrder, 3);
394+
EXPECT_EQ(userBlockingPriorityTaskCallOrder, 2);
395+
EXPECT_EQ(immediatePriorityTaskCallOrder, 1);
396+
EXPECT_EQ(stubQueue_->size(), 0);
397+
EXPECT_EQ(hostFunctionCallCount_, 5);
398+
}
399+
335400
TEST_P(RuntimeSchedulerTest, cancelTask) {
336401
bool didRunTask = false;
337402
auto callback = createHostFunctionFromLambda([&didRunTask](bool /*unused*/) {

packages/react-native/ReactCommon/react/renderer/runtimescheduler/tests/SchedulerPriorityTest.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ TEST(SchedulerPriorityTest, serialize) {
3131
TEST(SchedulerPriorityTest, timeoutForSchedulerPriority) {
3232
EXPECT_EQ(
3333
timeoutForSchedulerPriority(SchedulerPriority::ImmediatePriority),
34-
std::chrono::milliseconds(-1));
34+
std::chrono::milliseconds(0));
3535
EXPECT_EQ(
3636
timeoutForSchedulerPriority(SchedulerPriority::UserBlockingPriority),
3737
std::chrono::milliseconds(250));
@@ -43,5 +43,5 @@ TEST(SchedulerPriorityTest, timeoutForSchedulerPriority) {
4343
std::chrono::seconds(10));
4444
EXPECT_EQ(
4545
timeoutForSchedulerPriority(SchedulerPriority::IdlePriority),
46-
std::chrono::milliseconds::max());
46+
std::chrono::minutes(5));
4747
}

0 commit comments

Comments
 (0)