-
Notifications
You must be signed in to change notification settings - Fork 24.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RuntimeTarget refactor - Add this
-scoped async executors to all Targets
#43015
Closed
Conversation
This file contains 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
facebook-github-bot
added
CLA Signed
This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
p: Facebook
Partner: Facebook
Partner
labels
Feb 14, 2024
This pull request was exported from Phabricator. Differential Revision: D53356953 |
motiz88
force-pushed
the
export-D53356953
branch
from
February 14, 2024 13:09
8f650dd
to
bc03805
Compare
This pull request was exported from Phabricator. Differential Revision: D53356953 |
Summary: Changelog: [Internal] I'm refactoring the way the Runtime concept works in the modern CDP backend to bring it in line with the Page/Instance concepts. Overall, this will let us: * Integrate with engines that require us to instantiate a shared Target-like object (e.g. Hermes AsyncDebuggingAPI) in addition to an per-session Agent-like object. * Access JSI in a CDP context (both at target setup/teardown time and during a CDP session) to implement our own engine-agnostic functionality (`console` interception, `Runtime.addBinding`, etc). * Manage CDP execution contexts natively in RN, and (down the line) enable first-class debugging support for multiple Runtimes in an Instance. The core diffs in this stack will: * ~~Introduce a `RuntimeTarget` class similar to `{Page,Instance}Target`. ~~ * ~~Make runtime registration explicit (`InstanceTarget::registerRuntime` similar to `PageTarget::registerInstance`). ~~ * ~~Rename the existing `RuntimeAgent` interface to `RuntimeAgentDelegate`.~~ * ~~Create a new concrete `RuntimeAgent` class similar to `{Page,Instance}Agent`.~~ * Provide `RuntimeTarget` and `RuntimeAgent` with primitives for safe JSI access, namely a `RuntimeExecutor` for scheduling work on the JS thread. *← This diff* * We'll likely develop a similar mechanism for scheduling work on the "main" thread from the JS thread, for when we need to do more than just send a CDP message (which we can already do with the thread-safe `FrontendChannel`) in response to a JS event. ## Architecture diagrams Before this stack: https://pxl.cl/4h7m0 After this stack: https://pxl.cl/4h7m7 Reviewed By: hoxyq Differential Revision: D53266710
…gets Summary: Changelog: [Internal] # This diff 1. Provides all Targets with an `executorFromThis()` method, which can be used from within a Target to access a *`this`-scoped main thread executor* = a `std::function` that will execute a callback asynchronously iff the current Target isn't destroyed first. 2. Refactors how (all) Target objects are constructed and retained, from a plain constructor to `static shared_ptr create()`. This is because `executorFromThis()` relies internally on `enable_shared_from_this` plus two-phase construction to populate the executor. 3. Creates utilities for deriving scoped executors from other executors and `shared_ptr`s. The concept is very much like `RuntimeExecutor` in reverse: the facebook#1 use case is moving from the JS thread back to the main thread - where "main thread" is defined loosely as "anywhere it's legal to call methods on Target/Agent objects, access session state, etc". The actual dispatching mechanism is left up to the owner of each `PageTarget` object; for now we only have an iOS integration, where we use `RCTExecuteOnMainQueue`. Coupling the ownership/lifetime semantics with task scheduling is helpful, because it avoids the footgun of accidentally/nondeterministically moving `shared_ptr`s (and destructors!) to a different thread/queue . # This stack I'm refactoring the way the Runtime concept works in the modern CDP backend to bring it in line with the Page/Instance concepts. Overall, this will let us: * Integrate with engines that require us to instantiate a shared Target-like object (e.g. Hermes AsyncDebuggingAPI) in addition to an per-session Agent-like object. * Access JSI in a CDP context (both at target setup/teardown time and during a CDP session) to implement our own engine-agnostic functionality (`console` interception, `Runtime.addBinding`, etc). * Manage CDP execution contexts natively in RN, and (down the line) enable first-class debugging support for multiple Runtimes in an Instance. The core diffs in this stack: * ~~Introduce a `RuntimeTarget` class similar to `{Page,Instance}Target`. ~~ * ~~Make runtime registration explicit (`InstanceTarget::registerRuntime` similar to `PageTarget::registerInstance`). ~~ * ~~Rename the existing `RuntimeAgent` interface to `RuntimeAgentDelegate`.~~ * ~~Create a new concrete `RuntimeAgent` class similar to `{Page,Instance}Agent`.~~ * ~~Provide `RuntimeTarget` and `RuntimeAgent` with primitives for safe JSI access, namely a `RuntimeExecutor` for scheduling work on the JS thread.~~ * Provide RuntimeTarget with mechanism for scheduling work on the "main" thread from the JS thread, for when we need to do more than just send a CDP message (which we can already do with the thread-safe `FrontendChannel`) in response to a JS event. *← This diff* ## Architecture diagrams Before this stack: https://pxl.cl/4h7m0 After this stack: https://pxl.cl/4h7m7 Reviewed By: hoxyq Differential Revision: D53356953
motiz88
force-pushed
the
export-D53356953
branch
from
February 14, 2024 13:43
bc03805
to
98fcbb6
Compare
This pull request was exported from Phabricator. Differential Revision: D53356953 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
CLA Signed
This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
fb-exported
Merged
This PR has been merged.
p: Facebook
Partner: Facebook
Partner
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:
Changelog: [Internal]
This diff
executorFromThis()
method, which can be used from within a Target to access athis
-scoped main thread executor = astd::function
that will execute a callback asynchronously iff the current Target isn't destroyed first.static shared_ptr create()
. This is becauseexecutorFromThis()
relies internally onenable_shared_from_this
plus two-phase construction to populate the executor.shared_ptr
s.The concept is very much like
RuntimeExecutor
in reverse: the #1 use case is moving from the JS thread back to the main thread - where "main thread" is defined loosely as "anywhere it's legal to call methods on Target/Agent objects, access session state, etc". The actual dispatching mechanism is left up to the owner of eachPageTarget
object; for now we only have an iOS integration, where we useRCTExecuteOnMainQueue
.Coupling the ownership/lifetime semantics with task scheduling is helpful, because it avoids the footgun of accidentally/nondeterministically moving
shared_ptr
s (and destructors!) to a different thread/queue .This stack
I'm refactoring the way the Runtime concept works in the modern CDP backend to bring it in line with the Page/Instance concepts.
Overall, this will let us:
console
interception,Runtime.addBinding
, etc).The core diffs in this stack:
RuntimeTarget
class similar to{Page,Instance}Target
. ~~InstanceTarget::registerRuntime
similar toPageTarget::registerInstance
). ~~Rename the existingRuntimeAgent
interface toRuntimeAgentDelegate
.Create a new concreteRuntimeAgent
class similar to{Page,Instance}Agent
.ProvideRuntimeTarget
andRuntimeAgent
with primitives for safe JSI access, namely aRuntimeExecutor
for scheduling work on the JS thread.FrontendChannel
) in response to a JS event. ← This diffArchitecture diagrams
Before this stack:
https://pxl.cl/4h7m0
After this stack:
https://pxl.cl/4h7m7
Reviewed By: hoxyq
Differential Revision: D53356953