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.

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

Clean up unmounted fiber references for GC #15157

Closed
wants to merge 14 commits into from

Conversation

@paulshen
Copy link
Contributor

paulshen commented Mar 19, 2019

This change cleans up Fiber pointers to help with garbage collection.

This clears Fiber's stateNode pointer when committing unmounts. This removes a potential pointer back to host nodes (e.g. DOM) and component instances.

This cleans nextEffect pointer when we're done processing the effect chain (either when flushing passive effects or after committing layout effects). Without clearing this, it's possible to retain effect pointers to unmounted nodes.

A continuation of #14218. Detaching nextEffect needs to happen after committing host effects because the effect chain is needed for lifecycles and ref callbacks.

@sizebot

This comment has been minimized.

Copy link

sizebot commented Mar 19, 2019

ReactDOM: size: 🔺+0.1%, gzip: 🔺+0.1%

Details of bundled changes.

Comparing: b4bc33a...568ef3e

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.1% +0.2% 892.65 KB 893.88 KB 201.77 KB 202.08 KB UMD_DEV
react-dom.production.min.js 🔺+0.1% 🔺+0.1% 105.58 KB 105.72 KB 34.19 KB 34.22 KB UMD_PROD
react-dom.profiling.min.js +0.1% 0.0% 108.59 KB 108.73 KB 34.8 KB 34.81 KB UMD_PROFILING
react-dom.development.js +0.1% +0.2% 887.27 KB 888.51 KB 200.21 KB 200.52 KB NODE_DEV
react-dom.production.min.js 🔺+0.1% 🔺+0.2% 105.56 KB 105.7 KB 33.64 KB 33.7 KB NODE_PROD
react-dom.profiling.min.js +0.1% +0.1% 108.68 KB 108.82 KB 34.23 KB 34.25 KB NODE_PROFILING
ReactDOM-dev.js +0.1% +0.2% 913.37 KB 914.6 KB 202.1 KB 202.42 KB FB_WWW_DEV
ReactDOM-prod.js 🔺+0.1% 🔺+0.1% 337.04 KB 337.43 KB 62.09 KB 62.15 KB FB_WWW_PROD
ReactDOM-profiling.js +0.1% +0.1% 343.71 KB 344.16 KB 63.47 KB 63.54 KB FB_WWW_PROFILING
react-dom-unstable-fire.development.js +0.1% +0.2% 892.97 KB 894.21 KB 201.91 KB 202.22 KB UMD_DEV
react-dom-unstable-fire.production.min.js 🔺+0.1% 🔺+0.1% 105.59 KB 105.73 KB 34.2 KB 34.23 KB UMD_PROD
react-dom-unstable-fire.profiling.min.js +0.1% 0.0% 108.6 KB 108.74 KB 34.81 KB 34.82 KB UMD_PROFILING
react-dom-unstable-fire.development.js +0.1% +0.2% 887.6 KB 888.83 KB 200.35 KB 200.66 KB NODE_DEV
react-dom-unstable-fire.production.min.js 🔺+0.1% 🔺+0.2% 105.58 KB 105.72 KB 33.65 KB 33.71 KB NODE_PROD
react-dom-unstable-fire.profiling.min.js +0.1% +0.1% 108.69 KB 108.83 KB 34.24 KB 34.26 KB NODE_PROFILING
ReactFire-dev.js +0.1% +0.2% 912.55 KB 913.79 KB 202.11 KB 202.44 KB FB_WWW_DEV
ReactFire-prod.js 🔺+0.1% 🔺+0.1% 325.69 KB 326.07 KB 59.77 KB 59.84 KB FB_WWW_PROD
ReactFire-profiling.js +0.1% +0.1% 332.3 KB 332.75 KB 61.14 KB 61.2 KB FB_WWW_PROFILING
react-dom-unstable-new-scheduler.development.js +0.1% +0.2% 885.78 KB 887.02 KB 199.63 KB 199.94 KB NODE_DEV
react-dom-unstable-new-scheduler.production.min.js 🔺+0.1% 🔺+0.1% 103.65 KB 103.79 KB 33.12 KB 33.16 KB NODE_PROD
react-dom-unstable-new-scheduler.profiling.min.js +0.1% +0.1% 106.99 KB 107.12 KB 34.01 KB 34.04 KB NODE_PROFILING
ReactDOMNewScheduler-dev.js +0.1% +0.2% 913.4 KB 914.63 KB 202.1 KB 202.43 KB FB_WWW_DEV
ReactDOMNewScheduler-prod.js 🔺+0.2% 🔺+0.1% 331 KB 331.61 KB 61.64 KB 61.71 KB FB_WWW_PROD
ReactDOMNewScheduler-profiling.js +0.1% +0.1% 336.57 KB 336.83 KB 62.64 KB 62.72 KB FB_WWW_PROFILING
react-dom-test-utils.development.js 0.0% -0.0% 52.68 KB 52.68 KB 14.33 KB 14.33 KB UMD_DEV
react-dom-test-utils.production.min.js 0.0% -0.1% 10.52 KB 10.52 KB 3.9 KB 3.89 KB UMD_PROD
react-dom-test-utils.production.min.js 0.0% 0.0% 10.31 KB 10.31 KB 3.82 KB 3.82 KB NODE_PROD
react-dom-unstable-native-dependencies.development.js 0.0% -0.0% 60.76 KB 60.76 KB 15.85 KB 15.85 KB UMD_DEV
ReactDOMServer-dev.js 0.0% -0.0% 135.04 KB 135.04 KB 34.69 KB 34.69 KB FB_WWW_DEV
ReactDOMServer-prod.js 0.0% -0.0% 47.7 KB 47.7 KB 10.95 KB 10.95 KB FB_WWW_PROD
react-dom-server.node.development.js 0.0% -0.0% 134.59 KB 134.59 KB 35.53 KB 35.53 KB NODE_DEV
react-dom-server.node.production.min.js 0.0% -0.0% 20.03 KB 20.03 KB 7.53 KB 7.53 KB NODE_PROD
react-dom-unstable-fizz.browser.production.min.js 0.0% -0.1% 1.21 KB 1.21 KB 706 B 705 B UMD_PROD
react-dom-unstable-fizz.browser.production.min.js 0.0% -0.2% 1.05 KB 1.05 KB 637 B 636 B NODE_PROD
react-dom-unstable-fizz.node.development.js 0.0% -0.1% 3.74 KB 3.74 KB 1.43 KB 1.43 KB NODE_DEV

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.2% +0.2% 639.67 KB 640.89 KB 138.14 KB 138.45 KB UMD_DEV
react-art.production.min.js 🔺+0.2% 🔺+0.1% 97.31 KB 97.46 KB 29.77 KB 29.81 KB UMD_PROD
react-art.development.js +0.2% +0.3% 570.88 KB 572.12 KB 120.86 KB 121.16 KB NODE_DEV
react-art.production.min.js 🔺+0.2% 🔺+0.5% 62.31 KB 62.46 KB 19.04 KB 19.13 KB NODE_PROD
ReactART-dev.js +0.2% +0.3% 582 KB 583.24 KB 120.56 KB 120.86 KB FB_WWW_DEV
ReactART-prod.js 🔺+0.2% 🔺+0.2% 198.95 KB 199.36 KB 33.75 KB 33.81 KB FB_WWW_PROD

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +0.2% +0.2% 703.44 KB 704.67 KB 150.62 KB 150.94 KB RN_FB_DEV
ReactNativeRenderer-prod.js 🔺+0.2% 🔺+0.1% 246.9 KB 247.43 KB 43.19 KB 43.25 KB RN_FB_PROD
ReactNativeRenderer-profiling.js +0.2% +0.2% 252.91 KB 253.5 KB 44.51 KB 44.59 KB RN_FB_PROFILING
ReactNativeRenderer-dev.js +0.2% +0.2% 703.35 KB 704.59 KB 150.59 KB 150.9 KB RN_OSS_DEV
ReactNativeRenderer-prod.js 🔺+0.2% 🔺+0.1% 246.91 KB 247.44 KB 43.19 KB 43.25 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js +0.2% +0.2% 252.93 KB 253.52 KB 44.51 KB 44.59 KB RN_OSS_PROFILING
ReactFabric-dev.js +0.2% +0.2% 692.29 KB 693.53 KB 147.99 KB 148.3 KB RN_FB_DEV
ReactFabric-prod.js 🔺+0.2% 🔺+0.1% 240.19 KB 240.66 KB 41.93 KB 42 KB RN_FB_PROD
ReactFabric-profiling.js +0.2% +0.1% 245.49 KB 245.96 KB 43.27 KB 43.34 KB RN_FB_PROFILING
ReactFabric-dev.js +0.2% +0.2% 692.2 KB 693.43 KB 147.94 KB 148.25 KB RN_OSS_DEV
ReactFabric-prod.js 🔺+0.2% 🔺+0.2% 240.2 KB 240.66 KB 41.93 KB 41.99 KB RN_OSS_PROD
ReactFabric-profiling.js +0.2% +0.1% 245.5 KB 245.98 KB 43.27 KB 43.33 KB RN_OSS_PROFILING

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +0.2% +0.2% 585.47 KB 586.7 KB 123.95 KB 124.25 KB UMD_DEV
react-test-renderer.production.min.js 🔺+0.2% 🔺+0.2% 63.62 KB 63.76 KB 19.46 KB 19.5 KB UMD_PROD
react-test-renderer.development.js +0.2% +0.2% 581.09 KB 582.33 KB 122.81 KB 123.12 KB NODE_DEV
react-test-renderer.production.min.js 🔺+0.2% 🔺+0.2% 63.32 KB 63.47 KB 19.27 KB 19.3 KB NODE_PROD
ReactTestRenderer-dev.js +0.2% +0.2% 593.76 KB 595 KB 123.17 KB 123.47 KB FB_WWW_DEV
react-test-renderer-shallow.production.min.js 0.0% 0.0% 11.7 KB 11.7 KB 3.59 KB 3.59 KB UMD_PROD
react-test-renderer-shallow.production.min.js 0.0% 0.0% 11.89 KB 11.89 KB 3.7 KB 3.7 KB NODE_PROD

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.2% +0.3% 574.05 KB 575.28 KB 120.38 KB 120.69 KB NODE_DEV
react-reconciler.production.min.js 🔺+0.2% 🔺+0.5% 63.31 KB 63.46 KB 18.82 KB 18.9 KB NODE_PROD
react-reconciler-persistent.development.js +0.2% +0.3% 571.85 KB 573.09 KB 119.48 KB 119.79 KB NODE_DEV
react-reconciler-persistent.production.min.js 🔺+0.2% 🔺+0.5% 63.32 KB 63.47 KB 18.82 KB 18.91 KB NODE_PROD
react-reconciler-reflection.development.js 0.0% -0.0% 17.15 KB 17.15 KB 5.21 KB 5.21 KB NODE_DEV

Generated by 🚫 dangerJS

@bvaughn

This comment has been minimized.

Copy link
Contributor

bvaughn commented Mar 19, 2019

Purely from a DevTools perspective, this change looks okay (since we call onCommitFiberUnmount before this clean up happens). Haven't considered beyond that scope yet.

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Mar 20, 2019

Can you provide an example demonstrating in which scenario this solves a problem? Thanks.

@bvaughn

This comment has been minimized.

Copy link
Contributor

bvaughn commented Mar 20, 2019

This helps remove memory leaks where effect chains point to old nodes

I'm not sure I agree with describing this as a "memory leak" since there are only ever two fibers (current and alternate). Nothing is "leaked", just "retained" for longer than necessary in some cases.

(Unless maybe there's context here that I'm unaware of.)

@paulshen

This comment has been minimized.

Copy link
Contributor Author

paulshen commented Mar 20, 2019

@gaearon I'm having trouble creating an isolated repro but I can share a real world example.

Repro steps

  1. Log into Discord (https://discordapp.com/app) in your browser.
  2. Click this link. This puts you in a custom build with react-dom NPM 16.8.4.
  3. You can confirm custom build by hovering over gear icon in lower right.
    image
  4. Go into a server (example). Repeatedly switch between two text channels in a server while profiling memory usage. See memory usage go up a lot.

Screenshot 2019-03-19 17 54 53

Repro with this PR

Repeat with this custom build which uses react-dom from this PR.

See that memory is freed when you switch between channels.

Screenshot 2019-03-19 18 13 17

Investigation

When you have a Fiber node reference, it links to all the other nodes in that tree as well as old host nodes via stateNode.

@bvaughn Regarding "memory leak", I've found instances where mounted nodes have nextEffect references to unmounted nodes. This seems possible if a subset of the React tree changes (e.g. one pane of many). Components outside the changed tree are not touched and can continue to hold nextEffect pointers to old nodes within the changed tree. In Discord, the sidebar does not update when you change URL routes from one channel to another within the same server. I confirmed this by observing nextEffect.

Does this sound plausible? I'd love to get an isolated repro and will keep trying. Thanks for looking!

@sebmarkbage

This comment has been minimized.

Copy link
Member

sebmarkbage commented Mar 20, 2019

I think we should probably reset the nextEffect pointer to null during the last traversal over the effect list in the commit phase (most likely passive effects).

This gets more complex in concurrent mode renders can be interrupted tho.

@paulshen

This comment has been minimized.

Copy link
Contributor Author

paulshen commented Mar 20, 2019

@sebmarkbage The PR, as is, takes the conservative approach of removing only Deletion effects from the nextEffect chain. It's done in commitAllHostEffects, the last place deletion effects are needed.

This is an optimization. If we're okay doing another traversal to unwind all nextEffect pointers, I'm happy to do that. If there are passive effects, we can unwind in that traversal. Otherwise, do another traversal.

@paulshen

This comment has been minimized.

Copy link
Contributor Author

paulshen commented Mar 21, 2019

@acdlite looks like there's an active Scheduler rewrite! It might make this PR moot but I think unwinding nextEffect pointers is still relevant in the new version.

@matthargett

This comment has been minimized.

Copy link

matthargett commented Mar 26, 2019

@acdlite can you confirm that #15196 will ultimately resolve this issue? it's not obvious to me that it does in current draft form, but it's a draft after all :)

@paulshen

This comment has been minimized.

Copy link
Contributor Author

paulshen commented Apr 8, 2019

@sebmarkbage @gaearon I've updated this to clear all nextEffect pointers after they're no longer needed (either flushing passive effects or after committing layout effects). This seems simpler than removing only Deletion nodes.

I've also updated this to handle the new fiber scheduler. Let me know if there's anything I can do to move this forward. Thanks!

paulshen added a commit to discordapp/react that referenced this pull request Apr 11, 2019
@matthewwithanm

This comment has been minimized.

Copy link
Member

matthewwithanm commented Apr 23, 2019

@sebmarkbage pointed me to this thread so I thought I'd share this publicly FWIW:

I recently upgrade the WhatsApp web client from React 16.3.1 to 16.8.4 and we got a huge increase in memory consumption: every conversation component we showed was retained forever. We ended up tracing this back to our animation library holding on to a reference to two DOM nodes. These nodes referenced a linked list of Fibers (via __reactInternalInstance$jf6dr7u9fbf), each of which held on to a component instance.

While it doesn't exactly mirror what's going on in our app, I managed to create a minimal repro:

https://codepen.io/matthewwithanm/pen/LvmqJP

I get that this isn't technically a memory leak on React's part, but React made a tiny, very bounded leak (2 nodes) into an unbounded one that scaled linearly with the number of screens visited (!)

@andreieftimie

This comment has been minimized.

Copy link

andreieftimie commented May 7, 2019

We're seeing the same thing as @matthewwithanm points out above, applying this PR on a local fork solved the memory leak issue for us.

@sebmarkbage sebmarkbage mentioned this pull request Jul 9, 2019
0 of 5 tasks complete
@sebmarkbage

This comment has been minimized.

Copy link
Member

sebmarkbage commented Jul 9, 2019

I spawned another issue and tried to gather my thoughts on that one. #16087

Regarding the issue demonstrated in https://github.com/jonnycornwell/potential_react_leak, the next steps would be to find out what in Gatsby is actually causing this to leak since just React doesn't seem to trigger it. It's important to understand exactly the code pattern causing this because we don't know if this PR is a complete solution or if it just happened to minimize the effect enough in the test apps to not be noticed.

I can imagine some patterns that are sufficiently covered by this PR though so probably worth getting it in. I wonder if we could make some kind of regression test. Even if it's a DOM fixture.

@@ -756,6 +756,12 @@ function commitUnmount(current: Fiber): void {
}
}
}

// Remove reference for GC
current.stateNode = null;

This comment has been minimized.

Copy link
@sebmarkbage

sebmarkbage Jul 9, 2019

Member

We won't want to do this indiscriminately for all types of Fibers. They might not all have this field. We're looking to use Flow disjoint unions to enforce that they're consistently used for each type. They don't all use it in the same way where this makes sense.

We should be more specific and only do this for the type where it's needed.

However, which type is this supposed to clean up? Is it just DOM nodes? Is it class instance references? What about the equivalent pattern for Hooks? It's hard to tell without knowing more about the particular pattern this fix is solving for.

I'm guessing that this is somehow meant to release DOM nodes that might be part of larger subtrees and therefore might have back pointers into Fibers. However, where is the root that hold onto the first Fiber that might point back into those anyway?

This comment has been minimized.

Copy link
@paulshen

paulshen Jul 9, 2019

Author Contributor

image

Here's an example where mounted DOM/component link back to detached DOM elements. The root cause in this example is probably the effect pointers from mounted fibers to unmounted ones.

They don't all use it in the same way where this makes sense.

I'm not familiar enough with the internals to know where it doesn't make sense. I believe it though 😄 This was from a naive reading that FiberNodes have this field. Mainly, I was thinking about stateNode pointers for HostText/HostComponent (DOM) and ClassComponent but it didn't seem costly to just reset for all. Should I limit to HostText and HostComponent?

However, where is the root that hold onto the first Fiber that might point back into those anyway?

I know this is the crux of this whole thing but the chains were long, sometimes ending in V8 internals. I very much believe it could be a user-space issue but it was not obvious (to me) and these changes seemed cheap/reasonable enough to prevent cascading effects.

This comment has been minimized.

Copy link
@sebmarkbage

sebmarkbage Jul 11, 2019

Member

This particular example is rooted in a nextEffect. With your other fix, that would be reset to null. So would that be sufficient to fix this particular leak?

Maybe we can split out this part to a separate PR and just land the nextEffect part like you suggested? That way we can see if that is sufficient or not.

nextEffect = firstEffect;
while (nextEffect !== null) {
const nextNextEffect = nextEffect.nextEffect;
nextEffect.nextEffect = null;

This comment has been minimized.

Copy link
@sebmarkbage

sebmarkbage Jul 9, 2019

Member

I think this part of the fix makes sense because otherwise if this node (nextEffect) never gets any updates ever again, it'll now have a reference to a node that might later get deleted. Should be pretty easy to come up with a reduced test case for this.

This comment has been minimized.

Copy link
@paulshen

paulshen Jul 9, 2019

Author Contributor

I was able to come up with an isolated repro. https://github.com/paulshen/react-next-effect-leak/blob/master/src/App.js Live nextEffect pointers can point to zombie nodes. Here is a running sandbox https://etc33.codesandbox.io/. Examine the nextEffect pointer of <Sidebar> after clicking "Remount InnerBody" several times. You can see that it remains pointing at the original (and unmounted) <InnerBody>.

I tried to create this repro in the past but was bamboozled by cloneChildFibers and

workInProgress.nextEffect = null;

This comment has been minimized.

Copy link
@sebmarkbage

sebmarkbage Jul 11, 2019

Member

Maybe just add a comment next to it with a link to your repro. We don't need to add any infra just for this. We can figure that out later.

This comment has been minimized.

Copy link
@paulshen

paulshen Jul 17, 2019

Author Contributor

@sebmarkbage in case it was missed, I opened #16115

Copy link
Contributor Author

paulshen left a comment

Let me know what best next actions are for this PR. Does it make sense to break up into two (stateNode vs nextEffect)? It doesn't look like there are tests asserting Fiber internals but I can add them if desired.

@@ -756,6 +756,12 @@ function commitUnmount(current: Fiber): void {
}
}
}

// Remove reference for GC
current.stateNode = null;

This comment has been minimized.

Copy link
@paulshen

paulshen Jul 9, 2019

Author Contributor

image

Here's an example where mounted DOM/component link back to detached DOM elements. The root cause in this example is probably the effect pointers from mounted fibers to unmounted ones.

They don't all use it in the same way where this makes sense.

I'm not familiar enough with the internals to know where it doesn't make sense. I believe it though 😄 This was from a naive reading that FiberNodes have this field. Mainly, I was thinking about stateNode pointers for HostText/HostComponent (DOM) and ClassComponent but it didn't seem costly to just reset for all. Should I limit to HostText and HostComponent?

However, where is the root that hold onto the first Fiber that might point back into those anyway?

I know this is the crux of this whole thing but the chains were long, sometimes ending in V8 internals. I very much believe it could be a user-space issue but it was not obvious (to me) and these changes seemed cheap/reasonable enough to prevent cascading effects.

nextEffect = firstEffect;
while (nextEffect !== null) {
const nextNextEffect = nextEffect.nextEffect;
nextEffect.nextEffect = null;

This comment has been minimized.

Copy link
@paulshen

paulshen Jul 9, 2019

Author Contributor

I was able to come up with an isolated repro. https://github.com/paulshen/react-next-effect-leak/blob/master/src/App.js Live nextEffect pointers can point to zombie nodes. Here is a running sandbox https://etc33.codesandbox.io/. Examine the nextEffect pointer of <Sidebar> after clicking "Remount InnerBody" several times. You can see that it remains pointing at the original (and unmounted) <InnerBody>.

I tried to create this repro in the past but was bamboozled by cloneChildFibers and

workInProgress.nextEffect = null;

@brad-decker

This comment has been minimized.

Copy link

brad-decker commented Jul 11, 2019

Also confirming that this patch resolves the memory leak that we are experiencing in our application. @sebmarkbage the information here and posted in the umbrella issue has lead to us doing a lot to mitigate the impact of the leak, so thank you for the guidance on what can compound the issue. Still, we feel we are chipping away slowly at the problem, and it’s all worth while to do even in the absence of this issue, but with a looming launch date we are anxiously awaiting this fix.

@sebmarkbage

This comment has been minimized.

Copy link
Member

sebmarkbage commented Jul 11, 2019

@brad-decker Mind outlining what application pattern actually caused the leak similar to how others have posted a screenshot from the Chrome heap snapshots? Maybe you're still leaking large DOM trees and maybe we can help you fix that too rather than just mitigate the leak.

@brad-decker

This comment has been minimized.

Copy link

brad-decker commented Jul 11, 2019

@sebmarkbage absolutely thank you.

We use Nextjs and discovered this issue because on mobile when a user flips back and forth between two of our pages, ones that are a real-world use case / hot path of user traffic, the page crashes. The number of dom nodes in memory is increasing over time with each page view.

image

I am very novice at using the heap snapshot to determine what are the culprits of memory retention, but so far we have discovered a lot of the issues due to realizing that we were using hooks inappropriately and closing over refs and state. We fixed a lot of this via the exhaustive deps rule, but i'm sure we are still doing some things in various files that are not helping the situation.

What i've done to diagnose the issue:

  1. I chose two pages in our application and removed all of the layout that encapsulates them, to verify it wasn't anything higher than the page level holding onto memory. In Next land that means I removed my _app and _document so that each page is isolated.
  2. I added links at the top of the page from one page to the other.
  3. Flipping back and forth between pages, taking snapshots, hunting down retainers and figuring out why they are holding onto nodes.
  4. Removed whole sections of the page entirely to see if i could isolate components that were more problematic than others, etc.

I would love some guidance on how to best work through this. I see 'stateNode' and 'nextEffect' as common retainers of memory but i'm not familiar enough with the architecture to understand the significance of these items.

image

image

image

image

I have a heapshapshot i can send if it would help more than screenshots. Again pretty novice here.

@sebmarkbage

This comment has been minimized.

Copy link
Member

sebmarkbage commented Jul 11, 2019

@brad-decker This is actually super helpful because this is showing a new kind of leak that we haven't seen before in this thread. It looks like a closure that is closing over an internal React Fiber data structure. That is interesting because you probably aren't getting a handle to it yourself. Most likely it's React closing over its own internals. It only does this one case: The dispatch function returned from useReducer which is also used to implement setState from useState.

My guess is that you're leaking a dispatch or setState function in an event listener that you're manually attaching to the window (e.g. in an effect). This would be common in things listening to global key press events or click-outside solutions. You might be using a third party tool that does that.

An example that would cause this:

let [value, setValue] = useState(null);
useEffect(() => {
  window.addEventListener('keyup', () => {
    setValue(...);
  });
});

This is a pretty bad leak that you probably want to fix anyway since you don't want keypresses to potentially cause unexpected side-effects due to this leak.

However, it is super helpful to me because it's a pretty common bug. It's something we can try to add monitoring and logging for, and we can find a fix that is stronger than the one proposed in this PR.

@brad-decker

This comment has been minimized.

Copy link

brad-decker commented Jul 11, 2019

@sebmarkbage the only success I have had in totally resolving our memory issues is by removing the Link component from next js everywhere. I have had a few false positives in my journey so I'm really not sure if I'm on the right track.

image

https://github.com/zeit/next.js/blob/canary/packages/next/client/link.tsx

Does this seem like a good rabbit hole to you?

EDIT: doesn't seem to be the only or even the primary issue.

export const useDimensions = (
  ref: Object,
  throttleTime?: number = 100,
): UseDimensionsReturnType => {
  /**
   * This can be extended to return scroll position and other
   * attributes as needed
   */
  const [height, setHeight] = useState(0);
  const [width, setWidth] = useState(0);
  const [offsetHeight, setOffsetHeight] = useState(0);
  const [offsetWidth, setOffsetWidth] = useState(0);

  const handler = useCallback(() => {
    if (ref.current) {
      const { height, width } = ref.current.getBoundingClientRect();
      const { offsetHeight, offsetWidth } = ref.current;
      setHeight(height);
      setWidth(width);
      setOffsetHeight(offsetHeight);
      setOffsetWidth(offsetWidth);
    }
  }, [setHeight, setOffsetHeight, setWidth, ref, setOffsetWidth]);

  // useMemo to prevent creating this function more than once
  useEffect(() => {
    window.addEventListener('resize', handler);
    return () => {
      window.removeEventListener('resise', handler);
    };
  }, [handler]);

  return { height, width, offsetHeight, offsetWidth };
};

i've moved the calculation around to in the effect, outside the effect, etc, and can't seem to figure this out. setOffsetHeight is retaining dom nodes for sure, and this is our code. Any advice?

@matthewwithanm

This comment has been minimized.

Copy link
Member

matthewwithanm commented Jul 12, 2019

  // useMemo to prevent creating this function more than once
  useEffect(() => {
    window.addEventListener('resize', handler);
    return () => {
      window.removeEventListener('resise', handler);
    };
  }, [handler]);

hey @brad-decker! it looks like "resize" is spelled wrong in the removeEventListener() call so it's never actually being removed! (one reason why the EventTarget API is less than 💯)

@bvaughn

This comment has been minimized.

Copy link
Contributor

bvaughn commented Jul 12, 2019

hey @brad-decker! it looks like "resize" is spelled wrong in the removeEventListener() call so it's never actually being removed! (one reason why the EventTarget API is less than 💯)

That would cause a leak to everything in that function's scope right, since window would have a reference.

@brad-decker

This comment has been minimized.

Copy link

brad-decker commented Jul 12, 2019

Well, dang. Thanks for pointing that out! I am kicking myself for that typo :P

@brad-decker

This comment has been minimized.

Copy link

brad-decker commented Jul 12, 2019

#14387 reading through this and trying to understand the implications of accessing a ref in an effect on memory leaks. The ref is a mutable pointer but its part of the component scope, yeah? Do we put refs in the dependency array of an effect? Using my previous example, the one with the typo, if the handler was created in the effect as per the recommendation in the hooks FAQ for dealing with function, wouldn't I need to also include ref as a dependency? Same question, i suppose, with useCallback?

@bvaughn

This comment has been minimized.

Copy link
Contributor

bvaughn commented Jul 12, 2019

Do we put refs in the dependency array of an effect?

No, it's not necessary to specify refs in the dependency array because refs are expected to be mutated by user code (outside of React, without triggering a re-render).

@brad-decker

This comment has been minimized.

Copy link

brad-decker commented Jul 12, 2019

@sebmarkbage @bvaughn @timneutkens @Timer it turns out that we have a compounding problem with Next and React. The code snippet above with the unfortunate typo was a result of me trying to hack to find the memory leak, that code did not exist prior. While it did help me from continuing to go down a rabbit hole that was caused by my own typo in my search for the underlying leak, that was not the root cause.

on line: https://github.com/zeit/next.js/blob/canary/packages/next/client/link.tsx#L101 of next/link an element is set as the key of a map, this element comes from a ref passed to a child component by the Link component. Link basically forwards props to its child so we can add Next magic to 'a' tags or even 'div' tags. The problem seems to be that either next/link itself is closing over a fiber node by doing this, or something in our code that is adjacent to the Links in some way is holding onto the node.

How I have confirmed this:

  1. I checked out our master branch to make sure I was in the original state of our memory leak.
  2. in node_modules I found the link file and deleted the listeners.set call, and nothing else.
  3. ran performance recording for 25 seconds, and during twenty of those seconds, I flipped back and forth between two pages.

image

before

image (1)

after

image (2)

we still have minor memory leaks in our application, as illustrated by the gradual upwards trend, but it's significantly less prominent now with very clear successful garbage collecting without the listener being set in the map and no code change on our end.


thoughts?

@sebmarkbage

This comment has been minimized.

Copy link
Member

sebmarkbage commented Jul 12, 2019

This line in Link makes me skeptical: https://github.com/zeit/next.js/blob/canary/packages/next/client/link.tsx#L117

componentDidMount fires after the ref callbacks of children. The idea is that they need to be wired before they can be referenced in componentDidMount.

https://github.com/zeit/next.js/blob/canary/packages/next/client/link.tsx#L124

So that would imply that the clean up function gets overridden before it can be called.

Another issue is that makes this code a bit fragile is if handleRef gets called multiple times it doesn't clean up the old ones. E.g. if props switches or something. I don't see that happening now but seems like it would be easy to make such as mistake.

@timneutkens

This comment has been minimized.

Copy link

timneutkens commented Jul 12, 2019

Having @ijjk look into it!

@brad-decker

This comment has been minimized.

Copy link

brad-decker commented Jul 15, 2019

Update, tested the above application against changes made in zeit/next.js#7943 by @ijjk and it seems to be resolved. We have also started fixing a myriad of "minor" leaks that have an additive effect. All of them are related to things already brought up by @sebmarkbage in this and the umbrella thread. Thanks for all of the assistance, @sebmarkbage @bvaughn @matthewwithanm @timneutkens and @ijjk

@linusthe3rd

This comment has been minimized.

Copy link

linusthe3rd commented Jul 24, 2019

Just to note for folks following this thread (as the information can be easy to miss), the author of this PR opened another PR (#16115) to address part of the leak.

This was merged in last week and looks to help my application's memory usage a lot better.

@eltonchan

This comment has been minimized.

Copy link

eltonchan commented Aug 1, 2019

Can you provide an example demonstrating in which scenario this solves a problem? Thanks.

My application memory leak is for this reason

@Fabbok1x

This comment has been minimized.

Copy link

Fabbok1x commented Aug 7, 2019

Edit: This does seem to fix quite a lot.

@eltonchan

This comment has been minimized.

Copy link

eltonchan commented Aug 8, 2019

Can you provide an example demonstrating in which scenario this solves a problem? Thanks.

When will I plan to merge this?

@abirmingham

This comment has been minimized.

Copy link

abirmingham commented Aug 9, 2019

I am noticing a large disparity between the memory leak in my application when NODE_ENV=development versus NODE_ENV=production.

Here is the development leak:
image

Here is the production leak:
image

This is a profile of twice loading/unloading a particular tree in an internal application. I've focused on two spikes which consistently appear and continue to appear regardless of how many times I unload/reload the vertical tree. As you can see, the leak is much worse in production (2MB vs 18MB). This profile was run on 568ef3e.

EDIT: More details here.

@zebralight

This comment has been minimized.

Copy link

zebralight commented Sep 5, 2019

I tried using this fork to resolve some memory leak issues I've been having but I actually ended up getting better results with 16.9 than with this. For a bit of context, I have an app with react router where switching between two components will lead to an accumulation of stale fiber nodes.

@stale

This comment has been minimized.

Copy link

stale bot commented Jan 10, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

@stale

This comment has been minimized.

Copy link

stale bot commented Jan 17, 2020

Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you!

@stale stale bot closed this Jan 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.