Fix memory leaks on historyHook and workspaceHistoryHook

[RubenAstudillo]

Description

Both of these hooks leaked memory on long running sessions.

To fix it I had to force the thunks stored on the hooks as the compiler wouldn't be able to see where they would be demanded as they depended on the user interaction. Now the graph looks like this.

The most polemic change on this PR is the inclusion of the parallel library as a dependency. I needed for the Control.Seq module which gave me combinators to force just enough of the Data.Seq and [] data types.

Checklist

• [🗸] I've read CONTRIBUTING.md

• [🗸] I've considered how to best test these changes (property, unit,
  manually, ...) and concluded: It shows the memory improvements!

• [🗸] I updated the CHANGES.md file (None)

[slotThe]

This is neat, thanks!

I wonder if we could get away with just stictifying the respective data structures (since neither is exported); i.e.,

data SP a b = SP !a !b
  deriving (Read, Show)

newtype WorkspaceHistory = WorkspaceHistory
  { history :: [StrictPair ScreenId WorkspaceId]
    -- ^ Workspace Screens in reverse-chronological order.
  } deriving (Read, Show)

---

data HistoryDB = HistoryDB !(Maybe Window) -- currently focused window
                           !(Seq Window)   -- previously focused windows
               deriving (Read, Show)

This is still not ultra-strict but since we're not really interesting in forcing things more than to WHNF it should be fine I think. We may or may not still need the odd call to seq to force the list, not entirely sure about that right now

EDIT: Oh, and

* [🗸] I've considered how to best test these changes (property, unit,
  manually, ...) and concluded: XXX

:)

[RubenAstudillo]

I wonder if we could get away with just stictifying the respective data structures (since neither is exported); i.e.,

data SP a b = SP !a !b
  deriving (Read, Show)

newtype WorkspaceHistory = WorkspaceHistory
  { history :: [StrictPair ScreenId WorkspaceId]
    -- ^ Workspace Screens in reverse-chronological order.
  } deriving (Read, Show)

---

data HistoryDB = HistoryDB !(Maybe Window) -- currently focused window
                           !(Seq Window)   -- previously focused windows
               deriving (Read, Show)

This is still not ultra-strict but since we're not really interesting in forcing things more than to WHNF it should be fine I think. We may or may not still need the odd call to seq to force the list, not entirely sure about that right now

We are interested on forcing things to more than WHNF. On the WorkspaceHistory patch I needed to force at least until each element of each pair on the history list. WHNF on that data structure would be the first cons of that list, not enough.

On a related note, given that we are concerned with forcing evaluation until certain "depth", using strict data types as StrictPair as above is not enough. We would also need:

• A strict list container so we match to WHNF any StrictPair.
• Match on the WorkspaceHistory to WHNF before the put calls. As it is a newtype, it doesn't have extra bottoms. But we still need its head matched, so it forces the strict list and then the StrictPair.

So, instead of doing those two things, we can use the combinators on Control.Seq and match before the put call to get the correct behavior.

The same is true in respect to HistoryDB. We would need a strict Seq data structure. A bang pattern as !(Seq Window) is not enough to deal with this leak as it forces only the head. Every other element remains as a thunk until a undefined point in the future.

EDIT: Oh, and

* [🗸] I've considered how to best test these changes (property, unit,
  manually, ...) and concluded: XXX

Thanks. I should have put something on those XXX. I edited that part on the original post.

[RubenAstudillo]

I was missing the parallel dependency for the test build. Allow the workflows to be re run.

[liskin]

The most polemic change on this PR is the inclusion of the parallel library as a dependency. I needed for the Control.Seq module which gave me combinators to force just enough of the Data.Seq and [] data types.

Would https://hackage.haskell.org/package/deepseq work here as well? Both seem to be maintained by the Core Libraries Committee but deepseq is distributed with GHC so adding that dependency is easier.

[RubenAstudillo]

Yeah, we are basically doing the same as in deepseq now. It is a better dependency. I will use that, re-run my tests and publish an update.

[RubenAstudillo]

@liskin using deepseq exposes other tradeoffs. The NFData typeclass instance has to be derived for WorkspaceHistory. This can be done on two ways per the docs

1. Derive Generic, Generic1 and then NFData. The Generics instances are just to for using the derive anyclass instance for NFData. But these are heavyweigh for just forcing.
2. Use GeneralizedNewtypeDeriving for deriving NFData. This requires a NFData instance on ScreenId on the main xmonad project.

I consider both of these alternatives worse than the current status. Specially considering ScreenId is a newtype, so it doesn't introduce an extra bottom between it and the base Int it contains. So the strictness of ScreenId is equivalent to a shallow seq. I use this on the current code but it is not apparent for the NFData deriving mechanism.

I vote for keep using parallel instead of deepseq. Thoughts?

[slotThe]

I vote for keep using parallel instead of deepseq. Thoughts?

While GeneralizedNewtypeDeriving is probably a no-go as we would like to keep compatibility with xmonad 0.17.0 a bit longer than a few weeks (well, I would, anyways), I don't really see a problem with deriving Generic. It's not like this will affect a lot of data structures such that actual compile times are affected.

I consider both of these alternatives worse than the current status. Specially considering ScreenId is a newtype, so it doesn't introduce an extra bottom between it and the base Int it contains. So the strictness of ScreenId is equivalent to a shallow seq. I use this on the current code but it is not apparent for the NFData deriving mechanism.

While this is true, I think the idea of what we want to achieve is expressed much more clearly by "this is an instance of NFData".

[RubenAstudillo]

@slotThe You are right in that the intent is more clear with an NFData instance!

The memory figures are basically the same. I still had to do an instance by hand but the liftRnf/liftRnf2 and rwhnf from deepseq were useful. Can you give permission to re-run the GH workflows?

[RubenAstudillo]

I think is merge ready.

[slotThe]

LGTM now!

[RubenAstudillo]

Using the latest version with $! the memory profile is the same

Again, let the workflows re-run and I think is ready to merge. Thanks for all you patience and time @slotThe and @liskin 💪 .

[slotThe]

Thanks!