Tidy the bridgeless runtime executor plumbing - #58325
Open
javache wants to merge 2 commits into
Open
Conversation
Summary: In bridgeless, native reaches JS by two routes that end in the same `RuntimeScheduler` queue but get there differently. `callFunctionOnModule` goes through the instance's `BufferedRuntimeExecutor`; the `CallInvoker` goes straight to `scheduleTask`. The CallInvoker therefore skips the buffer entirely and can reach the runtime while a module call issued earlier is still parked, unflushed, because the bundle is mid-evaluation. Native code that issues both cannot rely on the order it issued them in, and `Task` is a min-heap on `now() + timeout(priority)` with no insertion tiebreak, so equal priorities do not settle it either. Gives the two channels the same buffering. `BufferedRuntimeExecutor` gains a priority-carrying `execute`, so work routed through it keeps the scheduler priority it was submitted with instead of collapsing to the executor default, and buffered work from both overloads stays in one submission-ordered stream. `BufferedCallInvoker` sits on that executor and becomes the bridgeless `jsCallInvoker` on Android, iOS and macOS. `invokeSync` deliberately keeps going straight to the scheduler: a synchronous call cannot wait for a flush that only happens once the bundle has run. Behind `enableBufferedCallInvoker`, default true. `ReactInstance` picks between the buffered invoker and the existing `RuntimeSchedulerCallInvoker` in one place, so the platform call sites are identical either way and the change is revertible at runtime — it moves when native-issued async work first reaches JS during startup, which is the intended contract but affects every native module. One lifetime hazard this surfaces, worth knowing about beyond this diff: `BufferedRuntimeExecutor` reaches the scheduler through a raw pointer captured at construction, which is safe only while the owning instance is alive. A CallInvoker is routinely held across instance teardown, so `BufferedCallInvoker` guards every async dispatch on a weak reference to the scheduler and drops the work when it has expired — the same contract `RuntimeSchedulerCallInvoker` has. Without that guard this reliably segfaults on a reload. Changelog: [General][Changed] - Async `CallInvoker` work is now buffered alongside callable module calls, so it no longer runs before the JS bundle has finished evaluating Differential Revision: D118456662
Summary: Follow-ups to the buffered CallInvoker, kept out of that diff so the behavioural change stays reviewable on its own. No behaviour change intended here. - `BufferedRuntimeExecutor` gains `asRuntimeExecutor()` and `asWeakRuntimeExecutor()`. RuntimeExecutor is a std::function, so callers cannot hand it the object and instead each hand-rolled the same capturing lambda; the ownership question now has one answer per lifetime policy. The weak form is what `getBufferedRuntimeExecutor` needs, which must not keep the instance alive. - `BufferedWork` becomes private. It was only ever an implementation detail, and being public put its fields in the C++ API snapshot. - The buffer holds a `std::vector` sorted once at flush rather than a `std::priority_queue`. Indices are handed out before the lock, so arrival order can differ from submission order and something has to restore it — but a heap is the expensive way. `std::priority_queue::top()` returns a const reference, so every flushed item copied its `std::function` on the way out; that copy is now a move, and nothing sifts on the way in. - `runtimeExecutorThatGoesThroughRuntimeScheduler` was a second copy of `getUnbufferedRuntimeExecutor()`; it now calls it. - `RuntimeSchedulerCallInvoker` is marked deprecated. Its remaining users are migrated: `ReactCxxPlatform` holds a `ReactInstance` and can use `createJSCallInvoker()`, and four other files only `#include`d it without ever constructing one. The single remaining use is the flag-off branch, which is suppressed locally and goes when the flag is cleaned up. - Drops a null check on `bufferedRuntimeExecutor_` in `callFunctionOnModule` that the constructor makes unreachable. Changelog: [Internal] Differential Revision: D118616045
meta-cla
Bot
added
the
CLA Signed
|
@javache has exported this pull request. If you are a Meta employee, you can view the originating Diff in D118616045. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
Follow-ups to the buffered CallInvoker, kept out of that diff so the behavioural
change stays reviewable on its own. No behaviour change intended here.
BufferedRuntimeExecutorgainsasRuntimeExecutor()andasWeakRuntimeExecutor(). RuntimeExecutor is a std::function, so callerscannot hand it the object and instead each hand-rolled the same capturing
lambda; the ownership question now has one answer per lifetime policy. The
weak form is what
getBufferedRuntimeExecutorneeds, which must not keep theinstance alive.
BufferedWorkbecomes private. It was only ever an implementation detail, andbeing public put its fields in the C++ API snapshot.
std::vectorsorted once at flush rather than astd::priority_queue. Indices are handed out before the lock, so arrivalorder can differ from submission order and something has to restore it — but a
heap is the expensive way.
std::priority_queue::top()returns a constreference, so every flushed item copied its
std::functionon the way out;that copy is now a move, and nothing sifts on the way in.
runtimeExecutorThatGoesThroughRuntimeSchedulerwas a second copy ofgetUnbufferedRuntimeExecutor(); it now calls it.RuntimeSchedulerCallInvokeris marked deprecated. Its remaining users aremigrated:
ReactCxxPlatformholds aReactInstanceand can usecreateJSCallInvoker(), and four other files only#included it without everconstructing one. The single remaining use is the flag-off branch, which is
suppressed locally and goes when the flag is cleaned up.
bufferedRuntimeExecutor_incallFunctionOnModulethat the constructor makes unreachable.
Changelog:
[Internal]
Differential Revision: D118616045