From 7b1718bb9e6aa198efad7f226eb89730ce0b6d3d Mon Sep 17 00:00:00 2001 From: Ollie Charles Date: Thu, 16 Jun 2022 08:12:18 +0100 Subject: [PATCH] Fix space leak in dynamic event switching (#256) Currently we have a nasty space leak in dynamic event switching. The crux of the problem is that we have a child event that needs to have a new parent. When we change the parent of a node, we need to create a weak pointer to the new parent and store it in the child's list of parents, and we need to store a weak pointer to the child in the new parent node. Currently `connectChild` does this by creating a new weak pointer to the child whenever the parent changes: ```haskell w <- mkWeakNodeValue child child ``` Here we create a weak pointer to the child `SomeNode`, kept alive by the underlying `IORef` that backs `child`. The problem with this approach is that the `child` `IORef` may be reachable throughout the entire duration of the program execution (e.g., `child` is connected to a top-level `reactimate`). This is problematic, because a weak pointer is kept alive by the garbage collector if the key of the weak pointer is alive *regardless* of whether anyone actually has a reference to the weak pointer itself. In the case of `switchE`, this means any time we switch a child to have a new parent, we allocate one more weak pointer that can never be garbage collected. The fix to allow the old weak pointers to be garbage collected is to call `finalize` on them, which we now do in `removeParents`. `removeParents` is responsible for removing a child from all current parents, so when we remove ourself from a parent (removing the weak pointer from the parent), we finalize the weak pointer. Fixes #253, #152. --- .../src/Reactive/Banana/Prim/Low/Dependencies.hs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/reactive-banana/src/Reactive/Banana/Prim/Low/Dependencies.hs b/reactive-banana/src/Reactive/Banana/Prim/Low/Dependencies.hs index db79513c..67304117 100644 --- a/reactive-banana/src/Reactive/Banana/Prim/Low/Dependencies.hs +++ b/reactive-banana/src/Reactive/Banana/Prim/Low/Dependencies.hs @@ -72,7 +72,18 @@ removeParents child = do forM_ _parentsP $ \w -> do Just (P parent) <- deRefWeak w -- get parent node finalize w -- severe connection in garbage collector - let isGoodChild w = not . maybe True (== P child) <$> deRefWeak w + let isGoodChild w = deRefWeak w >>= \x -> + case x of + Just y | y /= P child -> return True + _ -> do + -- The old parent refers to this child. In this case we'll remove + -- this child from the parent, but we also need to finalize the + -- weak pointer that points to the child. We need to do this because + -- otherwise the weak pointer will stay alive (even though it's + -- unreachable) for as long as the child is alive + -- https://github.com/HeinrichApfelmus/reactive-banana/pull/256 + finalize w + return False new <- filterM isGoodChild . _childrenP =<< readRef parent modify' parent $ set childrenP new -- replace parents by empty list