Implement "tickleJS" for Hermes #39289
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 pull request was exported from Phabricator. Differential Revision: D48952581 |
|
This pull request was exported from Phabricator. Differential Revision: D48952581 |
luluwu2032
pushed a commit
to luluwu2032/react-native
that referenced
this pull request
Sep 5, 2023
Summary: Pull Request resolved: facebook#39289 "tickleJS" is a function in RN Hermes Debugger, we need to implement it for Bridgeless for feature correctness and keep parity with Bridge. Changelog: [General][Changed] - Implement "tickleJS" for Hermes Differential Revision: D48952581 fbshipit-source-id: ce83dfd81c8b1258f462c7325998db7586073f31
luluwu2032
force-pushed
the
export-D48952581
branch
from
d713e72
to
ea7d908
Compare
|
This pull request was exported from Phabricator. Differential Revision: D48952581 |
luluwu2032
pushed a commit
to luluwu2032/react-native
that referenced
this pull request
Sep 5, 2023
Summary: Pull Request resolved: facebook#39289 "tickleJS" is a function in RN Hermes Debugger, we need to implement it for Bridgeless for feature correctness and keep parity with Bridge. Changelog: [General][Changed] - Implement "tickleJS" for Hermes Differential Revision: D48952581 fbshipit-source-id: 321855443381761ec0dc709c6f5ff64d49369bda
luluwu2032
force-pushed
the
export-D48952581
branch
from
ea7d908
to
d78f631
Compare
luluwu2032
force-pushed
the
export-D48952581
branch
from
d78f631
to
7aeacc4
Compare
|
This pull request was exported from Phabricator. Differential Revision: D48952581 |
luluwu2032
added a commit
to luluwu2032/react-native
that referenced
this pull request
Sep 13, 2023
Summary: "tickleJS" is a function in RN Hermes Debugger, we need to implement it for Bridgeless for feature correctness and keep parity with Bridge. Changelog: [Internal] - Implement "tickleJS" for Hermes in Bridgeless mode ### Summary of discussion: **Goal of this diff**: Implement tickleJS for Hermes debugger with Bridgeless, here is Bridge's implementation https://www.internalfb.com/code/fbsource/[7058fb4dae4f]/xplat/ReactNative/react/jsi/HermesExecutorFactory.cpp?lines=45-57 **The key problem to solve:** As you can see from Bridge's implementation, we need to dispatch the js call of "__tickleJs" to dedicated JS thread queue for thread safety, and it did cause a crash for me locally without dispatching to JS thread queue (see P818439922). For Bridgeless passing the JS message queue directly works, but then we would naturally want ask if RuntimeExecutor/RuntimeScheduler is a better choice because: 1, It should also work 2, It's the common way to run js code in native with Bridgeless 3, Can avoid potential race condition caused by dispatching to JS message queue directly 4, We're planning to expose RuntimeExecutor/RuntimeScheduler or a similar abstraction anyway. So the argument here is which solution should we take? **Why we decide to move forward with the JS msq solution?** - Time constraint: we need to ship this before 0.73 - After discussion there hasn't been any major concern over dispatching to JS msq directly so far - How to expose RuntimeExecutor/RuntimeScheduler is a bigger topic which need further research, we don't want to hold this diff for it For detailed discussion see the post https://fb.workplace.com/groups/react.technologies.discussions/permalink/3615044702060575/ Reviewed By: javache Differential Revision: D48952581
Base commit: 5a926c5 |
Summary: "tickleJS" is a function in RN Hermes Debugger, we need to implement it for Bridgeless for feature correctness and keep parity with Bridge. Changelog: [Internal] - Implement "tickleJS" for Hermes in Bridgeless mode ### Summary of discussion: **Goal of this diff**: Implement tickleJS for Hermes debugger with Bridgeless, here is Bridge's implementation https://www.internalfb.com/code/fbsource/[7058fb4dae4f]/xplat/ReactNative/react/jsi/HermesExecutorFactory.cpp?lines=45-57 **The key problem to solve:** As you can see from Bridge's implementation, we need to dispatch the js call of "__tickleJs" to dedicated JS thread queue for thread safety, and it did cause a crash for me locally without dispatching to JS thread queue (see P818439922). For Bridgeless passing the JS message queue directly works, but then we would naturally want ask if RuntimeExecutor/RuntimeScheduler is a better choice because: 1, It should also work 2, It's the common way to run js code in native with Bridgeless 3, Can avoid potential race condition caused by dispatching to JS message queue directly 4, We're planning to expose RuntimeExecutor/RuntimeScheduler or a similar abstraction anyway. So the argument here is which solution should we take? **Why we decide to move forward with the JS msq solution?** - Time constraint: we need to ship this before 0.73 - After discussion there hasn't been any major concern over dispatching to JS msq directly so far - How to expose RuntimeExecutor/RuntimeScheduler is a bigger topic which need further research, we don't want to hold this diff for it For detailed discussion see the post https://fb.workplace.com/groups/react.technologies.discussions/permalink/3615044702060575/ Reviewed By: javache Differential Revision: D48952581
luluwu2032
force-pushed
the
export-D48952581
branch
from
7aeacc4
to
bb8021b
Compare
|
This pull request was exported from Phabricator. Differential Revision: D48952581 |
|
This pull request was successfully merged by Lulu Wu in fe4dab9. When will my fix make it into a release? | Upcoming Releases |
adamaveray
added a commit
to adamaveray/react-native
that referenced
this pull request
Sep 13, 2023
* main: (1012 commits) Add simple constructor for JSError (facebook#39415) Breaking: per-node pointScaleFactor (facebook#39403) Implement "tickleJS" for Hermes (facebook#39289) Add thread idle indicator (facebook#39206) Unblock `yarn android` on main (facebook#39413) Remove Codegen buck-oss scripts as they're unused (facebook#39422) Immediately dispatch events to the shared C++ infrastructure to support interruptability (facebook#39380) Fix race condition in Binding::reportMount (facebook#39420) Clone free state progression (facebook#39357) fix: return the correct default trait collection (facebook#38214) Read the React Native version and set the new arch flag properly (facebook#39388) Deprecate default_flags in Podfile (facebook#39389) Create Helper to read the package.json from Cocoapods (facebook#39390) Create helper to enforce the New Arch enabled for double released (facebook#39387) Remove layoutContext Drilling (facebook#39401) Remove JNI Binding usage of layoutContext (facebook#39402) Extract isBaselineLayout() (facebook#39400) Refactor and separate layout affected nodes react marker (facebook#39249) Bump AGP to 8.1.1 (facebook#39392) Fix broken Gradle Sync when opening the project with Android Studio (facebook#39412) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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:
"tickleJS" is a function in RN Hermes Debugger, we need to implement it for Bridgeless for feature correctness and keep parity with Bridge.
Changelog:
[General][Changed] - Implement "tickleJS" for Hermes
Differential Revision: D48952581