Memory leak in ListBox implementation (I think)

[s0561259]

In Widgets.hs the implementation of listBox uses the WriteAttribute items to change the child elements of the ListBox when the supplied Behavior [a] changes and it is defined as:

items :: WriteAttr Element [UI Element]
items = mkWriteAttr $ \i x -> void $ do
    return x # set children [] #+ map (\i -> UI.option #+ [i]) i

Here each new child element gets created with the supplied UI Element and attached to the list box but the old children don't get deleted, just detached. I am not sure if they get garbage collected automatically but I don't think so because I tested this with following snippet and the memory usage of the program increased by about 150mb per minute:

timer       <- UI.timer # set UI.interval 10
bCount      <- accumB (0 :: Int) $ UI.tick timer $> (1 +)

let bItems = flip map [0 .. 30] . (+) <$> bCount

testListBox <- UI.listBox bItems (pure Nothing) (pure $ UI.string . show)
UI.start timer

[sjakobi]

Which GHC version did you test this with?

[R4stafa]

I tested it with GHC 8.10.4 and threepenny-gui 0.9.1 I just tested a different version of the snippet which doesn't use the ListBox from threepenny-gui and calls UI.delete on no longer needed elements but the increase in memory usage looks the same so I don't know what's going on. Maybe I am doing something wrong. Anyway here is the snippet:

  timer  <- UI.timer # set UI.interval 10
  eCount <- accumE (0 :: Int) $ UI.tick timer $> (1 +)

  let unsafeMapUI :: (a -> UI b) -> Event a -> Event b
      unsafeMapUI = UI.unsafeMapIO . (UI.runUI window .)

      mkElem :: Int -> UI Element
      mkElem i = UI.option # set UI.text (show i)

      mkElems :: Int -> UI [Element]
      mkElems i = mapM mkElem [i .. i + 30]

  testListBox      <- UI.select

  ePrevAndCurElems <-
    accumE ([], [])
    $   (\curElems (_, prevElems) -> (prevElems, curElems))
    <$> unsafeMapUI mkElems eCount :: UI (Event ([Element], [Element]))

  UI.onEvent ePrevAndCurElems $ \(prevElems, curElems) -> do
    void $ element testListBox # set children curElems
    mapM UI.delete prevElems

  UI.start timer

I am @s0561259 by the way I previously logged in with my secondary account by accident

[HeinrichApfelmus]

Thanks for reporting! 🤔 Two quick questions:

• Is the memory allocated to the browser process (e.g. firefox) or the Haskell process (e.g. ghc)?
• If it is allocated to the Haskell process: Do you still see the memory leak if you compile the program with -O2?

[R4stafa]

@HeinrichApfelmus Thanks for replying! The continuously increasing memory usage comes from the Haskell part of the program (not the browser) I tried it with -02 but it's still the same

I also tried a simple example where I just create a div with lots of children with a button click and with another button click they get deleted and here the memory usage also never decreases when I delete the Elements it only increases so seemingly the delete function does not help to free the memory either. Here is the code snippet:

setup :: Window -> UI ()
setup window = do
    createBtn <- UI.button #+ [UI.string "Create"]

    UI.on UI.click createBtn $ const $ do
      items       <- mapM (UI.string . show) ([0 .. 10000] :: [Int])
      testListBox <- UI.div # set children items
      delBtn      <- UI.button #+ [UI.string "Delete"]

      UI.on UI.click delBtn $ const $ do
        void $ element testListBox # set children []
        mapM_ UI.delete items
        UI.delete testListBox
        void $ getBody window # set children [createBtn]

      void $ getBody window # set children [testListBox, delBtn]

    void $ getBody window #+ [element createBtn]

[R4stafa]

@HeinrichApfelmus can you reproduce this or do you think it has something to do with my setup? I'm on Arch Linux by the way (no meme intended :D) but I might test this on Windows in a few days when I get to it

[HeinrichApfelmus]

I have now looked into this a bit, and while I'm not sure if I can reproduce this yet, I did find a few issues with my code where garbage collection is probably not handled correctly. I'll fix them and get back to you to ask whether the issue persists.

(That said, GHC has historically had a tendency to not return memory that it has acquired before. So, it may happen that GHC consumes memory, but which is effectively free. But that is probably not relevant yet.)

[HeinrichApfelmus]

There! I have combed through the garbage collection and call buffer logic and fixed a few issues. Does this fix the space leak for you as well?

[R4stafa]

I tested it and the commit d7a7d16 fixed the space leak for me. Thanks! But it seems that Events no longer work (at least the click event). I have to investigate further what exactly does and does not work but when I try to react to a button click with UI.on or UI.onEvent or when I create a Behavior from a Event with stepper and sink an attribute it does not react to the button click. In the previous commit 379a6a5 it still works as intended. Should I open a new issue for this?

[HeinrichApfelmus]

I tested it and the commit d7a7d16 fixed the space leak for me. Thanks! But it seems that Events no longer work (at least the click event). I have to investigate further what exactly does and does not work but when I try to react to a button click with UI.on or UI.onEvent or when I create a Behavior from a Event with stepper and sink an attribute it does not react to the button click. In the previous commit 379a6a5 it still works as intended. Should I open a new issue for this?

Oopsie. 😅 It seems that event handlers are now garbage collected too quickly. I take it that this affects even the most simplest examples? (No need to open a second issue, thanks!)

[R4stafa]

Oopsie. :sweat_smile: It seems that event handlers are now garbage collected too quickly. I take it that this affects even the most simplest examples? (No need to open a second issue, thanks!)

Haha that's kind of funny :smile: Yes a single button with UI.on UI.click btn $ const $ liftIO $ putStrLn "Click!" does not work

[R4stafa]

I investigated this a bit myself because I wanted to learn about the internals of this library and I stumbled upon the error (or rather the warning of my editor did). In your commit d7a7d16 you changed a line in newRemotePtr from

modifyMVar coupons $ \m -> return (Map.insert coupon w m, ())

to

atomicModifyIORef' coupons $ \m -> return (Map.insert coupon w m, ())

but atomicModifyIORef' has type

IORef a -> (a -> (a, b)) -> IO b

The modification function does not return an IO (like with modifyMVar) so the return here refers to the Monad instance of Tuple and not IO :smile:. I removed the return and now everything seems to work but I was really curious why this even compiled so I investigated more and it turns out it's a real coincidence that it did! This only compiled because Tuple is a Monad if its first type argument (in this case Map) is a Monoid (which it is) so what this actually did was return created a Tuple with an empty Map (Monoid instance) as it's first element and (Map.insert coupon w m, ()) as it's second element so this actually deleted the Map inside coupons and ignored the Map.insert function (which my editor warned me about otherwise I would never have found this)

[HeinrichApfelmus]

Great investigation! Yes, that was indeed the culprit; I came to the same conclusion after observing that RemotePtrs not only appeared to be garbage collected too early, they also seemed to never make it into the Vendor in the first place.

Out of curiosity: What was the specific editor warning that brought this to your attention? I noticed that you probably get warnings about ignored return values in do notation; I have switched them off in my editor, because they are usually false positives and are usually fixed by prepending void without thinking too much about it.

[R4stafa]

Yes indeed the warning was about the ignored return value: A do-notation statement discarded a result of type ‘(Map.HashMap Coupon (Weak (IORef (RemoteData a))), ())

I agree that these warnings are usually a false positive (that is you actually want to discard the value) but it never came to my mind to switch off these warnings instead I always just use void. I have not that much Haskell experience yet and this was more of a habit I learned than an educated decision but now I think maybe the advantage of using void or even _ <- (which I find ugly but at least it's even more noticeable) instead of turning off these warnings is that you explicitly and visibly discard a value otherwise you get a warning and in this case even if you just prepend void without thinking too much about it maybe later you see this code and think "this void should be redundant". On that note a warning about redundant voids could be useful too.

Nevertheless I thank you for this wonderful library which I have most fun with and from which I learned a lot! :+1:

[HeinrichApfelmus]

Hm. I'm a bit ambivalent about this warning. 🤔 As you probably noticed, the library uses a particular style where different calls to set can be chained with #. This makes the code quite nice and succinct, but it does rely on not having this warning — after all, the last entry in the chain will always be discarded.

Nevertheless I thank you for this wonderful library which I have most fun with and from which I learned a lot! 👍

😊 Thank you for your report! I had put all the garbage collection stuff into place, but it was never stress-tested properly. Do let me know if you find more issues.

[R4stafa]

Hm. I'm a bit ambivalent about this warning. :thinking: As you probably noticed, the library uses a particular style where different calls to set can be chained with #. This makes the code quite nice and succinct, but it does rely on not having this warning — after all, the last entry in the chain will always be discarded.

Ok that makes sense :thinking:. It's a bit tedious to put a void in front of all these chains and results in needless boilerplate code

:blush: Thank you for your report! I had put all the garbage collection stuff into place, but it was never stress-tested properly. Do let me know if you find more issues.

No problem! I'm glad I could contribute and will gladly do so in the future... maybe even with a pull request as soon as I get more familiar with the internals of this library :smiley:

As the memory leak now seems to be fixed should I close this issue?

[HeinrichApfelmus]

😊

As the memory leak now seems to be fixed should I close this issue?

Yes, please!