Skip to content
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

Remove document from state table when it is closed. #753

Merged
merged 7 commits into from
Jul 9, 2024

Conversation

rtetley
Copy link
Collaborator

@rtetley rtetley commented Mar 13, 2024

Closes #751

  • make the limit configurable
  • what to do when tabs are invisible but not purged?

@rtetley rtetley requested a review from gares March 13, 2024 13:29
@gares
Copy link
Member

gares commented Mar 23, 2024

We could use ocaml GC module to know the memory footprint and let the user declare a cap above which we purge closed tabs.

@gares
Copy link
Member

gares commented Jun 27, 2024

@rtetley I did look at this PR implementing the memory reclaim when the memory footprint > 4G (should be configurable I guess).

There are two problems:

  • I added a visible attribute to each tab, and there is a lot of code that is done on all tabs. Now we could only operate on visible tabs, but before this patch I really don't understand what the code was doing
  • Removing tabs for the states table does not seem to help much in reclaiming memory, so either I'm misusing the Gc API or the state is also referenced by something else. Try to open a large file, like Pfff.v and close the tab. I now print how much memory was reclaimed.

@gares
Copy link
Member

gares commented Jun 27, 2024

Thanks Gaetan, that clearly helped! Indeed the stack is a root. Kudos.

There is still something bad, I don't know if we should used some memory profiler.
If I open and close Pfff (and set the treshold to 0G, so that it always collects):

[lspManager          , 722507, 1719490340.485819] memory footprint 3967614976 -> 2680127488

(3.97G -> 2.68G)
while if I open a trivial file

[lspManager          , 722939, 1719490376.821685] memory footprint 93913088 -> 124223488

(93.9M -> 124M)
So clearly just parsing Pfff leaves a crapload of memory hanging

@gares
Copy link
Member

gares commented Jun 27, 2024

Closing the trivial buffer actually increses the memory, I don't know how Gc.compact can result in that.

@SkySkimmer
Copy link
Contributor

SkySkimmer commented Jun 27, 2024

Closing the trivial buffer actually increses the memory, I don't know how Gc.compact can result in that.

Since you measure heap_words which is the major heap size, maybe something got promoted from minor to major?
Or it got resized but the live_words count decreased?

@gares
Copy link
Member

gares commented Jun 27, 2024

update it is not reproducible, this time it went down to 500M, that is still huge, but 5 times smaller. So I guess the measure implemented in this PR is OKish.

The other question for @rtetley is still valid: why is the code doing something on invisible tabs? Is it wanted? Should I systematically skip working on invisible tabs (even if their state is not purged)?

@SkySkimmer
Copy link
Contributor

What is the state of the installed summary (eg the parser state, Global.env, etc) when running the Gc.compact?

@gares
Copy link
Member

gares commented Jun 27, 2024

Exactly, this is what could have changed, since this time I worked on the smaller tab before closing the huge one.

It means that calling compact without installing a minimal state may not work well. Looking into it.

@gares
Copy link
Member

gares commented Jun 27, 2024

Bingo! it now goes to 100M without fiddling with other tabs

let path = DocumentUri.to_path textDocument.uri in
begin match Hashtbl.find_opt states path with
| None -> assert false
| Some { st } -> replace_state path st false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for doing replace with visible = false then filtering instead of doing Hashtbl.remove?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't necessarily want to remove the execution state of the document when you close it. For example you might close a document by mistake.

Copy link
Member

@gares gares Jun 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point is that, if memory allows for it, we want to keep the coq sessions alive.

It is not uncommon to close a tab and reopen it.
In VSCode you have the usual browser shortcut for it, ctrl-w to close and shift-ctrl-t to open the last closed tab.
In this scenario we want to be nice to the users and not lose their session.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does reopening the tab get the saved state with the current code?

Also I guess the consider_purge should be delayed a bit since big sessions are the ones that are the worst to lose by mistake.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. Probably not at the moment.

For the consider_purge a reasonable alternative would be to send a popup message to the user when memory runs out asking him if he wants to purge ?

@rtetley rtetley force-pushed the delete-file-on-close branch from a44edc6 to eca732a Compare July 9, 2024 05:43
@rtetley
Copy link
Collaborator Author

rtetley commented Jul 9, 2024

I think we are good to go. @gares ?

@gares gares merged commit 1cb2b94 into main Jul 9, 2024
23 checks passed
@gares gares deleted the delete-file-on-close branch July 9, 2024 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Closed Files Still Use Memory
3 participants