Closed RubenAstudillo closed 2 years ago
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:
StrictPair
.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.
I was missing the parallel
dependency for the test build. Allow the workflows to be re run.
The most polemic change on this PR is the inclusion of the
parallel
library as a dependency. I needed for theControl.Seq
module which gave me combinators to force just enough of theData.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.
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.
@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
Generic, Generic1
and then NFData
. The Generic
s instances are just to for using the derive anyclass instance for NFData
. But these are heavyweigh for just forcing.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?
I vote for keep using
parallel
instead ofdeepseq
. 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 baseInt
it contains. So the strictness ofScreenId
is equivalent to a shallowseq
. I use this on the current code but it is not apparent for theNFData
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".
@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?
I think is merge ready.
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 :muscle: .
Thanks!
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 theControl.Seq
module which gave me combinators to force just enough of theData.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)