Fix to the memory leak issue with useEffect

[trueadm]

This is a possible solution to the memory leak issues @localvoid alerted us to in this repro https://codesandbox.io/s/lz61v39r7.

I'm not 100% sure if the changes in this PR break something else, so it would be good to get @acdlite eye's on this. Specifically this PR makes the following changes:

• When processing an effect's create function, we null out the create field after. If we don't do this, the context of the create function closure causes memory retainment.
• Furthermore, if we don't pass the create function from useEffect to the dependencies array. Doing so, brings the memory retainment issue in the above point. Instead, we tag the create function with a field __key and assign a unique symbol to the function. We then use this symbol as the key in the dependencies.

I can confirm that in the cases of the repro the memory leak issues no longer exist with these two changes. Unfortunately, I don't know of any good way to make tests that can validate memory leaks. If anyone has ideas on how we might test these changes, please let me know.

[sizebot]

Details of bundled changes.

Comparing: 051272f...d05da56

react-dom

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-dom.development.js	+0.1%	+0.2%	708.8 KB	709.46 KB	163.87 KB	164.11 KB	UMD_DEV
react-dom.development.js	+0.1%	+0.2%	704.11 KB	704.77 KB	162.5 KB	162.74 KB	NODE_DEV
ReactDOM-dev.js	+0.1%	+0.2%	724.84 KB	725.5 KB	163.62 KB	163.87 KB	FB_WWW_DEV
ReactDOM-prod.js	0.0%	0.0%	309.59 KB	309.68 KB	57.03 KB	57.05 KB	FB_WWW_PROD
ReactDOM-profiling.js	0.0%	0.0%	316.36 KB	316.46 KB	58.42 KB	58.45 KB	FB_WWW_PROFILING

react-art

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-art.development.js	+0.1%	+0.2%	495.94 KB	496.6 KB	109.49 KB	109.75 KB	UMD_DEV
react-art.development.js	+0.2%	+0.3%	427.72 KB	428.38 KB	92.46 KB	92.7 KB	NODE_DEV
ReactART-dev.js	+0.2%	+0.3%	434.86 KB	435.52 KB	91.37 KB	91.62 KB	FB_WWW_DEV
ReactART-prod.js	🔺+0.1%	🔺+0.1%	183.51 KB	183.6 KB	31.34 KB	31.37 KB	FB_WWW_PROD

react-test-renderer

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-test-renderer.development.js	+0.1%	+0.3%	440.38 KB	441.04 KB	95.15 KB	95.4 KB	UMD_DEV
react-test-renderer.development.js	+0.2%	+0.3%	435.6 KB	436.26 KB	94.01 KB	94.25 KB	NODE_DEV
ReactTestRenderer-dev.js	+0.1%	+0.3%	442.93 KB	443.59 KB	93.21 KB	93.45 KB	FB_WWW_DEV

react-reconciler

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-reconciler.development.js	+0.2%	+0.3%	425.56 KB	426.22 KB	90.94 KB	91.2 KB	NODE_DEV
react-reconciler-persistent.development.js	+0.2%	+0.3%	424.01 KB	424.67 KB	90.31 KB	90.55 KB	NODE_DEV

react-native-renderer

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
ReactNativeRenderer-dev.js	+0.1%	+0.2%	560.05 KB	560.71 KB	122.04 KB	122.28 KB	RN_FB_DEV
ReactNativeRenderer-prod.js	0.0%	🔺+0.1%	238.83 KB	238.93 KB	42.01 KB	42.04 KB	RN_FB_PROD
ReactNativeRenderer-dev.js	+0.1%	+0.2%	559.75 KB	560.41 KB	121.95 KB	122.19 KB	RN_OSS_DEV
ReactFabric-dev.js	+0.1%	+0.2%	550.37 KB	551.03 KB	119.55 KB	119.8 KB	RN_FB_DEV
ReactFabric-dev.js	+0.1%	+0.2%	550.4 KB	551.06 KB	119.56 KB	119.82 KB	RN_OSS_DEV
ReactNativeRenderer-profiling.js	0.0%	+0.1%	244.63 KB	244.73 KB	43.36 KB	43.38 KB	RN_FB_PROFILING

scheduler

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
scheduler.development.js	n/a	n/a	0 B	19.17 KB	0 B	5.74 KB	UMD_DEV
scheduler.production.min.js	n/a	n/a	0 B	3.16 KB	0 B	1.53 KB	UMD_PROD

Generated by 🚫 dangerJS

[sophiebits]

• Do you disagree that it’s OK to keep this memory until unmount, as we discussed in the team meeting? I think we were struggling to find practical examples of when this would really matter.

• The __key thing seems kind of complicated and sort of slow. If we want to try to fix this perhaps it makes more sense to not use create as an input? Instead always rerunning if there is no deps list, even if the function is unchanged. Since it is easy to get from that to the current behavior if you need it.

[trueadm]

@sophiebits In order to remove the memory leak on unmount, you need to null out the memoizedState from the fiber and alternate fiber in detachFiber. That doesn't stop the memory leak properly though, so this approach aims at proactively tackling that. In regards to the __key change, I actually thought that the key change would improve performance compared to always rerunning.

Furthermore, when I tried the approach of always rerunning some hook tests fail, specifically fires all mutation effects before firing any layout effects and force flushes passive effects before firing new layout effects unless I add the create function to the deps list (or the symbol alternative I added in this PR).

[sebmarkbage]

The big question is if we want to be optimizing for the common case:

useEffect(() => { });

In this case this will always be a new function so all attempts to avoid recalling it will just slow it down.

Since it will always reexecute we ideally don’t need to even create a hook object for it. But we don’t know before we call it if it will also have a destructor associated with it. Unless we add those to a different list somehow.

[sebmarkbage]

So this doesn’t work when you have a destroy function right?

useEffect(() => () => ...);

Still would leak since we need to store the callback somewhere.

Similarly what happens with useCallback(...) etc? Do they still leak?

I’m not sure what the bar is for when we’re ok with leaking vs not and which scenarios.

[trueadm]

@sebmarkbage I'm not super sure about all cases. The underlying issue is here is retention of closure references and their contexts.

[trueadm]

@sebmarkbage I tried the alternative approach as I outlined above: #14218