RJS-2852: Adding a `CallInvoker`-based scheduler for Core on React Native #6791

kraenhansen · 2024-07-16T21:02:01Z

What, How & Why?

This implements a realm::util::Scheduler for the React Native platform through the React Native CallInvoker API.

It's my current understanding that all active implementations of this API that a React Native app will construct, utilize the RuntimeSchedulerCallInvoker which in turn drop into either RuntimeScheduler_Legacy or RuntimeScheduler_Modern, both of which have invariants that we want when Core schedules calling of listeners:

Running tasks using a RuntimeExecutor (ensuring the function will be called when it is safe to do so)
Draining microtasks after execution (effectively solving Async JSI functions with promises block the event loop indefinitely facebook/react-native#33006)
Reporting uncaught JSError exceptions via handleJSError (effectively solving Refactor throwing uncaught exceptions from callbacks dispatched onto the event loop from C++ #6772 on React Native)

This fixes #2655 and #3571 and #6772 on React Native (iOS and Android) and replace the workaround introduced with #4389, #4725 and 03c30e7.

tgoyne · 2024-07-16T21:18:08Z

packages/realm/binding/jsi/react_scheduler.cpp

+        m_js_call_invoker->invokeAsync(
+            // Using low priority to avoid blocking rendering
+            facebook::react::SchedulerPriority::LowPriority,
+            // Wrapping the func in a shared_ptr to ensure it outlives the invocation
+            [func = std::make_shared<util::UniqueFunction<void()>>(std::move(func))] {
+                (*func)();
+            });
+    }


[ptr = func.release())] { util::UniqueFunction<void()>(ptr)(); }

LowPriority seems suspect to me. We do want it to be below user input, but processing refreshes promptly is pretty important.

These are the available values https://github.com/facebook/react-native/blob/8d0cbbf0e6435e0a29d1d5d218b93c9eef1a2dcd/packages/react-native/ReactCommon/callinvoker/ReactCommon/SchedulerPriority.h#L14-L20

enum class SchedulerPriority : int { ImmediatePriority = 1, UserBlockingPriority = 2, NormalPriority = 3, LowPriority = 4, IdlePriority = 5, };

Unfortunately not greatly documented 🤔
NormalPriority might be better.

Glancing at the implementation, it looks like it always runs the highest priority waiting task unless a lower priority task has hit a timeout (250ms for UserBlocking, 5s for Normal, 10s for Low). If there's some specific Normal priority thing that we know we need to not interrupt then using Low makes sense, but otherwise I would default to Normal?

[ptr = func.release())] { util::UniqueFunction<void()>(ptr)(); }

I try to refrain from c-style castings 🙈 It seems to work, but what ensure the original function owned by func ever gets destructed?

The docs for release says:

If not null, the returned pointer must be used at a later point to construct a new UniqueFunction.

There's no casts involved there. That's constructing a new UniqueFunction using the impl pointer (which takes ownership of the pointer) and then immediately invoking it. The function is then destroyed at the end of the statement as it's an unnamed temporary.

I don't know how I misread that as a cast 🙈 What about the case where the lambda is dropped and never called, that would leak the function, right? (since no new UniqueFunction takes ownership) .. although very unlikely. I like that your suggestion doesn't need to perform a heap allocation - and I've pushed an update to incorporate it 👍

This relies on CallInvoker guaranteeing that it'll invoke the function it's given exactly once; more than once and it's a use-after-free and zero times is a memory leak. This is generally a very safe thing to rely on as basically everything gets way more complicated otherwise.

tgoyne · 2024-07-16T21:19:38Z

packages/realm/binding/jsi/react_scheduler.cpp

+std::shared_ptr<Scheduler> scheduler_{};
+
+
+class ReactScheduler : public realm::util::Scheduler {


This is very minor, but this type and the global variable should be in an anonymous namespace so that the symbols are not exported.

kneth

It is a better solution that the old workaround.

Please include an entry in CHANGELOG.md - maybe under "Internal" if you don't consider it a bug fix.

kneth · 2024-07-17T07:30:33Z

packages/realm/binding/apple/RealmReactModule.mm

@@ -79,7 +79,7 @@ - (void)dealloc {
  // See https://github.com/facebook/react-native/pull/43396#issuecomment-2178586017 for context


Should we remove the comments?

kneth · 2024-07-17T07:30:57Z

packages/realm/binding/apple/RealmReactModule.mm

@@ -79,7 +79,7 @@ - (void)dealloc {
  // See https://github.com/facebook/react-native/pull/43396#issuecomment-2178586017 for context
  // If it was, we could use the enablement of "microtasks" to avoid the overhead of calling the invokeAsync on every call from C++ into JS.


Should we remove the comments?

kraenhansen · 2024-07-17T10:07:02Z

.watchmanconfig

+    ".wireit",
+    "packages/realm/.wireit",
+    "integration-tests/environments/react-native-test-app/.wireit"


I got annoyed by these being indexed by watchman.

kraenhansen added 4 commits July 16, 2024 22:47

Removing the FlushMicrotaskQueueGuard and flush_ui_workaround

b2b449d

Adding a ReactScheduler

49bc2da

Rename injectCallInvoker → createScheduler

aa65852

Updating comments

43492ab

kraenhansen added T-Internal no-changelog no-jira-ticket Skip checking the PR title for Jira reference labels Jul 16, 2024

kraenhansen self-assigned this Jul 16, 2024

cla-bot bot added the cla: yes label Jul 16, 2024

kraenhansen requested review from kneth, fealebenpae and tgoyne July 16, 2024 21:02

Ran formatting

fca3679

tgoyne reviewed Jul 16, 2024

View reviewed changes

kraenhansen added 3 commits July 17, 2024 09:16

Incorporated feedback and clarified the need for the shared_ptr

edc2c1b

Make watchman ignore .wireit dirs

7acc8b5

Updated package.lock

4ee3d51

kneth approved these changes Jul 17, 2024

View reviewed changes

kraenhansen added 2 commits July 17, 2024 09:39

Updating comments

1fa05ed

Adding a note to the changelog

5fac334

kraenhansen commented Jul 17, 2024

View reviewed changes

kraenhansen requested a review from tgoyne July 17, 2024 10:07

kraenhansen changed the title ~~Adding a CallInvoker-based scheduler for Core on React Native~~ RJS-2852: Adding a CallInvoker-based scheduler for Core on React Native Jul 17, 2024

kraenhansen removed the no-jira-ticket Skip checking the PR title for Jira reference label Jul 17, 2024

kraenhansen linked an issue Jul 17, 2024 that may be closed by this pull request

Revisit our workaround for flushing the microtask queue after calls from C++ into JS #6768

Closed

kraenhansen mentioned this pull request Jul 17, 2024

Async JSI functions with promises block the event loop indefinitely facebook/react-native#33006

Open

fealebenpae approved these changes Jul 17, 2024

View reviewed changes

kraenhansen mentioned this pull request Jul 17, 2024

Refactor throwing uncaught exceptions from callbacks dispatched onto the event loop from C++ #6772

Open

Simplified capture for lambda passed to invokeAsync

d24db8d

tgoyne approved these changes Jul 17, 2024

View reviewed changes

kraenhansen merged commit 13e70d7 into main Jul 18, 2024
27 of 29 checks passed

kraenhansen deleted the kh/rn-call-invoker-scheduler branch July 18, 2024 04:24

gvanderclay mentioned this pull request Jul 23, 2024

Realm does not build on iOS with use_frameworks! :linkage => :static as of version 12.12.0 #6808

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RJS-2852: Adding a `CallInvoker`-based scheduler for Core on React Native #6791

RJS-2852: Adding a `CallInvoker`-based scheduler for Core on React Native #6791

kraenhansen commented Jul 16, 2024 •

edited

Loading

tgoyne Jul 16, 2024

kraenhansen Jul 16, 2024 •

edited

Loading

tgoyne Jul 16, 2024

kraenhansen Jul 17, 2024 •

edited

Loading

tgoyne Jul 17, 2024

kraenhansen Jul 17, 2024 •

edited

Loading

tgoyne Jul 17, 2024

tgoyne Jul 16, 2024

kneth left a comment

kneth Jul 17, 2024

kneth Jul 17, 2024

kraenhansen Jul 17, 2024

		std::shared_ptr<Scheduler> scheduler_{};


		class ReactScheduler : public realm::util::Scheduler {

		@@ -79,7 +79,7 @@ - (void)dealloc {
		// See https://github.com/facebook/react-native/pull/43396#issuecomment-2178586017 for context

		@@ -79,7 +79,7 @@ - (void)dealloc {
		// See https://github.com/facebook/react-native/pull/43396#issuecomment-2178586017 for context
		// If it was, we could use the enablement of "microtasks" to avoid the overhead of calling the invokeAsync on every call from C++ into JS.

RJS-2852: Adding a CallInvoker-based scheduler for Core on React Native #6791

RJS-2852: Adding a CallInvoker-based scheduler for Core on React Native #6791

Conversation

kraenhansen commented Jul 16, 2024 • edited Loading

What, How & Why?

Choose a reason for hiding this comment

kraenhansen Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraenhansen Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraenhansen Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kneth left a 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

RJS-2852: Adding a `CallInvoker`-based scheduler for Core on React Native #6791

RJS-2852: Adding a `CallInvoker`-based scheduler for Core on React Native #6791

kraenhansen commented Jul 16, 2024 •

edited

Loading

kraenhansen Jul 16, 2024 •

edited

Loading

kraenhansen Jul 17, 2024 •

edited

Loading

kraenhansen Jul 17, 2024 •

edited

Loading