Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove ref loop from ShareableValue #1580

Merged
merged 3 commits into from
Dec 30, 2020
Merged

Remove ref loop from ShareableValue #1580

merged 3 commits into from
Dec 30, 2020

Conversation

Szymon20000
Copy link
Contributor

@Szymon20000 Szymon20000 commented Dec 30, 2020 •
edited

Description

ShareableValue keeps a reference to its JS representation. Unfortunately, worklet retains this in lambda function what creates a reference loop.

Changes

Capture in the lambda function only necessary values.

Test code and steps to reproduce

makeShareable(
   () =>{
      'worklet'
      //nothing
   }
);

ShareableValue destructor is never called.

Checklist

  • Included code example that can be used to test this change
  • Updated TS types
  • Added TS types tests
  • Added unit / integration tests
  • Updated documentation
  • Ensured that CI passes

karol-bisztyga
karol-bisztyga reviewed Dec 30, 2020
View reviewed changes
Copy link
Contributor

@karol-bisztyga karol-bisztyga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to explicitly copy all the values passed to lambdas

Common/cpp/SharedItems/ShareableValue.cpp Outdated Show resolved Hide resolved
Common/cpp/SharedItems/ShareableValue.cpp Outdated Show resolved Hide resolved
Common/cpp/SharedItems/ShareableValue.cpp Outdated Show resolved Hide resolved
@karol-bisztyga karol-bisztyga mentioned this pull request Dec 30, 2020
Merged
@karol-bisztyga
Copy link
Contributor

good job 🙌

@Szymon20000 Szymon20000 merged commit 17e4eeb into master Dec 30, 2020
@Szymon20000 Szymon20000 deleted the @szymon/remove_ref_loop branch December 30, 2020 12:35
karol-bisztyga added a commit that referenced this pull request Jan 4, 2021
@karol-bisztyga
Memory leaks (#1556)
c57e287 
## Description

Here we will solve/discuss/work on issues related to memory leaks and also code changes in #1543.

Every case will be taken care of separately in terms of both the code and the description.

# Cases

### Memory leak with `makeShareable`, inner class, and a method(that has to be inside of the component) passed to its constructor.


- solution
	  - prevent applying cache for objects containing host functions(be1cb52)
	  - remove ref loop from ShareableValue(#1580)

<details>
<summary>graph</summary>

![diagram](https://user-images.githubusercontent.com/59833830/103015936-861a6680-4541-11eb-898b-63ee7be1f033.png)

</details>

<details>
<summary>code</summary>

```
import {
  makeShareable,
} from 'react-native-reanimated';
import { Text } from 'react-native';
import React, { useEffect, useRef } from 'react';

export default function OneMakeShareable() {
  function sth() {}

  class Temp1 {
    constructor(fun) {
      this.fun = fun;
    }
  }

  const ctx = useRef(null);
  if (ctx.current === null) {
    const temp = new Temp1(sth);
    ctx.current = {
      t: makeShareable(temp),
    };
  }

  useEffect(() => {
    return () => {
      ctx.current = undefined;
    };
  }, []);

  return <Text>ONE</Text>;
}

```

</details>

### The same as in the first case but a worklet with a reference to the `temp` object is passed to `makeShareable` instead of a `temp` object directly.


<details>
<summary>graph</summary>

![graph](https://user-images.githubusercontent.com/59833830/103353589-88cd0c80-4aa9-11eb-95e4-a37c566fa24a.png)

</details>

<details>
<summary>code</summary>

```
const ThreeTest = () => {
  function sth() {}
  class Temp31 {
    constructor(fun) {
      this.fun = fun;
    }
  }

  const tempc = new Temp31(sth);

  const wrk = () => {
    'worklet';
    const t = tempc;
  };

  makeShareable(wrk);
  let x = makeShareable(wrk);
  x = null;
  return <Text>Three</Text>;
};
```

</details>


### Similar pattern as above but this time `useAnimatedStyle` is used instead of `makeShareable`

❗ this turned out not to be a problem. It just depends on the UI garbage collector. If it is triggered manually there are no dangling objects(it was observed that it cleans after some time).

<details>
<summary>graph</summary>

![depgraph2](https://user-images.githubusercontent.com/59833830/103352655-14916980-4aa7-11eb-870d-28f9bcf6edfc.png)

</details>



<details>
<summary>code</summary>

```
import { runOnJS, useAnimatedStyle } from 'react-native-reanimated';
import { Text } from 'react-native';
import React from 'react';

export default function OneMakeShareable() {
  function sth() {}

  class Temp2 {
    constructor(fun) {
      this.fun = fun;
    }
  }

  const temp = new Temp2(sth);

  useAnimatedStyle(() => {
    const x = temp;
    return {
      width: 100,
    };
  });

  return <Text>Two</Text>;
}

```

</details>

also tested with `useState`

<details>
<summary>code</summary>

```
export function FourState() {
 const [state, setState] = useState(0);
 
 class Temp4 {
   constructor(f) {
     this.f = f;
   }
 }
 const temp = new Temp4(setState);
 
 useAnimatedStyle(() => {
    runOnJS(temp.f)(state + 1);
   return {};
 });
 
 
 return (
   <View>
     <Text>Four</Text>
   </View>
 );
}
```

</details>

# Other

- forwarded: https://github.com/software-mansion/react-native-reanimated/pull/1543/files#r546597171
	- This seems to be unnecessary as unregistering from events happen anyway on `componentWillUnmount` in `createAnimatedComponent`
- forwarded: https://github.com/software-mansion/react-native-reanimated/pull/1543/files#r546594788
	- This prevents the loop: c++ -> ref -> fiberNode(@Szymon20000) where in ref we hold for instance `setState`
rpavlovs pushed a commit to rpavlovs/react-native-reanimated that referenced this pull request Jan 12, 2021
@rpavlovs
Remove ref loop from ShareableValue (software-mansion#1580)
7787195 
## Description

ShareableValue keeps a reference to its JS representation. Unfortunately, worklet retains `this` in lambda function what creates a reference loop.

## Changes

Capture in the lambda function only necessary values.

<!--

## Screenshots / GIFs

Here you can add screenshots / GIFs documenting your change.

You can add before / after section if you're changing some behavior.

### Before

### After

-->

## Test code and steps to reproduce

```JS
makeShareable(
   () =>{
      'worklet'
      //nothing
   }
);
```
ShareableValue destructor is never called.

<!--
Please include code that can be used to test this change and short description how this example should work.
This snippet should be as minimal as possible and ready to be pasted into editor (don't exclude exports or remove "not important" parts of reproduction example)
-->

## Checklist

- [ ] Included code example that can be used to test this change
- [ ] Updated TS types
- [ ] Added TS types tests
- [ ] Added unit / integration tests
- [ ] Updated documentation
- [ ] Ensured that CI passes
rpavlovs pushed a commit to rpavlovs/react-native-reanimated that referenced this pull request Jan 12, 2021
@karol-bisztyga @rpavlovs
Memory leaks (software-mansion#1556)
0ce033f 
## Description

Here we will solve/discuss/work on issues related to memory leaks and also code changes in software-mansion#1543.

Every case will be taken care of separately in terms of both the code and the description.

# Cases

### Memory leak with `makeShareable`, inner class, and a method(that has to be inside of the component) passed to its constructor.


- solution
	  - prevent applying cache for objects containing host functions(be1cb52)
	  - remove ref loop from ShareableValue(software-mansion#1580)

<details>
<summary>graph</summary>

![diagram](https://user-images.githubusercontent.com/59833830/103015936-861a6680-4541-11eb-898b-63ee7be1f033.png)

</details>

<details>
<summary>code</summary>

```
import {
  makeShareable,
} from 'react-native-reanimated';
import { Text } from 'react-native';
import React, { useEffect, useRef } from 'react';

export default function OneMakeShareable() {
  function sth() {}

  class Temp1 {
    constructor(fun) {
      this.fun = fun;
    }
  }

  const ctx = useRef(null);
  if (ctx.current === null) {
    const temp = new Temp1(sth);
    ctx.current = {
      t: makeShareable(temp),
    };
  }

  useEffect(() => {
    return () => {
      ctx.current = undefined;
    };
  }, []);

  return <Text>ONE</Text>;
}

```

</details>

### The same as in the first case but a worklet with a reference to the `temp` object is passed to `makeShareable` instead of a `temp` object directly.


<details>
<summary>graph</summary>

![graph](https://user-images.githubusercontent.com/59833830/103353589-88cd0c80-4aa9-11eb-95e4-a37c566fa24a.png)

</details>

<details>
<summary>code</summary>

```
const ThreeTest = () => {
  function sth() {}
  class Temp31 {
    constructor(fun) {
      this.fun = fun;
    }
  }

  const tempc = new Temp31(sth);

  const wrk = () => {
    'worklet';
    const t = tempc;
  };

  makeShareable(wrk);
  let x = makeShareable(wrk);
  x = null;
  return <Text>Three</Text>;
};
```

</details>


### Similar pattern as above but this time `useAnimatedStyle` is used instead of `makeShareable`

❗ this turned out not to be a problem. It just depends on the UI garbage collector. If it is triggered manually there are no dangling objects(it was observed that it cleans after some time).

<details>
<summary>graph</summary>

![depgraph2](https://user-images.githubusercontent.com/59833830/103352655-14916980-4aa7-11eb-870d-28f9bcf6edfc.png)

</details>



<details>
<summary>code</summary>

```
import { runOnJS, useAnimatedStyle } from 'react-native-reanimated';
import { Text } from 'react-native';
import React from 'react';

export default function OneMakeShareable() {
  function sth() {}

  class Temp2 {
    constructor(fun) {
      this.fun = fun;
    }
  }

  const temp = new Temp2(sth);

  useAnimatedStyle(() => {
    const x = temp;
    return {
      width: 100,
    };
  });

  return <Text>Two</Text>;
}

```

</details>

also tested with `useState`

<details>
<summary>code</summary>

```
export function FourState() {
 const [state, setState] = useState(0);
 
 class Temp4 {
   constructor(f) {
     this.f = f;
   }
 }
 const temp = new Temp4(setState);
 
 useAnimatedStyle(() => {
    runOnJS(temp.f)(state + 1);
   return {};
 });
 
 
 return (
   <View>
     <Text>Four</Text>
   </View>
 );
}
```

</details>

# Other

- forwarded: https://github.com/software-mansion/react-native-reanimated/pull/1543/files#r546597171
	- This seems to be unnecessary as unregistering from events happen anyway on `componentWillUnmount` in `createAnimatedComponent`
- forwarded: https://github.com/software-mansion/react-native-reanimated/pull/1543/files#r546594788
	- This prevents the loop: c++ -> ref -> fiberNode(@Szymon20000) where in ref we hold for instance `setState`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karol-bisztyga karol-bisztyga karol-bisztyga approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Szymon20000 @karol-bisztyga