-
Notifications
You must be signed in to change notification settings - Fork 65
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
memory unit formatter #131
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lib/benchee/conversion/memory.ex
Outdated
@bytes_per_terabyte @bytes_per_gigabyte * @bytes_per_kilobyte | ||
|
||
@units %{ | ||
terabyte: %Unit{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
formatting is a bit curious here, e.g. no other unit seems to be so big to need to indent that far.
lib/benchee/conversion/memory.ex
Outdated
|
||
@units %{ | ||
terabyte: %Unit{ | ||
name: :terabyte, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plus it seems the things within the unit aren't indented, I prefer it more like this:
%{
longest_unit: %Unit{
property1: val1,
prop2: val2
}
}
that's also what it hopefully should be like in the other formatters :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there any tool that could do that for me?
iex> value | ||
1.205078125 | ||
iex> unit.name | ||
:kilobyte |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay, doctests!
unit. In case of tie, chooses the largest of the most common units. | ||
|
||
Pass `[strategy: :smallest]` to always return the smallest unit in the list. | ||
Pass `[strategy: :largest]` to always return the largest unit in the list. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we support 4 strategies, weird to just mention 2. But that might be duplicated from other places anyhow...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point
lib/benchee/conversion/memory.ex
Outdated
end | ||
|
||
@doc """ | ||
The most basic unit in which momory occur, byte. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be memory
:)
lib/benchee/system.ex
Outdated
defp format_memory(memory, coefficient), do: "#{Float.round(memory / coefficient, 2)} GB" | ||
defp format_memory(memory) do | ||
scaled_memory = Memory.scale(memory, :gigabyte) | ||
Memory.format({scaled_memory, :gigabyte}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think just calling Memory.format(memory)
should give us the right result, it mgiht not always be GB as before but if it decides not to do GB then I think that's what we want :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i did not want to change how it was implemented before... and i missed the point of implementing memory formatter :)
lib/benchee/system.ex
Outdated
end | ||
defp parse_memory_for(:Linux, raw_output) do | ||
["MemTotal:" <> memory] = Regex.run(~r/MemTotal.*kB/, raw_output) | ||
{memory, _} = memory | ||
|> String.trim() | ||
|> String.trim_trailing(" kB") | ||
|> Integer.parse | ||
format_memory(memory, @kilobyte_to_gigabyte) | ||
format_memory(memory * 1024) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This impromptu conversion isn't ideal imo. I think it'd be better if we explicitly took it as kilobytes and converted it to bytes and then passed it into th format_memory function
|
||
describe ".format" do | ||
test ".format(1_023)" do | ||
assert format(1_023) == "1023 B" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation seems off here )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but good edge case testing!
… according to code review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, there is still that 1024 in non memory code floating around that I want to get rid off. Not 100% sure which method to call for that myself, so I'll go ahead and add it on top myself and then merge it :)
Thanks!
Shockingly we don't seem to have functions yet to scale units down or convert from one unit to the other 😱 That can be done in a separate ticket though :) |
#121