feat: Implement requestIdleCallback (#44636) #44759

robik · 2024-06-03T12:50:57Z

Summary:

Implements requestIdleCallback and cancelIdleCallback

Notes

Proposed implementation does yet cover all WHATWG eventloop requirements.

Deadline computation is not implemented and is polyfilled by giving each callback 50ms, rather than it being shared between other idle callbacks.
The requested callbacks are called with lowest priority by the scheduler as of now, but the execution is not as described in the standard.

Changelog:

[GENERAL] [ADDED] - Implemented requestIdleCallback and cancelIdleCallback

Test Plan:

analysis-bot · 2024-06-03T13:21:55Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	20,431,243	+65,684
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	23,635,260	+65,637
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: e78742a
Branch: main

rubennorte

Thanks for working on this, Robert! This is a great starting point. Please take a look at my comments and let me know if you have any questions.

packages/react-native/Libraries/Core/setUpTimers.js

packages/react-native/src/private/webapis/microtasks/specs/NativeMicrotasks.js

packages/react-native/ReactCommon/react/runtime/TimerManager.cpp

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

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

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

packages/react-native/ReactCommon/react/nativemodule/microtasks/NativeMicrotasks.h

packages/react-native/ReactCommon/react/nativemodule/microtasks/IdleDeadline.cpp

robik · 2024-06-05T14:53:09Z

Also, what is the preferred approach for writing tests for this? C++ and JS unit tests? Are any existing ones I can base them on? 🤔

rubennorte · 2024-06-05T18:57:49Z

Also, what is the preferred approach for writing tests for this? C++ and JS unit tests? Are any existing ones I can base them on? 🤔

You can just test the native bits using a similar test to the one we use for RuntimeScheduler. The JS layer is so thin that's probably not worth testing it. We're in discussions to have proper integration tests covering both JS and C++, and we'd be able to test this e2e, but we're not there yet.

rubennorte

Looks great! Please address the comments and take a look at the failing CI jobs.

packages/react-native/ReactCommon/react/nativemodule/idlecallbacks/IdleTaskRef.h

packages/react-native/ReactCommon/react/nativemodule/idlecallbacks/IdleTaskRef.cpp

packages/react-native/ReactCommon/react/nativemodule/idlecallbacks/NativeIdleCallbacks.cpp

rubennorte · 2024-06-11T14:57:57Z

packages/react-native/ReactCommon/react/nativemodule/idlecallbacks/NativeIdleCallbacks.cpp

+            size_t /* unused */
+        ) noexcept -> jsi::Value {
+            auto didTimeout = args[0].getBool();
+            // Below is a partial solution and does not comply to WHATWG event-loop standards


How is this not spec compliant? The spec doesn't require this to align with the frame duration and it allows implementations to specific a static deadline.

Sorry I just re-read the PR summary and I understood. I forgot about this problem when we changed the design. I think it's fine because we check getShouldYield which would return true if there's any other higher priority task pending execution. It might be good to explain this in the comment.

Not sure if that is related (or is) frame alignment, but we have separate and fixed deadlines for each idleTask. This might be problematic if there are lots of idleTasks which might impact the performance.
Unless I misunderstood or missed anything, the deadline should be computed at RuntimeScheduler (or Scheduler::EventPipeConclusion perhaps?), reduced by requestAnimationFrame and timers run time and then read from IdleCallbacks TurboModule. I postponed this to reduce changes in RuntimeScheduler API as the initial design had this right 😅.

So the reason why we have a fixed time for all idle callbacks on Web is that we need to yield the execution of JS at regular intervals to process rendering. That's done in the main thread in JS, so this is never an issue for us. In the case that we need to process a user interaction, we'd be scheduling a task with a higher priority in the scheduler, which would make getShouldYield return true, so we'd return 0 the next time the current idle callback checks the time remaining, and we'd stop running idle callbacks after that. Essentially, it's the same behavior as on Web but with a different implementation.

Would be good to clarify this behavior in a comment here :)

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

facebook-github-bot · 2024-06-11T15:05:51Z

@rubennorte has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

packages/rn-tester/js/examples/Timer/TimerExample.js

cortinico

Let's try to avoid having to add yet another .so

packages/react-native/ReactCommon/react/nativemodule/idlecallbacks/CMakeLists.txt

rubennorte · 2024-06-11T18:25:45Z

Let's try to avoid having to add yet another .so

@cortinico do we have any so merging mechanism in OSS builds? I think we're moving away from forcing static on libraries internally and not sure if we should change it in this case (Robert was following the same example we have for the microtask native module).

cortinico · 2024-06-12T09:09:07Z

Let's try to avoid having to add yet another .so

@cortinico do we have any so merging mechanism in OSS builds? I think we're moving away from forcing static on libraries internally and not sure if we should change it in this case (Robert was following the same example we have for the microtask native module).

Yes we do. Unless you have a valid reason for making this library a dynamic library, it should default to be static. Copying from other libraries is sadly going to push us more work down the line. Most of the libs we don't explicitly load with SoLoader.loadLibrary() can be static and we should be aiming at that direction.

packages/react-native/ReactCommon/react/nativemodule/idlecallbacks/IdleTaskRef.cpp

rubennorte · 2024-06-12T09:23:50Z

packages/react-native/ReactCommon/react/nativemodule/idlecallbacks/NativeIdleCallbacks.cpp

+    }
+  );
+
+  auto timeoutDuration = std::chrono::duration<double, std::milli>(timeout.value_or(0.0));


We should use the default param in scheduleIdleTask instead of 0 when timeout isn't set, because otherwise we'd be using the priority of normal tasks instead of the priority of idle tasks.

Since this function is for idle callbacks only, maybe it would be better to make the timeout max(timeout, timeoutForPriotity(SchedulerPriority::Idle)) in the scheduleIdleTask function body? This way we don't allow queueing normal tasks with this and the API contract is more clear. WDYT? 🤔

timeoutForPriotity(SchedulerPriority::Idle) is infinite, so that max will always give you this value. The moment you specify a timeout you're kinda saying, after this consider this to be a normal task, so I think it should be fine. If anything, we should do something like expirationTime = now() + timeout + timeoutForPriority(SchedulerPriority::Normal), so worst case scenario (timeout = 0) this is scheduled as a normal task. That should be handled in RuntimeScheduler_Modern though, and we should still honor the defaults here if the timeout wasn't defined.

With this code, we still have the problem of requestIdleCallback(cb) being scheduled as a normal priority task, instead of as a regular idle priority task without a timeout.

packages/react-native/ReactCommon/react/nativemodule/idlecallbacks/NativeIdleCallbacks.cpp

packages/react-native/ReactCommon/react/nativemodule/idlecallbacks/NativeIdleCallbacks.h

packages/react-native/ReactCommon/react/nativemodule/microtasks/NativeMicrotasks.cpp

packages/react-native/ReactCommon/react/nativemodule/microtasks/NativeMicrotasks.h

facebook-github-bot · 2024-06-12T10:23:10Z

@rubennorte has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

packages/react-native/Libraries/Core/setUpTimers.js

rubennorte · 2024-06-12T11:16:36Z

packages/react-native/ReactCommon/react/nativemodule/idlecallbacks/NativeIdleCallbacks.cpp

+    timeout = options.value().timeout;
+  }
+
+  auto userCallback = std::make_shared<jsi::Function>(std::move(callback));


This looks very sketchy. We shouldn't have shared pointers to jsi::Function because it prevents some of the safety mechanisms defined in that class. We should define the parameter in this function as SyncCallback instead, which allows us to move it into the lambda.

I've just checked the code and it looks like I cannot use the RawCallback&& variant. This is because it uses the std::function under the hood, which is required to be copyable, so we cannot move capture the callback into the lambda.
I've pushed the version that uses SyncCallback but still uses shared_ptr

rubennorte · 2024-06-12T11:17:14Z

packages/react-native/ReactCommon/react/nativemodule/idlecallbacks/NativeIdleCallbacks.cpp

+
+  auto userCallback = std::make_shared<jsi::Function>(std::move(callback));
+
+  auto wrappedCallback = jsi::Function::createFromHostFunction(


This doesn't need to be a host function, because we're not calling it directly but passing it to the runtime scheduler. I can just be an std::function that calls the SyncCallback that we receive via params.

Oh right, I just (wrongly) assumed that I need access to didTimeout argument at the moment of writing

rubennorte · 2024-06-12T11:18:38Z

packages/react-native/ReactCommon/react/nativemodule/idlecallbacks/NativeIdleCallbacks.cpp

+      const jsi::Value* args,
+      size_t /* unused */
+    ) noexcept -> jsi::Value {
+      auto didTimeout = args[0].getBool();


I just realized this is wrong. If we have something like this:

requestIdleCallback(() => { /* ... */ }, 10);

And this executes after 1 second, didTimeout would be false because, as an implementation detail, we actually used the timeout of the normal priority tasks, which is 5 seconds. We should check the current time here and use that to determine if it exceeded the timeout instead.

rubennorte · 2024-06-13T22:16:46Z

packages/react-native/ReactCommon/react/nativemodule/idlecallbacks/NativeIdleCallbacks.cpp

+    timeout = options.value().timeout;
+  }
+
+  auto userCallbackShared = std::make_shared<SyncCallback<void(jsi::Object)>>(std::move(userCallback));


Let me try to find a way to pass this without the shared pointer tomorrow.

rubennorte · 2024-06-13T22:17:22Z

packages/react-native/ReactCommon/react/nativemodule/idlecallbacks/NativeIdleCallbacks.cpp

+
+  auto wrappedCallback = [runtimeScheduler, userCallbackShared](
+    jsi::Runtime& runtime
+  ) noexcept -> void {


This shouldn't have noexcept because the user callback that you're calling here can throw.

rubennorte · 2024-06-13T22:17:56Z

packages/react-native/ReactCommon/react/nativemodule/idlecallbacks/NativeIdleCallbacks.cpp

+    auto deadline = runtimeScheduler->now() + 50ms;
+
+    jsi::Object idleDeadline {runtime};
+    idleDeadline.setProperty(runtime, "didTimeout", false);


We should check the time and set this property accordingly.

rubennorte · 2024-06-13T22:19:34Z

packages/react-native/ReactCommon/react/nativemodule/microtasks/NativeMicrotasks.h

@@ -7,6 +7,8 @@

 #pragma once

+#include <memory>
+#include <unordered_map>


They're still here 🤔

facebook-github-bot · 2024-06-14T13:50:42Z

@rubennorte has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

rubennorte · 2024-06-14T15:56:53Z

packages/react-native/ReactCommon/react/nativemodule/idlecallbacks/NativeIdleCallbacks.cpp

+        }
+
+        auto now = runtimeScheduler->now();
+        double remainingTime = std::chrono::duration_cast<std::chrono::milliseconds>(deadline - now).count();


For future reference, this deadline here is garbage because it's passed as a reference to this host function and the value is being cleared from the stack of makeTimeRemainingFunction, which made timeRemaining() always return 0. The fix is to copy the value to the lambda.

I'll fix this before landing.

the feedback was addressed already

github-actions · 2024-06-17T11:25:17Z

This pull request was successfully merged by @robik in abfadc6.

^{When will my fix make it into a release? | How to file a pick request?}

facebook-github-bot · 2024-06-17T11:25:18Z

@rubennorte merged this pull request in abfadc6.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 3, 2024

robik changed the title ~~feat: Implement requestIdleCallback~~ feat: Implement requestIdleCallback (#44636) Jun 3, 2024

robik mentioned this pull request Jun 3, 2024

Implement requestIdleCallback and cancelIdleCallback on the new architecture #44636

Closed

rubennorte requested changes Jun 4, 2024

View reviewed changes

robik force-pushed the robik/request-idle-callback branch from cbed54b to 2e4c6bf Compare June 11, 2024 10:07

rubennorte requested changes Jun 11, 2024

View reviewed changes

rubennorte reviewed Jun 11, 2024

View reviewed changes

packages/rn-tester/js/examples/Timer/TimerExample.js Outdated Show resolved Hide resolved

cortinico previously requested changes Jun 11, 2024

View reviewed changes

packages/react-native/ReactCommon/react/nativemodule/idlecallbacks/CMakeLists.txt Outdated Show resolved Hide resolved

robik force-pushed the robik/request-idle-callback branch 2 times, most recently from 1c6a1eb to e4d89a7 Compare June 12, 2024 08:42

rubennorte reviewed Jun 12, 2024

View reviewed changes

packages/react-native/Libraries/Core/setUpTimers.js Outdated Show resolved Hide resolved

rubennorte reviewed Jun 12, 2024

View reviewed changes

packages/react-native/Libraries/Core/setUpTimers.js Outdated Show resolved Hide resolved

rubennorte reviewed Jun 12, 2024

View reviewed changes

rubennorte reviewed Jun 13, 2024

View reviewed changes

rubennorte reviewed Jun 14, 2024

View reviewed changes

rubennorte force-pushed the robik/request-idle-callback branch from ff37b8d to 288335a Compare June 14, 2024 16:23

feat: Implement requestIdleCallback

cc9179d

rubennorte force-pushed the robik/request-idle-callback branch from 288335a to cc9179d Compare June 14, 2024 17:25

rubennorte marked this pull request as ready for review June 14, 2024 18:10

rubennorte approved these changes Jun 14, 2024

View reviewed changes

facebook-github-bot closed this in abfadc6 Jun 17, 2024

facebook-github-bot added the Merged This PR has been merged. label Jun 17, 2024

github-actions bot added the Merged This PR has been merged. label Jun 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement requestIdleCallback (#44636) #44759

feat: Implement requestIdleCallback (#44636) #44759

robik commented Jun 3, 2024 •

edited

Loading

analysis-bot commented Jun 3, 2024 •

edited

Loading

rubennorte left a comment

robik commented Jun 5, 2024

rubennorte commented Jun 5, 2024

rubennorte left a comment

rubennorte Jun 11, 2024

rubennorte Jun 11, 2024

robik Jun 11, 2024 •

edited

Loading

rubennorte Jun 12, 2024 •

edited

Loading

facebook-github-bot commented Jun 11, 2024

cortinico left a comment

rubennorte commented Jun 11, 2024

cortinico commented Jun 12, 2024

rubennorte Jun 12, 2024

robik Jun 13, 2024 •

edited

Loading

rubennorte Jun 13, 2024 •

edited

Loading

facebook-github-bot commented Jun 12, 2024

rubennorte Jun 12, 2024

robik Jun 13, 2024 •

edited

Loading

rubennorte Jun 12, 2024

robik Jun 13, 2024

rubennorte Jun 12, 2024

rubennorte Jun 13, 2024

rubennorte Jun 13, 2024

rubennorte Jun 13, 2024

rubennorte Jun 13, 2024

facebook-github-bot commented Jun 14, 2024

rubennorte Jun 14, 2024

github-actions bot commented Jun 17, 2024

facebook-github-bot commented Jun 17, 2024


		auto userCallback = std::make_shared<jsi::Function>(std::move(callback));

		auto wrappedCallback = jsi::Function::createFromHostFunction(

feat: Implement requestIdleCallback (#44636) #44759

feat: Implement requestIdleCallback (#44636) #44759

Conversation

robik commented Jun 3, 2024 • edited Loading

Summary:

Notes

Changelog:

Test Plan:

analysis-bot commented Jun 3, 2024 • edited Loading

rubennorte left a comment

Choose a reason for hiding this comment

robik commented Jun 5, 2024

rubennorte commented Jun 5, 2024

rubennorte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robik Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

rubennorte Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

facebook-github-bot commented Jun 11, 2024

cortinico left a comment

Choose a reason for hiding this comment

rubennorte commented Jun 11, 2024

cortinico commented Jun 12, 2024

Choose a reason for hiding this comment

robik Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

rubennorte Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

facebook-github-bot commented Jun 12, 2024

Choose a reason for hiding this comment

robik Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jun 14, 2024

Choose a reason for hiding this comment

github-actions bot commented Jun 17, 2024

facebook-github-bot commented Jun 17, 2024

robik commented Jun 3, 2024 •

edited

Loading

analysis-bot commented Jun 3, 2024 •

edited

Loading

robik Jun 11, 2024 •

edited

Loading

rubennorte Jun 12, 2024 •

edited

Loading

robik Jun 13, 2024 •

edited

Loading

rubennorte Jun 13, 2024 •

edited

Loading

robik Jun 13, 2024 •

edited

Loading