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

Disambiguate memory units in favor of IEC names #136

Closed
aalexand opened this issue Apr 27, 2017 · 7 comments
Closed

Disambiguate memory units in favor of IEC names #136

aalexand opened this issue Apr 27, 2017 · 7 comments

Comments

@aalexand
Copy link
Collaborator

On reports, pprof uses B/kB/MB/GB to represent 1/2^10/2^20/2^30, which as at least one user pointed out is ambiguous at best. We should probably remap these to B/KiB/MiB/GiB, which should be more accurate. The only compat concerns would be for people that parse pprof's output, and we offer no guarantees not to break them.

The other place where we use units is on the unit fields in profile.proto and the command line override, where we recognize kb/mb/gb to mean kiB, MiB, GiB, etc... This is also ambiguous at best, and problematic because changing it could cause us to stop parsing existing profiles. In reality I don't know if we have any profile sources that currently generate profiles with anything other than 'b', but if there are we would break them. A quick search over the accessible code shows only profiles generated using 'bytes', so this is probably worthwhile to do in general. It would only affect people that are used to specifying units by hand to pprof. For example, pprof -unit=mb would change from MiB to MB, and instead users would have to say pprof -unit=MiB (or maybe pprof -unit=mib).

@aalexand aalexand changed the title Disambiguate memory unit names in favor of IEC units Disambiguate memory units in favor of IEC names Apr 27, 2017
@rsc
Copy link
Contributor

rsc commented Apr 27, 2017

Please please please don't do that. It's just needlessly pedantic. Just accept that on computers MB means 2^20 and so on.

@ahh
Copy link
Contributor

ahh commented Apr 27, 2017

Not everyone does accept that, and when I read a pprof profile I have no idea which sort of person the author is.

Saying KiB makes it absolutely clear.

@ahh
Copy link
Contributor

ahh commented Apr 27, 2017

@aalexand Also, should I file a separate issue for needing larger units (TiB and PiB) or is that worth dealing with in one fell swoop?

@aalexand
Copy link
Collaborator Author

aalexand commented Apr 27, 2017 via email

@aalexand
Copy link
Collaborator Author

I filed this request at ahh@'s ask to discuss it.

In practice there isn't a lot of ambiguity about MB / GB for memory units - assuming powers of 2 are used is a safe bet. E.g. top shows RSS as like "123M", and that expectedly means MiB.

That said, the clarity becomes more important when a profile represents larger amounts of memory as at TB vs. TiB the difference becomes ~10%.

@rauls5382
Copy link
Contributor

How about if we leave the current defaults as-is (as broken as they can pedantically be considered), but we support users specifying IEC units on the command line (eg -unit=GiB or even perhaps even -unit=IEC)?

Would that please/appease/at least not annoy everyone?

aalexand added a commit to aalexand/pprof that referenced this issue Apr 30, 2017
This fixes google#137 (but not google#136, this will come separately once we decide
what we want to do if anything).
rauls5382 pushed a commit that referenced this issue May 1, 2017
* Add support for terabyte and petabyte memory units.

This fixes #137 (but not #136, this will come separately once we decide
what we want to do if anything).

* Add a test.

* Update copyright year
@rauls5382
Copy link
Contributor

Closing this due to lack of interest. Reopen if you care about it.

gmarin13 pushed a commit to gmarin13/pprof that referenced this issue Dec 17, 2020
* Add support for terabyte and petabyte memory units.

This fixes google#137 (but not google#136, this will come separately once we decide
what we want to do if anything).

* Add a test.

* Update copyright year
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

No branches or pull requests

4 participants