Extract runner and move to scenarios #96

devonestes · 2017-07-06T11:23:32Z

Ok, this is a biggie, so I tried to have each commit be a standalone piece that could be reviewed on its own. I took a pretty methodical approach, so hopefully if you follow the commits you'll get a sense of the changes I made at each step.

It still feels a little "incomplete" without the memory usage stuff in there - especially the console formatter - but all tests are passing and it feels to me like it's in a good place now.

This commit has Benchee.Benchmark refactored and in good shape (including documentation) after extracting the new runner from that module. The implementation at this point is still pretty much broken, but next is to get the runner up to shape and tested. Once the runner is implemented and tested, then we'll need to update the statistics.

At this point pretty much all the test and code for `Benchee.Benchmark.Runner` is good to go. Next up is re-doing the statistics to add that stuff to the scenarios instead of to a map on the suite.

This was an easy update! Now `Benchee.Statistics` is good and all tests are updated and passing for that module.

This commit updates the console formatter and associated tests, as well as the top level integration tests since this was the last step in the rough refactoring. There is some additional cleanup I'd like to do (like changing some names and such), but I wanted to at least get the suite green before I did any additional cosmetic things. Yay - the hard part is done!

I forgot we had those old keys in `%Benchee.Suite{}` that could be removed at this point, so I did that and updated the associated tests.

devonestes · 2017-07-06T11:24:08Z

lib/benchee/benchmark/scenario_context.ex

@@ -0,0 +1,21 @@
+defmodule Benchee.Benchmark.ScenarioContext do
+  defstruct [


I added the fast warning and run_time here pretty much just so I
had a consistent way of running warmups and real runs without having to have
additional information. It's essentially duplicating data from the config,
though. Is this a problem?

Seems like a smell to me :)

Also, on further inspection - why do we carry along the whole config when the only values we seem to be using are warmup, run_time and show_fast_warning? (If I didn't forget anyone)

We could just pick those values from the configuration and drop the configuration itself in the ScenarioContext imo. Anything speaking against this?

I somehow also don't feel like updating the ScenarioContext for the warmup is necessarily the best as I see it more immovable than this and a bit confusing (as it's the context for the whole scenario, if it changes based on warmup adn run time there might be another ExecutionContext but that seems like overkill). I feel like passing the benchmarking function the time it should run for would still be viable but I'd need to play with that to see how many arguments it adds where. Would like to play with that after this is merged, so definitely can go ahead with the ScenarioContext being adjusted. Would still like to explore dropping the dependency on configuration :)

Looks like we're using the whole config when passed to the printer for a function here: https://github.com/PragTob/benchee/pull/96/files#diff-402aedd43cbd599699f2ab3157ffdf8aR34 and here: https://github.com/PragTob/benchee/pull/96/files#diff-402aedd43cbd599699f2ab3157ffdf8aR35

We also use parallel here: https://github.com/PragTob/benchee/pull/96/files#diff-402aedd43cbd599699f2ab3157ffdf8aR36

I think for now, leaving config as a whole in the ScenarioContext is a good first step, especially since I'm not 100% sure how all this will change with the introduction of measuring memory usage...

Ok, back to playing around with removing those duplicate values!

Some of the function names didn't fit anymore, so I updated them, changed some typespecs, and used the shorthand function definition in a few places.

PragTob · 2017-07-13T10:30:41Z

FYI le me is 🤧 so sorry the review will still take a while :'(

devonestes · 2017-07-13T12:21:38Z

No problem - feel better! 😷

PragTob

Looks great, thank you a ton for the amount of work you put into this! 💚 👍

After all this, I believe you deserve a good nap.

The biggest discussion point I can think of as of now, is what goes into ScenarioContext et. al. but the discussion point for that is higher up.

I"m really positive for this to land soon 🙂

edit: oh yeah the & stuff, we could leave it in and I just remove it later or you convince me that it's amazing :)

PragTob · 2017-07-15T10:59:46Z

lib/benchee/benchmark/runner.ex

+  for `warmup` time without gathering results and them running them for `time`
+  gathering their run times.
+
+  This means the total run time of a single benchmarking job is warmup + time.


more precisely benchmarking scenario :)

PragTob · 2017-07-15T11:06:34Z

lib/benchee/benchmark/runner.ex

+                   config: %Configuration{warmup: warmup}
+                 }) do
+    warmup_context = %ScenarioContext{scenario_context | show_fast_warning: false,
+                                                         run_time: warmup}


Hm yeah not a huge of duplicating these keys alongside having them in config, first thought was "wait... can you do this, this has to be on config, doesn't it?" More comments on the struct :)

PragTob · 2017-07-15T11:13:23Z

lib/benchee/benchmark/runner.ex

+      }
+      try_n_times(scenario, new_context)
+    end
+  end


I still look at this and I wanna make try_n_times and determine_n_times one. Not here, alas.

PragTob · 2017-07-15T11:22:46Z

lib/benchee/benchmark/scenario_context.ex

@@ -0,0 +1,21 @@
+defmodule Benchee.Benchmark.ScenarioContext do
+  defstruct [


Seems like a smell to me :)

Also, on further inspection - why do we carry along the whole config when the only values we seem to be using are warmup, run_time and show_fast_warning? (If I didn't forget anyone)

We could just pick those values from the configuration and drop the configuration itself in the ScenarioContext imo. Anything speaking against this?

I somehow also don't feel like updating the ScenarioContext for the warmup is necessarily the best as I see it more immovable than this and a bit confusing (as it's the context for the whole scenario, if it changes based on warmup adn run time there might be another ExecutionContext but that seems like overkill). I feel like passing the benchmarking function the time it should run for would still be viable but I'd need to play with that to see how many arguments it adds where. Would like to play with that after this is merged, so definitely can go ahead with the ScenarioContext being adjusted. Would still like to explore dropping the dependency on configuration :)

PragTob · 2017-07-15T11:23:16Z

lib/benchee/benchmark/scenario_context.ex

+  ]
+
+  @type t :: %__MODULE__{
+    config: map,


if we keep config, this could use the configuration type? :)

PragTob · 2017-07-15T11:33:02Z

lib/benchee/formatters/console.ex

-      jobs
-      |> Enum.flat_map(fn({_name, job}) -> Map.to_list(job) end)
+      scenarios
+      |> Enum.flat_map(&(Map.to_list(&1.run_time_statistics)))


Ok I might just go through here later and convert them back to fn or you convince me why & is great, always happy to learn :)

PragTob · 2017-07-15T11:36:40Z

test/benchee/benchmark/runner_test.exs

+  alias Benchee.Suite
+  alias Benchee.Benchmark
+  alias Benchee.Configuration
+  alias Benchee.Statistics


I think most places use the one line aliasing? :)

PragTob · 2017-07-15T11:39:10Z

test/benchee/benchmark/runner_test.exs

+    suite.scenarios
+    |> Enum.filter_map(filter_fun, map_fun)
+    |> List.flatten()
+  end


This is something we eventually wanna provide on Suite, one of the downsides of the new approach. I guess this might become more relevant when working on bringing the plugins up to date which I sort of dread :D

I'm happy to help with the other plugins, but maybe we would want to wait until we implement the memory usage info anyway? I anticipate the console formatter module to change significantly once that's implemented, and I'd imagine we'd need similar changes to the other plugins as well...

hm true/it's an idea. Imo new formatter versions don't need to support memory usage straight away and that could be released later. The general access to data still needs to change and I think the implementation of memory usages is more of just adding an extra section, it shouldn't interfere as much with the other/existing code imo

PragTob · 2017-07-15T11:41:06Z

test/benchee/benchmark/runner_test.exs

+    end
+
+    test "can run multiple benchmarks in parallel" do
+      suite = test_suite(%Suite{configuration: %{parallel: 6, time: 60_000}})


now that I see it here, 6 seems like overkill. Sticking to just 4 should work just as fine, no biggy and not related though :D

PragTob · 2017-07-15T11:53:26Z

test/benchee_test.exs

-            end,
-            fn(suite) -> IO.puts suite.configuration.assigns.custom end
-          ]
+          formatters: [formatter_one, formatter_two, formatter_three]


In this commit I'm updating some things according to feedback on the previous commits. I'm removing the &() functions, fixing a doctest that was incorrect (but still passed for some reason), and a couple other small things.

PragTob · 2017-07-21T09:09:49Z

ah yeah, FYI I think this is good I just wanted to take a calm moment on the weekend to give it one more read and then merge

PragTob

Thanks a ton! 👍

Ghost is happy how all of this came together

devonestes added 5 commits July 5, 2017 13:41

Add Benchee.Benchmark.Runner and tests

16ca4a7

At this point pretty much all the test and code for `Benchee.Benchmark.Runner` is good to go. Next up is re-doing the statistics to add that stuff to the scenarios instead of to a map on the suite.

Update statistics module and tests

94a86e7

This was an easy update! Now `Benchee.Statistics` is good and all tests are updated and passing for that module.

Update Benchee.Suite struct and update associated tests

84e75c6

I forgot we had those old keys in `%Benchee.Suite{}` that could be removed at this point, so I did that and updated the associated tests.

devonestes commented Jul 6, 2017

View reviewed changes

devonestes mentioned this pull request Jul 9, 2017

WIP - Begin refactoring of Benchmark module #93

Closed

Refactor console formatter module

260c172

Some of the function names didn't fit anymore, so I updated them, changed some typespecs, and used the shorthand function definition in a few places.

PragTob reviewed Jul 15, 2017

View reviewed changes

Incorporate feedback

2b706e5

In this commit I'm updating some things according to feedback on the previous commits. I'm removing the &() functions, fixing a doctest that was incorrect (but still passed for some reason), and a couple other small things.

PragTob approved these changes Jul 23, 2017

View reviewed changes

PragTob merged commit 3e5a84b into bencheeorg:master Jul 23, 2017

devonestes deleted the extract-runner branch July 23, 2017 18:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract runner and move to scenarios #96

Extract runner and move to scenarios #96

devonestes commented Jul 6, 2017

devonestes Jul 6, 2017

PragTob Jul 15, 2017

devonestes Jul 18, 2017

PragTob commented Jul 13, 2017

devonestes commented Jul 13, 2017

PragTob left a comment •

edited

PragTob Jul 15, 2017

PragTob Jul 15, 2017

PragTob Jul 15, 2017

PragTob Jul 15, 2017

PragTob Jul 15, 2017

PragTob Jul 15, 2017

PragTob Jul 15, 2017

PragTob Jul 15, 2017

devonestes Jul 18, 2017

PragTob Jul 18, 2017

PragTob Jul 15, 2017

PragTob Jul 15, 2017

PragTob commented Jul 21, 2017

PragTob left a comment

		@@ -0,0 +1,21 @@
		defmodule Benchee.Benchmark.ScenarioContext do
		defstruct [

Extract runner and move to scenarios #96

Extract runner and move to scenarios #96

Conversation

devonestes commented Jul 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PragTob commented Jul 13, 2017

devonestes commented Jul 13, 2017

PragTob left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PragTob commented Jul 21, 2017

PragTob left a comment

Choose a reason for hiding this comment

PragTob left a comment •

edited