-
-
Notifications
You must be signed in to change notification settings - Fork 368
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Garbage collection of dirty keys #2263
Conversation
b57b779
to
fd86d4e
Compare
ca47c29
to
fd36e18
Compare
1a97462
to
09fe2dd
Compare
770b91c
to
2897dd3
Compare
This is now ready for review and dogfooding, since I'm not sure whether the problem with GC breaking diagnostics still exists. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great, this was really needed
Maybe we should wait to merge this after the next release? |
Maybe related: #713, which is asking for garbage collect diagnostics from files not in or not linked to any actual file of interes |
Thanks for linking it. That ticket is about cleaning up diagnostics, not related to this. But there's a comment by @mpickering on performing garbage collection only when the FOI set is modified that is very relevant. |
c163dc8
to
e2c8d3a
Compare
e23f7c6
to
78ca34b
Compare
Looks good. However, I wonder if it would be neater to use an LRU datastructure to store the hls-graph database itself, instead of manually triggering garbage collection on idle. |
Yesterday I changed the collection to trigger on file close, rather than on idle. We rely on dirtiness to inform on which keys are not relevant and can be deleted, so I'm not sure that LRU would be helpful. I have updated the PR description to reflect this. |
78ca34b
to
4f68223
Compare
The "visited age" metric is not accurate in hls-graph because of reverse-dependencies-guided work avoidance
a43af6a
to
f8d11d3
Compare
I am dogfooding this MR @pepeiborra, thanks for working on it. |
Is this still the plan or should we go ahead and merge ? |
We are gonna [skip circleci] |
i think we have to postpone the release so i would go ahead and merge |
This runs a garbage collection process when a file is closed, releasing all the keys that remain dirty after a build of all the open files. Essential keys are not GC'ed to avoid information loss and inefficient rebuilds.
For example, if we have A, B and C open in the IDE and close A, the garbage collection will release all the keys that were specific to A, and preserve all the keys that are still needed by B and C.
I have been able to validate heap usage reductions after closing all the files in the editor and triggering a keys GC followed by a heap GC. So it does seem to do what we want.
TODO
EDIT: moved EKG to another PR since it introduces new deps that break the GHC 9 build