From 0ca3ed87f3424a333c815c9d57b17f133dd19eab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Norte?= Date: Fri, 12 Jul 2024 15:51:51 -0700 Subject: [PATCH] Fix incorrect application of idle priority in RuntimeScheduler (#45408) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/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 --- .../RuntimeScheduler_Modern.cpp | 2 +- .../runtimescheduler/SchedulerPriorityUtils.h | 8 +-- .../tests/RuntimeSchedulerTest.cpp | 65 +++++++++++++++++++ .../tests/SchedulerPriorityTest.cpp | 4 +- 4 files changed, 72 insertions(+), 7 deletions(-) diff --git a/packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler_Modern.cpp b/packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler_Modern.cpp index d8b0de2dec0..6684f62c7e2 100644 --- a/packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler_Modern.cpp +++ b/packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler_Modern.cpp @@ -24,7 +24,7 @@ std::chrono::milliseconds getResolvedTimeoutForIdleTask( timeoutForSchedulerPriority(SchedulerPriority::IdlePriority) ? timeoutForSchedulerPriority(SchedulerPriority::LowPriority) + customTimeout - : timeoutForSchedulerPriority(SchedulerPriority::IdlePriority); + : customTimeout; } } // namespace diff --git a/packages/react-native/ReactCommon/react/renderer/runtimescheduler/SchedulerPriorityUtils.h b/packages/react-native/ReactCommon/react/renderer/runtimescheduler/SchedulerPriorityUtils.h index ca4e11d8307..ccba5e6c7b7 100644 --- a/packages/react-native/ReactCommon/react/renderer/runtimescheduler/SchedulerPriorityUtils.h +++ b/packages/react-native/ReactCommon/react/renderer/runtimescheduler/SchedulerPriorityUtils.h @@ -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); } } diff --git a/packages/react-native/ReactCommon/react/renderer/runtimescheduler/tests/RuntimeSchedulerTest.cpp b/packages/react-native/ReactCommon/react/renderer/runtimescheduler/tests/RuntimeSchedulerTest.cpp index 7ec0e2d38ea..27bf5852ecd 100644 --- a/packages/react-native/ReactCommon/react/renderer/runtimescheduler/tests/RuntimeSchedulerTest.cpp +++ b/packages/react-native/ReactCommon/react/renderer/runtimescheduler/tests/RuntimeSchedulerTest.cpp @@ -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*/) { diff --git a/packages/react-native/ReactCommon/react/renderer/runtimescheduler/tests/SchedulerPriorityTest.cpp b/packages/react-native/ReactCommon/react/renderer/runtimescheduler/tests/SchedulerPriorityTest.cpp index ade776cc2b1..d1807ad8146 100644 --- a/packages/react-native/ReactCommon/react/renderer/runtimescheduler/tests/SchedulerPriorityTest.cpp +++ b/packages/react-native/ReactCommon/react/renderer/runtimescheduler/tests/SchedulerPriorityTest.cpp @@ -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)); @@ -43,5 +43,5 @@ TEST(SchedulerPriorityTest, timeoutForSchedulerPriority) { std::chrono::seconds(10)); EXPECT_EQ( timeoutForSchedulerPriority(SchedulerPriority::IdlePriority), - std::chrono::milliseconds::max()); + std::chrono::minutes(5)); }