From 2373a0003afc8a25e1aecb367b0674ceefe61d74 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 1e24294dbbc..912ee4baf41 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)); }