Avoid unnecessary babel polyfills and fix problem with nested worklets. #882
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
kmagiera
reviewed
Jun 10, 2020
plugin.js
Outdated
| enter(path) { | ||
| if (path.get('callee').matchesPattern('Object.assign')) { | ||
| // @babel/plugin-transform-object-assign | ||
| path.node.callee.object.name = 'random_temp_name'; |
There was a problem hiding this comment.
Can we rename to something more descriptive. For example: "Object__DO_NOT_TRANSFORM"
kmagiera
reviewed
Jun 10, 2020
plugin.js
Outdated
| }, | ||
| FunctionExpression: { | ||
| exit(path) { | ||
| processIfWorkletNode(t, path); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Can we put all of these under a single visitor the way it used to be:
'FunctionDeclaration|FunctionExpression|ArrowFunctionExpression': {
enter(path) { ... }
|
The code looks good, just see a few minor comment. Would be great to improve the description of the PR such that:
|
added 3 commits
June 10, 2020 15:27
|
Re pt 3 I think we should transform all the worklets. In the example you provided the way
If we only transform |
added 2 commits
June 12, 2020 13:39
kmagiera
approved these changes
Jun 12, 2020
piaskowyk
pushed a commit
that referenced
this pull request
Dec 8, 2022
## Summary
This PR removes a piece of code in `RuntimeDecorator` that used to overwrite `global` property of the global JS object with a dummy JS object which was a regular JS object filled only with Reanimated-specific values.
## Motivation
I tried to confirm that Hermes was indeed enabled on the UI context with the following snippet:
```ts
runOnUI(() => {
'worklet';
console.log('HermesInternal' in global); // should return true
})();
```
However, it returned `false`, despite the fact that a breakpoint on `facebook::hermes::makeHermesRuntime` was hit.
Then I noticed that we call `RuntimeDecorator::decorateRuntime(rt, "UI");` which overwrites `global` property in the UI context with a "dummy global", effectively causing `global.HermesInternal` to be `undefined`.
## Context
The concept of dummy global was introduced in the following PR:
* #882
There was one attempt to remove dummy global in the past but the change was immediately reverted in f846704:
* #2253
As @wkozyra95 stated:
> removed dummyGlobal and make `global.global` point to itself. I don't necessarily need that change to make it work, but the current behavior of global value seems incorrect
## Changes
One might wonder why not just remove these three lines:
https://github.com/software-mansion/react-native-reanimated/blob/074cac02d2ef503e7897ef63560d3b8514a8e350/Common/cpp/Tools/RuntimeDecorator.cpp#L33-L35
I'm glad you asked. The answer is that is simply doesn't work:
<img width="250" alt="Zrzut ekranu 2022-12-6 o 11 27 08" src="https://user-images.githubusercontent.com/20516055/205896462-1616af17-070d-43a9-9231-1bf4c6cb3e24.png">
That's because in many places we use `global.foo` syntax where `global` most likely represents the `global` property of the global object, not the global object itself (it's complicated).
The solution is to add the global JS object as the `global` property of itself so that `global.global === global`.
Obviously, it's like a reference cycle but hopefully the runtime knows how to clean itself once it gets terminated.
## What about ...?
### Overriding runtime global
Snippet:
```tsx
import React from 'react';
import { runOnUI } from 'react-native-reanimated';
export default function App() {
// JS
console.log('HermesInternal' in global);
runOnUI(() => {
'worklet';
// UI
console.log('HermesInternal' in global);
})();
return <></>;
}
```
Before:
After:
### Runtime deallocation
After the changes, the runtime is deallocated on app reload:
<img width="1103" alt="dealloc" src="https://user-images.githubusercontent.com/20516055/205902815-8087572d-33ff-4909-901d-39458f68ac73.png">
### Capturing global in worklets
Should work the same way since `plugin.js` hasn't been changed.
Input (from #882):
```ts
function out(easing) {
'worklet';
return t => {
'worklet';
return 1 - easing(1 - t);
};
}
```
Command: `npx babel file.js`
Output (after formatting):
```js
var out = (function () {
var _f = function _f(easing) {
return (function () {
var _f = function _f(t) {
return 1 - easing(1 - t);
};
_f._closure = { easing: easing };
_f.asString =
'function _f(t) {\n const {\n easing\n } = this._closure;\n return 1 - easing(1 - t);\n}\n//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJtYXBwaW5ncyI6IkFBRVNBLFNBQUNDLEVBQURELENBQUNBLENBQURBLEVBQUs7QUFBQTtBQUFBO0FBQUE7QUFFVixTQUFPLElBQUlFLE1BQU0sQ0FBQyxJQUFJRixDQUFMLENBQWpCO0FBRktBIiwibmFtZXMiOlsidCIsIl9mIiwiZWFzaW5nIl0sInNvdXJjZXMiOlsiL1VzZXJzL3RvbWVremF3L1JOT1MvU2hhcmVhYmxlUmV3cml0ZS9yZWFjdC1uYXRpdmUtcmVhbmltYXRlZC9maWxlLmpzIl0sInNvdXJjZXNDb250ZW50IjpbImZ1bmN0aW9uIG91dChlYXNpbmcpIHtcbiAgJ3dvcmtsZXQnO1xuICByZXR1cm4gdCA9PiB7XG4gICAgJ3dvcmtsZXQnO1xuICAgIHJldHVybiAxIC0gZWFzaW5nKDEgLSB0KTtcbiAgfTtcbn0iXX0=';
_f.__workletHash = 8053949848116;
_f.__location =
'/Users/tomekzaw/RNOS/ShareableRewrite/react-native-reanimated/file.js (3:9)';
return _f;
})();
};
_f._closure = {};
_f.asString =
"function out(easing) {\n return function (t) {\n 'worklet';\n\n return 1 - easing(1 - t);\n };\n}\n//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJtYXBwaW5ncyI6IkFBQUE7QUFFRSxTQUFPQSxhQUFLO0FBQ1Y7O0FBQ0EsV0FBTyxJQUFJQyxNQUFNLENBQUMsSUFBSUQsQ0FBTCxDQUFqQjtBQUZGO0FBRkYiLCJuYW1lcyI6WyJ0IiwiZWFzaW5nIl0sInNvdXJjZXMiOlsiL1VzZXJzL3RvbWVremF3L1JOT1MvU2hhcmVhYmxlUmV3cml0ZS9yZWFjdC1uYXRpdmUtcmVhbmltYXRlZC9maWxlLmpzIl0sInNvdXJjZXNDb250ZW50IjpbImZ1bmN0aW9uIG91dChlYXNpbmcpIHtcbiAgJ3dvcmtsZXQnO1xuICByZXR1cm4gdCA9PiB7XG4gICAgJ3dvcmtsZXQnO1xuICAgIHJldHVybiAxIC0gZWFzaW5nKDEgLSB0KTtcbiAgfTtcbn0iXX0=";
_f.__workletHash = 3984642990765;
_f.__location =
'/Users/tomekzaw/RNOS/ShareableRewrite/react-native-reanimated/file.js (1:0)';
return _f;
})();
```
Conclusion: `global` is not captured (as `_f._closure = {};`), which is correct.
### `gc`
Snippet:
```tsx
import React from 'react';
import { runOnUI } from 'react-native-reanimated';
export default function App() {
// JS
console.log(gc);
console.log(gc());
console.log(global.gc);
console.log(global.gc());
runOnUI(() => {
'worklet';
// UI
console.log(gc);
// console.log(gc()); // Tried to synchronously call a non-worklet function on the UI thread.
console.log(global.gc);
console.log(global.gc());
})();
return <></>;
}
```
Before:
After:
## Test plan
Just check the above test cases one-by-one.
## TODO
- [ ] check if runtime deallocates on reload (set a breakpoint in `~JSCRuntime`)
- [ ] check if `global` is not in closure
- [ ] check if `gc` works
- [ ] check if expo-gl works
fluiddot
pushed a commit
to wordpress-mobile/react-native-reanimated
that referenced
this pull request
Jun 5, 2023
## Summary
This PR removes a piece of code in `RuntimeDecorator` that used to overwrite `global` property of the global JS object with a dummy JS object which was a regular JS object filled only with Reanimated-specific values.
## Motivation
I tried to confirm that Hermes was indeed enabled on the UI context with the following snippet:
```ts
runOnUI(() => {
'worklet';
console.log('HermesInternal' in global); // should return true
})();
```
However, it returned `false`, despite the fact that a breakpoint on `facebook::hermes::makeHermesRuntime` was hit.
Then I noticed that we call `RuntimeDecorator::decorateRuntime(rt, "UI");` which overwrites `global` property in the UI context with a "dummy global", effectively causing `global.HermesInternal` to be `undefined`.
## Context
The concept of dummy global was introduced in the following PR:
* software-mansion#882
There was one attempt to remove dummy global in the past but the change was immediately reverted in f846704:
* software-mansion#2253
As @wkozyra95 stated:
> removed dummyGlobal and make `global.global` point to itself. I don't necessarily need that change to make it work, but the current behavior of global value seems incorrect
## Changes
One might wonder why not just remove these three lines:
https://github.com/software-mansion/react-native-reanimated/blob/074cac02d2ef503e7897ef63560d3b8514a8e350/Common/cpp/Tools/RuntimeDecorator.cpp#L33-L35
I'm glad you asked. The answer is that is simply doesn't work:
<img width="250" alt="Zrzut ekranu 2022-12-6 o 11 27 08" src="https://user-images.githubusercontent.com/20516055/205896462-1616af17-070d-43a9-9231-1bf4c6cb3e24.png">
That's because in many places we use `global.foo` syntax where `global` most likely represents the `global` property of the global object, not the global object itself (it's complicated).
The solution is to add the global JS object as the `global` property of itself so that `global.global === global`.
Obviously, it's like a reference cycle but hopefully the runtime knows how to clean itself once it gets terminated.
## What about ...?
### Overriding runtime global
Snippet:
```tsx
import React from 'react';
import { runOnUI } from 'react-native-reanimated';
export default function App() {
// JS
console.log('HermesInternal' in global);
runOnUI(() => {
'worklet';
// UI
console.log('HermesInternal' in global);
})();
return <></>;
}
```
Before:
After:
### Runtime deallocation
After the changes, the runtime is deallocated on app reload:
<img width="1103" alt="dealloc" src="https://user-images.githubusercontent.com/20516055/205902815-8087572d-33ff-4909-901d-39458f68ac73.png">
### Capturing global in worklets
Should work the same way since `plugin.js` hasn't been changed.
Input (from software-mansion#882):
```ts
function out(easing) {
'worklet';
return t => {
'worklet';
return 1 - easing(1 - t);
};
}
```
Command: `npx babel file.js`
Output (after formatting):
```js
var out = (function () {
var _f = function _f(easing) {
return (function () {
var _f = function _f(t) {
return 1 - easing(1 - t);
};
_f._closure = { easing: easing };
_f.asString =
'function _f(t) {\n const {\n easing\n } = this._closure;\n return 1 - easing(1 - t);\n}\n//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJtYXBwaW5ncyI6IkFBRVNBLFNBQUNDLEVBQURELENBQUNBLENBQURBLEVBQUs7QUFBQTtBQUFBO0FBQUE7QUFFVixTQUFPLElBQUlFLE1BQU0sQ0FBQyxJQUFJRixDQUFMLENBQWpCO0FBRktBIiwibmFtZXMiOlsidCIsIl9mIiwiZWFzaW5nIl0sInNvdXJjZXMiOlsiL1VzZXJzL3RvbWVremF3L1JOT1MvU2hhcmVhYmxlUmV3cml0ZS9yZWFjdC1uYXRpdmUtcmVhbmltYXRlZC9maWxlLmpzIl0sInNvdXJjZXNDb250ZW50IjpbImZ1bmN0aW9uIG91dChlYXNpbmcpIHtcbiAgJ3dvcmtsZXQnO1xuICByZXR1cm4gdCA9PiB7XG4gICAgJ3dvcmtsZXQnO1xuICAgIHJldHVybiAxIC0gZWFzaW5nKDEgLSB0KTtcbiAgfTtcbn0iXX0=';
_f.__workletHash = 8053949848116;
_f.__location =
'/Users/tomekzaw/RNOS/ShareableRewrite/react-native-reanimated/file.js (3:9)';
return _f;
})();
};
_f._closure = {};
_f.asString =
"function out(easing) {\n return function (t) {\n 'worklet';\n\n return 1 - easing(1 - t);\n };\n}\n//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJtYXBwaW5ncyI6IkFBQUE7QUFFRSxTQUFPQSxhQUFLO0FBQ1Y7O0FBQ0EsV0FBTyxJQUFJQyxNQUFNLENBQUMsSUFBSUQsQ0FBTCxDQUFqQjtBQUZGO0FBRkYiLCJuYW1lcyI6WyJ0IiwiZWFzaW5nIl0sInNvdXJjZXMiOlsiL1VzZXJzL3RvbWVremF3L1JOT1MvU2hhcmVhYmxlUmV3cml0ZS9yZWFjdC1uYXRpdmUtcmVhbmltYXRlZC9maWxlLmpzIl0sInNvdXJjZXNDb250ZW50IjpbImZ1bmN0aW9uIG91dChlYXNpbmcpIHtcbiAgJ3dvcmtsZXQnO1xuICByZXR1cm4gdCA9PiB7XG4gICAgJ3dvcmtsZXQnO1xuICAgIHJldHVybiAxIC0gZWFzaW5nKDEgLSB0KTtcbiAgfTtcbn0iXX0=";
_f.__workletHash = 3984642990765;
_f.__location =
'/Users/tomekzaw/RNOS/ShareableRewrite/react-native-reanimated/file.js (1:0)';
return _f;
})();
```
Conclusion: `global` is not captured (as `_f._closure = {};`), which is correct.
### `gc`
Snippet:
```tsx
import React from 'react';
import { runOnUI } from 'react-native-reanimated';
export default function App() {
// JS
console.log(gc);
console.log(gc());
console.log(global.gc);
console.log(global.gc());
runOnUI(() => {
'worklet';
// UI
console.log(gc);
// console.log(gc()); // Tried to synchronously call a non-worklet function on the UI thread.
console.log(global.gc);
console.log(global.gc());
})();
return <></>;
}
```
Before:
After:
## Test plan
Just check the above test cases one-by-one.
## TODO
- [ ] check if runtime deallocates on reload (set a breakpoint in `~JSCRuntime`)
- [ ] check if `global` is not in closure
- [ ] check if `gc` works
- [ ] check if expo-gl works
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.
The Typescript fix revealed another issue with overzealous babel polyfills. The pull-request solves two most important problems caused by @babel/plugin-transform-object-assign and @babel/plugin-transform-parameters.
Fixes problem with nested worklets by adding 'global' to our blacklist and providing dummy __reanimatedWorkletInit implementation on UI side.
Additional info:
1 "Object.assign"
Babel plugin "@babel/plugin-transform-object-assign" transforms Object.assign() call into (0, _extend.default)() where _extend is a module with Object.assign polyfill. Because the plugin do that we capture _extend object and bad things happen. This pr renames "Object" to "Object__DO_NOT_TRANSFOR" in each Object.assign call on entry and reverts this change on exit.
2 "function(...args)"
Babel plugin @babel/plugin-transform-parameters transforms spread syntax and uses arguments instead. To prevent "arguments" variable from being captured the pr add it to the blacklist.
3 Nested Worklets problem
->
As we can see global is captured.