reflex-frp / reflex-dom

Web applications without callbacks or side-effects. Reflex-DOM brings the power of functional reactive programming (FRP) to the web. Build HTML and other Document Object Model (DOM) data with a pure functional interface.
https://reflex-frp.org
BSD 3-Clause "New" or "Revised" License
357 stars 145 forks source link

Memory leak? #147

Closed eskimor closed 7 years ago

eskimor commented 7 years ago

Memory consumption of gonimo is constantly increasing over time. Visit https://app.alpha.gonimo.com with Chrome, open the task manager (in More Tools). Then click baby and home alternating and watch the memory grow.

The problem seems to be that dom nodes are not released, as can be seen in the attached heap snapshot, where you can find 88 detached dom trees. (I did the clicking game for a while)

You can also see it clearly in the retained dom nodes image: nodes is constantly growing. Am I doing something wrong? Or have I just found a memory leak in reflex-dom?

Used version of reflex-platform: 079c93c218dad6e9125dcb182b1a903571cc251d

I tried to analyze the heap snapshot, but could not make much sense of it - as ghcjs JavaScript is still pretty unreadable to me.

The switching between the baby screen and start screen happens via widgetHold.

retained dom nodes

Heap-20170419T130403.heapsnapshot.gz

eskimor commented 7 years ago

Interestingly, memory stays stable when switching to parent all the time .. investigating.

ryantrinkle commented 7 years ago

@eskimor That sounds bad! Can you provide the exact version of reflex and reflex-dom you're using, as well as reflex-platform if you're using that?

eskimor commented 7 years ago

I am using the reflex-platform with commit hash: 079c93c218dad6e9125dcb182b1a903571cc251d and reflex-dom and reflex that come with it. I am overridding a few packages though (don't think this matters, but for the record):

ghcjs-dom-jsffi: 680e70ebe3f2369b5e7e0d3f1da458557bba74e4
jsaddle-dom: 4ca445075bc03f950d5e387668996c31b51d5496
jsaddle-warp: 2e1bdcf385b69beef02ad733a0ab4d92db44f2df

Apart from that it is an unmodified reflex-platform.

eskimor commented 7 years ago

reflex-dom:

[robert@xps13:~/projects/reflex-platform]$ cat reflex-dom/github.json 
{
  "owner": "reflex-frp",
  "repo": "reflex-dom",
  "rev": "6be990ea6cdea57ef5c3caf2c02d77e0a6c28bb1",
  "sha256": "099qrrj5xfdif53z4jmmczqdbfj1ajdzii7scw812y66h6031drn"
}

reflex:

[robert@xps13:~/projects/reflex-platform]$ cat reflex/github.json 
{
  "owner": "reflex-frp",
  "repo": "reflex",
  "rev": "02f0afb6b9a138e61b9d348e01ece8039583262d",
  "sha256": "04w4xm6mvi520273ls7vl596kizbfgiffqw8rmsf8rrrq9x0gd6f"
}
ryantrinkle commented 7 years ago

@eskimor Knowing that it's a specific widget that causes it is very helpful. If you're able to reduce it to a small test case, I'm sure we'll be able to sort it out.

eskimor commented 7 years ago

working on it!

mulderr commented 7 years ago

@eskimor can you reproduce it over longer periods of time, does it really keep increasing indefinitely? The screenshot shows only around 15s. This is around a minute of recording the code below: timeline

{-# LANGUAGE OverloadedStrings #-}

module Main where

import Control.Monad.Trans (liftIO)
import Data.Time
import Reflex.Dom

main :: IO ()
main = mainWidget $ do
  now <- liftIO getCurrentTime
  e <- tickLossy 0.05 now
  widgetHold blank $ (text "foo") <$ e
  return ()

Interestingly, this gives mostly the same results:

var f = function () {
  document.body.innerHTML = '';
  document.body.appendChild(document.createTextNode('foo'));
  setTimeout(f, 50);
};
f();

I'm not entirely sure but from what I could find Nodes is the number of nodes that are still in memory but can be GC'ed. Which can happen whenever the browser feels like it. I do not think Nodes == detached nodes, those cannot be GC'ed if I understand correctly.

The fact that Nodes is increasing is not surprising at all then. It would be a problem however if it indeed does not periodically drop or if forcing a GC (trash icon in dev tools) does not cause it drop to zero.

Could you try to record again but manually force GC during?

eskimor commented 7 years ago

I noticed the leak, because of a resource leak. I saw that that gonimo kept pulse audio resources indefinitely*) - which caused audio to fail after some time on mobile. I got rid of this by manually setting srcObject on my video tags to null whenever the stream stops, this braught me to investigating why the video tag does not get collected in the first place. So I am pretty sure there is a leak, but I will spend today investigating further! Thanks for you help in any case!

*) at least minutes.

eskimor commented 7 years ago

Also memory keeps increasing over minutes, I also forced manual collection - does not change much.

eskimor commented 7 years ago

but you are right that this short period I posted is pretty meaningless, I just tested again - it keeps climbing - clicked for about a minute, while when I click parent it does not, garbage gets collected periodically. So that's a start, I have a fine case and a broken case - can't be too hard to find the reason.

eskimor commented 7 years ago

Interestingly when compiling with ghc and running with jsaddle-warp I also get a leak when clicking parent ... long-running-parent-ghc

eskimor commented 7 years ago

The slope at the beginning is about a minute, so I left it idle for quite some time (also clicked manual collection).

eskimor commented 7 years ago

Ok I reduced this to a very basic version, which you can see here, I also wrote a JavaScript variant here.

The JavaScript variant suffers from the same problem! I also found a Chrome issue regarding this. So this bug does not seem to be caused by reflex.

The other thing though - the memory leak when using jsaddle is still weird and has nothing to do with video or audio. Can anyone reproduce this with his/her app? In any case this should probably be a separate issue.

3noch commented 7 years ago

@eskimor What did you end up finding?

eskimor commented 7 years ago

dev tools are pretty unreliable in finding memory leaks, although the memory timeline suggested otherwise, it seems that the leak can be fixed by manually setting srcObject on the video to null when the MediaStream ends.

This works in the js version, in the reflex version it looks like it does not work, but in chrome task manager you can see a pretty stable memory consumption if you trigger garbage collection manually.

Most importantly the resource leak is definitely fixed by simply setting srcObject to null. All in all, two whole days got wasted, because this solution, I already had yesterday morning.

Regarding the leak of the ghc/jsaddle version: It could easily be that there is none. I have to check with the task manager, as this seems to be the most reliable way.

3noch commented 7 years ago

Thanks for the rundown. Sorry for the lost time. Good work though.

eskimor commented 7 years ago

With ghc/jsaddle it seems to leak. Also task manager shows increasing memory which also does not go down with forced collection. But this is likely an issue with jsaddle - or something I am doing wrong and not reflex-dom related.

Well I hope this helps others, then it was useful at least ;-)