DOM memory leak?

[rleir]

Current Behavior
A clear and concise description of what is happening / what the bug is.
DOM memory climbs when you open a different dataset.

Expected behavior
A clear and concise description of what you expected to happen instead.
DOM memory should be reclaimed when you are finished with a dataset.

How to reproduce
Steps to reproduce the current behavior:

1. Open the chrome task manager with Shift-esc
2. make sure you have a column for memory footprint (this shows DOM memory)
3. http://localhost:4000/ 55k
4. http://localhost:4000/flu/seasonal/h1n1pdm/ha/12y 177k
5. http://localhost:4000/
6. http://localhost:4000/flu/seasonal/yam/ha/12y 232k dropping back to 166k
7. http://localhost:4000/ 170k
8. http://localhost:4000/flu/seasonal/h1n1pdm/na/6y 255k dropping back to 212k

Hi all,
Not bad, it recovers memory. Now rapidly choose a data set then use the back button
4 or 5 times. I am looking at 425k now, and it is not dropping back. Admittedly this is gorilla testing, it would not affect normal users. So let's call this a bug of minor importance. Moving on ..
thanks -- Rick

Possible solution
(optional)

Your environment: if browsing Nextstrain online

• Operating system:
• Browser:

Your environment: if running Nextstrain locally

• Operating system: Ubuntu 91.10
• Browser: Chrome
• Version (e.g. auspice 2.7.0): master (2.14.0)

Additional context
Add any other context about the problem here.

[jameshadfield]

There are two separate, but possibly related, memory problems here as far as I can tell. @rleir it's not clear to me what approach you're using.

Hard (force) refreshing the same page

This seams to result in different behavior depending on the browser. Force-reloading the same auspice page in the same tab (ncov/global is my test set as it's rather large):

• Chrome - memory footprint increases each reload, even after triggering garbage collection each time. 284Mb -> 565Mb -> 813 Mb -> 1.1Gb -> 967 Mb. Live JS memory stays relatively stable around 37Mb.
• Firefox - Memory stays stable at around 250Mb and doesn't increase when refreshing.

Loading new datasets from within auspice

Here are the numbers I get navigating between datasets within the app (i.e. not reloading the page). These seem to indicate expanding memory usage of auspice.

dataset	Chrome "memory footprint"	Chrome "live JS memory"	Firefox memory (snapshot)
zika	139Mb	10Mb	54Mb
ebola	176Mb	19Mb	105Mb
zika	200Mb	22Mb	112 Mb

[rleir]

Hi James , This is a gorilla test, not something that normal users would do so we can say "this is unreasonable, auspice works fine for a real user". Yes, make it a low priority.
To repeat what I was doing:
1/ http://localhost:4000/
2/ choose randomly a dataset, click on the link
3/ click the back button, repeat from step 2

You could consider closing the issue "won't fix", that is a good option.
Thanks -- Rick

[jameshadfield]

I'm going to keep this open, as I do think that memory increases as we load more and more datasets within auspice is a bug (the chrome-specific increases when force refreshing the page are harder to explain). As you say, it's perhaps not typically encountered by users, but should be fixed one day.

[dos077]

This seems to be a bug with React. I poke around the code a bit and looked at some Chrome memory snap shots. Every time the user switch to another virus a new Tree component is render. It seems like the old unmounted Tree components and their Phylotree children remains in the memory, and they can get pretty big pretty soon.

Issue can be replicated by switching between viruses then open up chrome console -> memory -> heap snapshot -> take snapshot -> search for the class "tree"

However many times the user switch between viruses will spam corresponding number of trees in memory, even if it's just switching back and forth between the same viruses.

It appears to be the same bug reported multiple times in React's repo:
facebook/react#18790

[rleir]

Hi James @dos077 Maybe we should not blame it on React. The warnings below from DeepScan have been outstanding for a while, and could be causing the leak by holding on to a reference to the tree after an unmount. I did not have time to track the fix down.
thanks -- Rick

An event listener is added to the 'document' object at 'componentDidMount()'. But it 
is never removed afterwards. Consider calling 'document.removeEventListener()' at 
'componentWillUnmount()'.
src/components/main/index.js (71:5-71:79)
src/components/main/index.js (72:5-75:14)

An event listener is added to the 'window' object at 'componentDidMount()'. But it 
is never removed afterwards. Consider calling 'window.removeEventListener()' at 
'componentWillUnmount()'.
src/components/framework/monitor.js (36:5-36:59)
src/components/framework/monitor.js (23:5-31:6)

[dos077]

I tested out the demo case in React's issue tracker and even with a simple function component with no useEffect, the unmounted component still stayed in memory. So the bug in React is real but it may not be directly related to this issue.

The componentDidMount call doesn't directly add any event listener, but the PhyloTree render call renders out elements with callbacks on events. I am not familiar with D3, I can't say whether that's the best practice or if additional house keeping is needed on the PhyloTree renderer.

And something that's definitely not best practice is the setTimeOut in change.js and label.js. These delayed calls effect DOM elements and are anonymous with no proper queuing and abort... Maybe we should start some new issues to clean up the rendering pipeline...

[jameshadfield]

I can believe that this is being caused by the onHover & onClick handlers within the <Tree> component not being properly cleaned up when we unmount. Thanks for looking into this!

definitely not best practice is the setTimeOut in change.js and label.js

Yes. In short, they are there because we have to wait for d3 transitions to finish first. I once implemented a system to keep track of each (individual) transition and then run the code currently implemented in a timeout. It didn't work as well as hoped, but I can try to track this down. We also use them in the <Map> component where we have to wait for leaflet to update (e.g. on panning) before drawing the d3 overlay.

[rleir]

@jameshadfield Hi James, You don't have to wait in some cases. Define layers as explained in this Stackoverflow post [1]. In my mind, the code would be better without the timeouts.

@dos077 Hi James, agreed about anonymous functions. I see too many in stack traces. When I want to find something in the src tree using grep, they are a problem. And code readability suffers when the function is invoked far from where it is defined. Also, unit test writing can become difficult. Sorry for whining like Homer Simpson. Best practices are discussed in [2] and [3].

But who has time for re-working code?
cheers -- Rick
[1] https://stackoverflow.com/questions/24045673/reorder-elements-of-svg-z-index-in-d3-js
[2] https://stackoverflow.com/questions/3462255/are-anonymous-functions-a-bad-practice-in-javascript
[3] https://www.linkedin.com/pulse/javascript-named-vs-anonymous-functions-chris-ng

[dos077]

@rleir I am not sure if layering is the right approach to the current timeout issues, since the desire effect is to manipulate the existing layer to animate the tree transitions. I suspect d3js or svg should have some kind of callback chain, but again, not personally familiar with it and the existing codes are pretty intricate and delicate.

@jameshadfield I think the easier improvement to the current anonymous timeouts are to have a timeout queue for that has a clearAll() that will be call on the componentWillUnmount call. Below is a quick draft:

const timeouts = {};

export const addTimeout = (component, fn, delay) => {
if (!timeouts[component]) timeouts[component] = [];
const newTimeout = setTimeout(() => {
fn();
timeouts[component] = timeouts[component].filter((to) => to !== newTimeout);
}, delay);
timeouts[component].push(newTimeout);
};

export const clearAllTimeouts = (component) => {
timeouts[component].forEach((to) => {
clearTimeout(to);
});
timeouts[component] = [];
};

Instead of setting a simple timeout, the rendering step will add a timeout to the queue and the queue can be clear by the parent component by calling the clearAllTimeouts(). There are way more elegant solutions, but this is a straight forward drop-in improvement over completely anonymous timeouts.

[rleir]

I like the clearAll idea.

D3 can have CSS transitions but the tree transitions would be too complex for CSS I suspect.

[dos077]

I coded the timeoutQueue with add/remove/clearall. Should be flexible to switch all setTimeout to it. Also added a unit test for the module for documentation/future maintenance. If it looks good I am going to switch all the component timeouts to it:

[rleir]

nice code. I wish it was not in an image tho, you can format it in markup using three back-ticks (this is your code from above):

const timeouts = {};

export const addTimeout = (component, fn, delay) => {
  if (!timeouts[component]) timeouts[component] = [];
  const newTimeout = setTimeout(() => {
    fn();
    timeouts[component] = timeouts[component].filter((to) => to !== newTimeout);
  }, delay);
  timeouts[component].push(newTimeout);
};

export const clearAllTimeouts = (component) => {
  timeouts[component].forEach((to) => {
    clearTimeout(to);
  });
  timeouts[component] = [];
};

But this is not colorized as your image is.