Skip to content
This repository has been archived by the owner on Oct 7, 2020. It is now read-only.

Fix a memory leak found by mpickering #1305

Merged
merged 2 commits into from
Jun 21, 2019
Merged

Fix a memory leak found by mpickering #1305

merged 2 commits into from
Jun 21, 2019

Conversation

lorenzo
Copy link
Collaborator

@lorenzo lorenzo commented Jun 20, 2019

closes #1296

@lorenzo lorenzo requested review from alanz and lukel97 June 21, 2019 07:34
@lorenzo
Copy link
Collaborator Author

lorenzo commented Jun 21, 2019

@alanz implemented your suggestion

@mpickering
Copy link
Collaborator

Thanks @lorenzo

@alanz alanz merged commit e2f8e32 into master Jun 21, 2019
@lorenzo lorenzo deleted the fix-leak-mpickering branch June 22, 2019 05:27
@alanz alanz added this to the 2019-06 milestone Jul 7, 2019
@tomjaguarpaw
Copy link
Member

Hello, I came here as a consequence of reading https://lukelau.me/haskell/posts/leak/.

I would be very interested to know, if the information is available, whether any space improvement was actually seen as a result of merging this PR. @bubba's PR had already been merged the day before and should make this PR redundant. In particular, modifyTVar' forces its argument before returning, and subsequent to @bubba's PR forcing the IdeState forces the GhcModuleCache it contains (that's what making a field strict means).

I have spent, many, many, days investigating the causes of various space leaks. I'm trying to come up with a set of principles that make it easy to write space-leak-free code. I would like to test my principles against the space leak that you have discovered. If this PR did indeed remove a space leak then my principles are incorrect and i need to go back to the drawing board!

@tomjaguarpaw
Copy link
Member

(Oh, and I got to Luke Lau's post via https://mpickering.github.io/ide/posts/2020-05-08-state-of-haskell-ide.html)

@fendor
Copy link
Collaborator

fendor commented May 10, 2020

The code-base has changed signifitcantly since this PR has been merged.

@tomjaguarpaw
Copy link
Member

That may be so, but I'm wondering whether there is any residual evidence either way that this PR helped.

@fendor
Copy link
Collaborator

fendor commented May 10, 2020

I am sorry, I dont think we have any hard numbers anymore. Doesnt the blog post state that the leak has been solved?

@tomjaguarpaw
Copy link
Member

Sure, I've no doubt the space leak has been solved. My question is whether it was by this PR or by @bubba's earlier PR.

@lukel97
Copy link
Collaborator

lukel97 commented May 10, 2020

@tomjaguarpaw these PRs might have been created in parallel - it does seem strange that we enforce strictness here as well. You might want to checkout to both of these revisions and try running the script from the blog post to see if it does make a difference. Otherwise this was probably done out of an abundance of caution

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

modifyCache is the source of a space leak
6 participants