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

Implementing extended options for console formatter #153

Merged
merged 2 commits into from Oct 29, 2017

Conversation

Projects
None yet
2 participants
@lwalter
Copy link
Contributor

lwalter commented Oct 21, 2017

Closes #50

Overview

Benchee collects statistics that may not always need to be shown to the general user but could still be beneficial to some. This PR introduces the capability to optionally display these "extended statistics", which consist of: minimum runtime, maximum runtime, mode runtime, and sample size. This PR implements it as a single boolean configuration value that when true will enable the console formatter to output all of the mentioned extended statistics. False, obviously will not display them.

Open questions

  • Should displaying these be turned off by default, as it is implemented in this PR?
  • Right now I do not have any formatting for sample size, merely just running the number through to_string/1 should this be formatted perhaps?
  • I felt that both the regular scenario and extended options formatter methods within the console formatter module could be generalized. I tried my hand at dissolving the methods involved for each into a set of methods that could handle any input of scenario data and set of options to ouput. I do have a small hang up on how to call the numeric formatting methods with the correct set of params for each statistic being formatted, e.g. when as ips => call ips_out() vs some other method. Here is a branch that captures what I was trying to accomplish, if you have the spare time please take a look as this implementation might result in a more re-usable, condense console formatter module.
@lwalter

This comment has been minimized.

Copy link
Contributor Author

lwalter commented Oct 21, 2017

Fixing the typespec error reported dialyzer...

@PragTob
Copy link
Member

PragTob left a comment

Wowzies, what a good looking PR! 🎉 Thanks for all the work!

img-20171019-wa0005

Comments mostly inline, I think naming it extended_statistics is better than options.

I think I'd opt to activate it by default (opinions @devonestes ) - otherwise lots of people probably won't realize it's there.

One other thing that I asked myself is whether to show it before or after the comparison 🤔

I might take a quick peek at the other branch, but I think it's fine as is and that could easily be a follow up PR :)

# formatters should end up here but known once are still picked up at
# the top level for now
formatter_options: %{
console: %{
comparison: true
comparison: true,
extended_options: false

This comment has been minimized.

@PragTob

PragTob Oct 21, 2017

Member

I think I'd call it extended_statistics and my initial urge would be to enable it by default. Not sure, but this way it'd be more discoverable. I also tend to flood people with things though :D

"\nExtended options: \n",
"\nName minimum maximum sample size mode\n",
"My Job 100.10 μs 200.20 μs 10101 333.20 μs\n",
"Job 2 200.20 μs 400.40 μs 20202 612.30 μs, 554.10 μs\n"]

This comment has been minimized.

@PragTob

PragTob Oct 21, 2017

Member

cool that we already have double mode in the test here!

scenario_reports(sorted_scenarios, units, label_width)
++ comparison_report(sorted_scenarios, units, label_width, config)]

if config.extended_options do
output ++ [descriptor("Extended options") |

This comment has been minimized.

@PragTob

PragTob Oct 21, 2017

Member

here and in method names I think I prefer calling it extended statistics, not options :)

This comment has been minimized.

@lwalter

lwalter Oct 22, 2017

Author Contributor

Agreed, "Extended options" isn't as explanatory as "Extended statistics".

scenario_reports(sorted_scenarios, units, label_width)
++ comparison_report(sorted_scenarios, units, label_width, config)]

if config.extended_options do

This comment has been minimized.

@PragTob

PragTob Oct 21, 2017

Member

Instead of the if/else I'd rather rely on a pattern match like comparison_report if feasible. i.e. just return an empty array and do ++ extended_statistics_report(args)

@spec extended_options_reports([Scenario.t], map, integer) :: [any, ...]
defp extended_options_reports(scenarios, units, label_width) do
Enum.map(scenarios, fn(scenario) ->
format_scenario_extended(scenario, units, label_width) end)

This comment has been minimized.

@PragTob

PragTob Oct 21, 2017

Member

end please on a separate line :D

end
defp mode_out(mode, run_time_unit) when is_number(mode) do
run_time_out(mode, run_time_unit)
end

This comment has been minimized.

@PragTob

PragTob Oct 21, 2017

Member

iirc this misses a case, mode can be nil if everything appears exactly the same amount of times (aka modeless). Also needs a test case :)

This comment has been minimized.

@lwalter

lwalter Oct 22, 2017

Author Contributor

Good catch! I've added a case where it just outputs the string "None" to be displayed. Is this sufficient or should we remove the mode column altogether maybe?

This comment has been minimized.

@PragTob

PragTob Oct 22, 2017

Member

Nope None is perfect 👍

defp run_time_out(average, unit) do
Duration.format({Duration.scale(average, unit), unit})
defp run_time_out(run_time, unit) do
Duration.format({Duration.scale(run_time, unit), unit})

This comment has been minimized.

@PragTob
"\nComparison: \n"
@spec descriptor(String.t) :: String.t
defp descriptor(header_str) do
"\n#{header_str}: \n"

This comment has been minimized.

@PragTob

PragTob Oct 21, 2017

Member

👍 I like the extraction of descriptor :)

assert descriptor2 =~ ~r/minimum/
assert descriptor2 =~ ~r/maximum/
assert descriptor2 =~ ~r/sample size/
assert descriptor2 =~ ~r/mode/

This comment has been minimized.

@PragTob

PragTob Oct 21, 2017

Member

I somehow like the terminology header better for descriptor2 - wdyt?

This comment has been minimized.

@lwalter

lwalter Oct 23, 2017

Author Contributor

Agreed, modified accordingly

assert result2 =~ ~r/333.30/
assert result2 =~ ~r/50000/
assert result2 =~ ~r/201.20/
assert result2 =~ ~r/205.55/

This comment has been minimized.

@PragTob

PragTob Oct 21, 2017

Member

I think we can omit the repeated asserts from the test before here and for the mode output actually test it as one regex 201.20 units, 205.55 or something like that :)

This comment has been minimized.

@lwalter

lwalter Oct 23, 2017

Author Contributor

Yeah, good call, otherwise it obfuscates the actual behavior under test.

format_scenario_extended(scenario, units, label_width) end)
end

@spec format_scenario_extended(Scenario.t, map, integer) :: String.t

This comment has been minimized.

@PragTob

PragTob Oct 21, 2017

Member

This is where the dialyzer error comes from... typing looks right to me and some builds also passed. Sometimes... dialyzer...

lib/benchee/formatters/console.ex:177: Invalid type specification for function 'Elixir.Benchee.Formatters.Console':format_scenario_extended/3. The success typing is (#{'__struct__':='Elixir.Benchee.Benchmark.Scenario', 'job_name':=_, 'run_time_statistics':=#{'__struct__':='Elixir.Benchee.Statistics', 'maximum':=number(), 'minimum':=number(), 'mode':=[number()], 'sample_size':=_, _=>_}, _=>_},#{'ips':=#{'__struct__':='Elixir.Benchee.Conversion.Unit', 'label':=binary(), 'long':=binary(), 'magnitude':=non_neg_integer(), 'name':=atom()}, 'run_time':=#{'__struct__':='Elixir.Benchee.Conversion.Unit', 'label':=binary(), 'long':=binary(), 'magnitude':=non_neg_integer(), 'name':=atom()}},integer()) -> binary()

This comment has been minimized.

@lwalter

lwalter Oct 23, 2017

Author Contributor

So this is failing locally for me, but I cannot for the life of me discern why this is failing. This method here even uses the exact same typespec and does not fail... https://github.com/lwalter/benchee/blob/8d87ccf0c4cc4354781714370f6c80a13f3391b3/lib/benchee/formatters/console.ex#L186

@PragTob

This comment has been minimized.

Copy link
Member

PragTob commented Oct 21, 2017

Had a quick look at the branch & left a comment. Approach looks neat, getting the respective data together. Looks like it'd make implementing further columns etc. easier 👍

Code needs some work but I know it's an experiment. I'd highly prefer it to be a separate PR after this one cause smaller PRs are much easier to review.

However, still pending a review then - I've grown afraid of magic for readability and maintainability reasons but this might be good.

@PragTob

This comment has been minimized.

Copy link
Member

PragTob commented Oct 22, 2017

Okay I think I flip flopped back on the default. Maybe false is better - after all it's extended statistics and sasmple size/min/max aren't as interesting for most people (although they should be!) and I'm not sure how many know what a mode is. So default false seems fine I guess :)

@lwalter lwalter force-pushed the lwalter:master branch from ec3acd5 to 8d87ccf Oct 23, 2017

@lwalter

This comment has been minimized.

Copy link
Contributor Author

lwalter commented Oct 23, 2017

@PragTob Added a response to your comment on the dialyzer failure, I havent found why the typespec is failing. This method here even uses the exact same... https://github.com/lwalter/benchee/blob/8d87ccf0c4cc4354781714370f6c80a13f3391b3/lib/benchee/formatters/console.ex#L186

@PragTob

This comment has been minimized.

Copy link
Member

PragTob commented Oct 23, 2017

Thanks! I'll see that I cut a release today evening (hopefully) and then I think I'll take to fighting dialyzer 👨‍🎤

Can round up some things and then get this out in another release relatively soon (I just wanna really finally get the hooks in hands and not delay further)

@PragTob

This comment has been minimized.

Copy link
Member

PragTob commented Oct 29, 2017

Taking a look now, sorry for the delay - sometimes life is busy. Can reproduce locally.

@PragTob

This comment has been minimized.

Copy link
Member

PragTob commented Oct 29, 2017

It doesn't seem to like Scenario.t 😱

When I do: @spec format_scenario_extended(any, unit_per_statistic, integer) :: String.t it complains about under spec... when I add the Scenario.t the error pops up.

Which is super weird because the calling method looks like this:

  @spec extended_statistics([Scenario.t], unit_per_statistic, integer)
    :: [String.t]
  defp extended_statistics(scenarios, units, label_width) do
    Enum.map(scenarios, fn(scenario) ->
              format_scenario_extended(scenario, units, label_width)
            end)
  end

we have a list of scenarios and then we pass a single scenario on... should be fine 🤔

@PragTob PragTob merged commit 8d87ccf into bencheeorg:master Oct 29, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
coverage/coveralls First build on master at 94.301%
Details
@PragTob

This comment has been minimized.

Copy link
Member

PragTob commented Oct 29, 2017

I gave up on it... I'll go get help in the elixirforum or something maybe :D Thanks a lot for the contribution. If you still wanna do the restyling, I'd be interested! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.