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

Display retain object size using human readable output #1956

Closed
opatry opened this issue Oct 2, 2020 · 8 comments
Closed

Display retain object size using human readable output #1956

opatry opened this issue Oct 2, 2020 · 8 comments

Comments

@opatry
Copy link
Contributor

opatry commented Oct 2, 2020

Problem description

Since v2.5, retained objects size is displayed ( 👍 ) as byte amount, would be nice to have a more user friendly output like du -h does for instance.
Having MB of leak vs bytes might be more alerting.

Potential solutions

If this part of the code depends on Android APIs, Formatter.formatFileSize might be used.
Otherwise, implementing should be feasible using this kind of algorithm: https://stackoverflow.com/a/3758880

@pyricau
Copy link
Member

pyricau commented Oct 2, 2020

The size is current printed here: https://github.com/square/leakcanary/blob/main/shark/src/main/java/shark/LeakTrace.kt#L143

That module does not have access to the Android APIs.

We should probably go with the SI (ie 1 KB = 1000 B)

@pyricau
Copy link
Member

pyricau commented Oct 2, 2020

Also, funny story, the older version of that stack overflow version (which was flawed) is already in LeakCanary:

@opatry
Copy link
Contributor Author

opatry commented Oct 3, 2020

Ok I'll try implementing it myself given this output.

@opatry
Copy link
Contributor Author

opatry commented Oct 3, 2020

@pyricau RenderHeapDumpScreen currently doesn't use SI, shouldn't we align both usage? (ie SI everywhere as you suggested in your previous comment).

Another question, where should I put the impl of humanReadableByteCount? Do you have a common module for such utils or should I duplicate impl in the :shark module?

@opatry
Copy link
Contributor Author

opatry commented Oct 3, 2020

I've also noticed such output in DisplayLeakAdapter, so might worth extracting a shared impl of humanReadableByteCount somewhere.

@opatry
Copy link
Contributor Author

opatry commented Oct 6, 2020

@pyricau maybe I missed something regarding GitHub issues flow but the issue is still opened despite being referenced in a commit message of a merged PR.
Should I close it myself?
Is it your responsibility as maintainer of the repo?

Maybe I should have used a verb like "closes" or "fixes" while mentioning the issue in my commit message?

(Note: Sorry if it's silly questions, I'm not used to GitHub PR from forks, I use BitBucket server at work and use GitHub on my own with self review for personal projects, not used to collaborate a lot with GitHub)

@pyricau
Copy link
Member

pyricau commented Oct 7, 2020

Yep the best way is to include "Fixes #number " in the commit message, then when its merge the issue auto closes. I should have closed it manually but forgot.

@pyricau pyricau added this to the 2.6 milestone Oct 7, 2020
@pyricau pyricau closed this as completed Oct 7, 2020
@opatry
Copy link
Contributor Author

opatry commented Oct 7, 2020

Thanks for explanation and your patience 🙇

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

No branches or pull requests

2 participants