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

PoC for reducing resolution of data to make it feasible to analyse long application runs #53

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

stoperro
Copy link

@stoperro stoperro commented Aug 6, 2021

The existing strip option helps to reduce size of data so that it can be analyzed on machines without terabytes of RAM. This is done by removing all allocations that didn't exist at the end of test. Unfortunately this loses information that may be necessary to decide which of the visible allocations may be a leak as the trend of allocation is unknown (all visible allocations will be seemingly monotonically increasing).

For that, instead in this PR squeeze-resolution option is introduced that reduces number of samples to smaller value (e.g. 100 from 100000000), combining multiple allocations so that "plots" of allocations look as original - but with reduced resolution, similarly to reducing 8k image into 128x128 one. The loss of detail is usually unimportant when analyzing memory allocation trend - as what matters is if memory remains at stable levels or is constantly growing, a question which plot of smaller resolution can answer.

The PR is behind master, but it doesn't matter as by no means I think the code provided in PR could be merged upstream as it's most likely even occasionally incorrect, i.e. calculated values may be a bit off. Though this doesn't necessarily matter when finding memory leak and this PoC may be good enough for most people wanting to analyze .dat files of huge size (e.g. 40GiB).

@koute
Copy link
Owner

koute commented Aug 6, 2021

FYI, the strip subcommand can now be used to only partially remove temporary allocations with the --threshold argument, so if you e.g. pass --threshold 60 then it will only keep allocations which lived at least for 60 seconds.

Your approach's pretty interesting too, collating multiple allocations into fake ones; I wouldn't be against merging something like this, however:

  1. It'd be better to just simply integrate this into the strip subcommand, and make it one-pass only with the sampling interval passed in externally.
  2. The fake allocations should be flagged as fake, and probably be hidden/filtered out by default in the GUI.
  3. Mmaps should probably be ignored for now; the way those are currently handled needs a rework anyway.

... and probably something else that I forgot. (:

@stoperro
Copy link
Author

stoperro commented Aug 6, 2021

so if you e.g. pass --threshold 60 then it will only keep allocations which lived at least for 60 seconds.

I presume many (most?) of allocations are such temporary "noise" and this method removes it saving RAM without sacrificing too much readability? I worry though a bit if this could give wrong impression in some corner case. To be honest, my method could be sometimes confusing too, as depending if "fake allocations" are created as average or maximum over period it will show a bit different plot (though, I assume not affecting decision if something is leak or not).

Yeah I think it would be best to have it in strip command as in my work I would never even want to remove all temporary allocations if I can just reduce resolution.

I was implementing this PR a while back and was on a limit to comprehend what I'm doing with those streams in Rust not to much additional processing passes etc...

I worried that those fake ones can be misleading to user as they show as if someone really did those allocation in GUI... though AFAIR I merge everything in them, so actually after this operation there was no other allocations than fake ones. Wasn't sure how to properly express it in GUI.

mmaps I was completely improvising, also reallocs etc. I didn't have data to doublecheck how to handle them, so may be all wrong as I was merely guessing proper handling :(

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.

2 participants