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

CAD-406 fixed space leak in LogBuffer #502

Merged
merged 2 commits into from
Jan 15, 2020

Conversation

CodiePP
Copy link
Contributor

@CodiePP CodiePP commented Jan 15, 2020

description

  • there was a space leak in LogBuffer; this fix was possible with great input from @ksaric

testing

1.) compile using stack --nix --profile
2.) run using stack --nix --profile exec example-complex -- +RTS -hc -L45 -RTS
3.) analyse: hp2ps -c example-complex.hp

checklist

  • compiles (cabal new-clean; cabal new-build)
  • tests run successfully (cabal new-test)
  • documentation added and created (cd docs; nix-shell --run make)
  • link to an issue
  • link to an epic
  • add estimate points
  • add milestone (the same as the linked issue)

Signed-off-by: Alexander Diemand <codieplusplus@apax.net>
@CodiePP CodiePP added the bug Something isn't working label Jan 15, 2020
@CodiePP CodiePP added this to the S4 2020-01-16 milestone Jan 15, 2020
@CodiePP CodiePP requested a review from ksaric January 15, 2020 13:28
@CodiePP
Copy link
Contributor Author

CodiePP commented Jan 15, 2020

image

@CodiePP CodiePP changed the title fixed space leak in LogBuffer CAD-406 fixed space leak in LogBuffer Jan 15, 2020
@CodiePP CodiePP added the Byron Byron rewrite label Jan 15, 2020
@@ -69,7 +69,7 @@ nameCounter (Counter RTSStats _ _) = "RTS"
\subsubsection{CounterState}\label{code:CounterState}\index{CounterState}
\begin{code}
data CounterState = CounterState {
csCounters :: [Counter]
csCounters :: ![Counter]
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that this just force the list to WHNF. Using _ to indicate a thunk, this changes csCounters = _ into csCounters = _ : _, so this is not forcing all the elements of the list.

Copy link
Contributor

Choose a reason for hiding this comment

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

If this really is leaking space (we need some evidence for that first), then removing the bang is not the solution. Either use a stricter data structure than a list or force its contents (e.g., Cardano.Prelude.forceElemsToWHNF).

Copy link
Contributor

@ksaric ksaric left a comment

Choose a reason for hiding this comment

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

While this does remove the memory leak, I have to admit that I'm still unable to completely figure out how the leak is occurring.
My assumption would be that the leak is happening because LogObject is referenced here so it cannot be GC'd, but interestingly the bang pattern on lo does no good.

iohk-monitoring/src/Cardano/BM/Data/Counter.lhs Outdated Show resolved Hide resolved
@mrBliss
Copy link
Contributor

mrBliss commented Jan 15, 2020

Can you show a heap profile of before in addition to the one after? Otherwise, it seems like you're just adding bangs in random places.

Signed-off-by: Alexander Diemand <codieplusplus@apax.net>
@mrBliss
Copy link
Contributor

mrBliss commented Jan 15, 2020

Can you show a heap profile of before in addition to the one after? Otherwise, it seems like you're just adding bangs in random places.

@CodiePP informed me that the JIRA issue has a heap profile before and after.

Rather annoying that we have to start looking at multiple places now for information, instead of just GitHub, the place we are already using for issues, PRs, and code.

@CodiePP CodiePP merged commit 15e7df2 into master Jan 15, 2020
@iohk-bors iohk-bors bot deleted the cad-406-space-leak-in-LogBuffer branch January 15, 2020 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Byron Byron rewrite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants