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

Fix space leak on cradle reloads #1316

Merged
merged 10 commits into from
Feb 7, 2021
Merged

Fix space leak on cradle reloads #1316

merged 10 commits into from
Feb 7, 2021

Conversation

pepeiborra
Copy link
Collaborator

We were leaking 'PackageExports' values for all past 'HscEnv's.
Fixed by moving the package exports inside the 'HscEnvEq'.

I have also added two experiments to characterise the leak, the main one is 'code actions after cradle edit'. On the Cabal 3 example, this is what the experiment looks like:

image

Copy link
Member

@Ailrun Ailrun left a comment

Choose a reason for hiding this comment

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

LGTM

ghcide/src/Development/IDE/Plugin/CodeAction.hs Outdated Show resolved Hide resolved
@pepeiborra pepeiborra added the merge me Label to trigger pull request merge label Feb 6, 2021
@wz1000
Copy link
Collaborator

wz1000 commented Feb 6, 2021

Now that the hiedb changes have landed, I think this feature would be a good candidate to implement using the database.

@pepeiborra
Copy link
Collaborator Author

Now that the hiedb changes have landed, I think this feature would be a good candidate to implement using the database.

But hiedb doesn't index build depends, does it?

@wz1000
Copy link
Collaborator

wz1000 commented Feb 6, 2021

But hiedb doesn't index build depends, does it?

Ah, right. I misread the PR and thought it was for the project local exports map.

However, we could still index and persist a export map for dependent packages, by indexing the exports directly from the ModIface instead of the .hie file.

This is necessary to prevent leaking the package exports
Expensive and already covered by 'code actions after cradle edit'
This was missing from the list
This fixes the th-linking-test because it restores the previous dynamic
semantics in which the package exports are only evaluated when code actions are
requested.
@pepeiborra
Copy link
Collaborator Author

All the tests pass now, but the benchmark run on master (where the leak is still present) times out. I'll wait for the speed ups in #1315 to land

@pepeiborra
Copy link
Collaborator Author

But hiedb doesn't index build depends, does it?

Ah, right. I misread the PR and thought it was for the project local exports map.

However, we could still index and persist a export map for dependent packages, by indexing the exports directly from the ModIface instead of the .hie file.

That's how the exports map works now, except that it isn't persisted. Do you think that persisting would be worth the effort? I think that's a nice idea, please file an issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants