Fix space leak in dynamic event switching

[ocharles]

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:

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.

[ocharles]

I think I prefer this approach as we don't have to maintain an invariant in all Pulses/LatchWrites/Outputs, which in the other PR have to store a weak pointer to themselves. There's a minor cost to this PR though, in that it means whenever we change parents we allocate and call finalize, which does increase the cost of dynamic switching. I'm curious which approach others prefer.

[HeinrichApfelmus]

I think I prefer this approach

I prefer this approach as well, as finalizing the weak pointer is very much in line with the actions that you would naturally expect removeParents to do.

The most curious thing is probably that calling finalize on a weak pointer will make the weak pointer unreachable — this behavior is not documented in System.Mem.Weak! The documentation only says "A heap object is reachable if … It is a weak pointer object whose key is reachable." We may want to consider a small patch to the GHC documentation. 🤔

[ocharles]

Indeed - it took three of us two days at ZuriHac to figure this out! It is documented, but in true Haskell fashion - only via a paper 🤦 See Stretching the Storage Manager, section 5.3 and again in 5.5. I agree docs could be better though!

[HeinrichApfelmus]

I agree docs could be better though!

Perhaps — could you add a small comment to that effect in the code — e.g. mention that finalize w does make w unreachable? For posterity and eternal fame.

[HeinrichApfelmus]

It is documented, but in true Haskell fashion - only via a paper 🤦 See Stretching the Storage Manager, section 5.3 and again in 5.5.

When originally writing the code, I had spent a lot of time with this paper — and was a bit disappointed when GHCJS implemented something less sophisticated. 😅

[ocharles]

Added a comment with a link back to this PR.

disappointed when GHCJS implemented something less sophisticated

Yes, this gives me nasty flashbacks to years and years ago when I was trying to use reactive-banana with GHCJS! I have no idea if things got better, but it certainly wasn't fun back then.

[HeinrichApfelmus]

Thanks! Feel free to merge pull requests that @HeinrichApfelmus has ✅ approved with a Github review. 🤓🙏