Optimize the way we extract code string and in which we pass it across runtimes #3930
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
tomekzaw
reviewed
Jan 11, 2023
tomekzaw
reviewed
Jan 12, 2023
tomekzaw
reviewed
Jan 12, 2023
tomekzaw
reviewed
Jan 12, 2023
|
Don't know why but this PR breaks the app when using JSC (Hermes is fine):
|
|
There was one issue with JSC related to Object.freeze use. I fixed that, updated other comments, made tests pass and think this is ready for review |
tomekzaw
approved these changes
Jan 17, 2023
fluiddot
pushed a commit
to wordpress-mobile/react-native-reanimated
that referenced
this pull request
Jun 5, 2023
…s runtimes (software-mansion#3930) ## Summary This PR changes the way we extract code string and additional metadata for worklets and the way we pass it later on between runtimes such that we can save on allocating code strings that tend to be relatively big and waste precious time on allocating those strings. The scope of the change includes: 1. Changes in plugin code, we no longer assign code string directly to worklet function objects, instead we extract a metadata object and define in in the program context (at the main file level). The metadata includes things that are static, like the code converted to string (formerly "asString", location, source map and stack details data). We then reference that metadata object and assign it to the worklet object under `__initData` field. 2. We update "Core Functions" that need to access code string such that it fetches the string from a new location – `__initData.code` 3. We refactor the concept of "retaining values" and only conditionally retain JS values on the UI runtime from the shareable. The purpose of this change is that we can be sure the initData is persisted on the UI runtime and not recreated each time the worklet it passed to be executed on the UI runtime. We add additional flag `shouldRetain` to the `makeShareableClone` JSI method that allows us to control this behavior. The refactoring updates the inheritance scheme and makes `ShareableObjects` etc no longer inherit `RetainingShareable` but instead we only use `RetainingShareable` when the flag is set. This change also deletes the previous behavior where we'd use weak objects to keep the reference to the JS Value as we are unsure about positive/negative impact of that code (we know it impacts GC times negatively but not sure if this time is counterbalanced by saving on the time to convert the values) ## Test plan Plugin tests needs to be updated. Wanted to put this change out for others to test.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jul 17, 2023
<!-- Thanks for submitting a pull request! We appreciate you spending the time to work on these changes. Please follow the template so that the reviewers can easily understand what the code changes affect. --> ## Summary I noticed that `version` and `sourceMap` properties are not included in plugin snapshots. This PR: * renames environmental variables used for testing purposes * unifies `compact` setting across app and tests and updates snapshots accordingly * restores `version` property in snapshots but uses mock version "x.y.z" instead of real version * moves `version` property to `__initData` to prevent allocating multiple strings (see #3930 for explanation) * restores `sourceMap` property in `__initData` but uses mock source map for all snapshot tests * adds one unit test that checks real `sourceMap` property * adds double negation to fix return type of `isRelease` (previously it returned `boolean | string | ""`) ## Test plan <!-- Provide a minimal but complete code snippet that can be used to test out this change along with instructions how to run it and a description of the expected behavior. -->
github-merge-queue bot
pushed a commit
that referenced
this pull request
Nov 13, 2023
<!-- Thanks for submitting a pull request! We appreciate you spending the time to work on these changes. Please follow the template so that the reviewers can easily understand what the code changes affect. --> ## Summary When serializing worklets, first we call `makeShareableCloneRecursive` for `value.__initData` (see #3930) and then we process the rest of the props. Currently, we don't skip `__initData` in the second phase which leads to double conversion of this field and contradicts the whole idea. The regression was introduced by me in #4944 which removed `delete value.__initData;` in order to allow worklets to be copied to multiple runtimes and not only one. This PR skips serializing `__initData` if it has already been serialized. ## Test plan <!-- Provide a minimal but complete code snippet that can be used to test out this change along with instructions how to run it and a description of the expected behavior. -->
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
This PR changes the way we extract code string and additional metadata for worklets and the way we pass it later on between runtimes such that we can save on allocating code strings that tend to be relatively big and waste precious time on allocating those strings.
The scope of the change includes:
__initDatafield.__initData.codeshouldRetainto themakeShareableCloneJSI method that allows us to control this behavior. The refactoring updates the inheritance scheme and makesShareableObjectsetc no longer inheritRetainingShareablebut instead we only useRetainingShareablewhen the flag is set. This change also deletes the previous behavior where we'd use weak objects to keep the reference to the JS Value as we are unsure about positive/negative impact of that code (we know it impacts GC times negatively but not sure if this time is counterbalanced by saving on the time to convert the values)Test plan
Plugin tests needs to be updated. Wanted to put this change out for others to test.