First version of separating memory and runtime measurements #204

PragTob · 2018-04-02T16:26:47Z

Introduces a new Measurer behaviour that can be adopted by Time
and Memory and in the future Reduction. Also switches over to
memory having its own configuration for time. Like how this ended up
in general.

This isn't merge ready yet, it's a first implementation already
up for review and CI.

Fixes #202

Ah yeah, most important proof - it actually fixes the problem - memory measurements don't seem to be impacting runtime measurements anymore:

tobi@speedy ~/github/benchee $ mix run samples/run.exs 
Name                  ips        average  deviation         median         99th %
flat_map           2.16 K      463.52 μs    ±10.62%         452 μs         762 μs
map.flatten        1.16 K      863.87 μs    ±16.60%         822 μs     1301.96 μs

Comparison: 
flat_map           2.16 K
map.flatten        1.16 K - 1.86x slower
# include memory measurements in the run.exs
tobi@speedy ~/github/benchee $ mix run samples/run.exs 
Name                  ips        average  deviation         median         99th %
flat_map           2.30 K      435.69 μs     ±9.88%         429 μs         720 μs
map.flatten        1.19 K      838.40 μs    ±16.38%         803 μs        1223 μs

Comparison: 
flat_map           2.30 K
map.flatten        1.19 K - 1.92x slower

Memory usage statistics:

Name           Memory usage
flat_map          625.54 KB
map.flatten       781.85 KB - 1.25x memory usage

**All measurements for memory usage were the same**

TODO:

README/documentation adjustments
respect memory_time during calculation of estimated total runtime
allow measurer to return nil when a a measurement "failed" and handle it appropriately (memory measurement is less than 0 case)

This also sort of sets up putting do_benchmark/3 into its own module as it is responsible for running the benchmarking with a given measurer. The rest of the module deals with setting up the measurements according to our different measurer.

Introduces a new `Measurer` behaviour that can be adopted by `Time` and `Memory` and in the future `Reduction`. Also switches over to memory having its own configuration for time. Like how this ended up in general. This isn't merge ready yet, it's a first implementation already up for review and CI. TODO: - [ ] README/documentation adjustments - [ ] respect `memory_time` during calculation of estimated total runtime - [ ] allow measurer to return `nil` when a a measurement "failed" and handle it appropriately (memory measurement is less than 0 case)

PragTob · 2018-04-02T16:30:13Z

test/benchee/benchmark/runner_test.exs

@@ -146,7 +146,7 @@ defmodule Benchee.Benchmark.RunnerTest do
      assert [memory_consumption] = Enum.uniq(memory_usages)
      assert memory_consumption >= 1
      # depending on the number iterations determined, there can be spikes/changes
-      assert memory_consumption <= 100
+      assert memory_consumption <= 1_000


woopsie I added a 0 here! ;)

Before this was run in the repeated code with a num_iterations of 100 or 10. It seems that we have some small basic overhead that now shows up again more. We can have a look at removing this some time later.

Just wrote a small script and memory usage for an empty function without anything seems to be 616 Byte on my machine.

wasnotrice

This turned out nicely! What do you think about changing the behavior name to Measure? I think it would improve readability. Like when you pass Measure.Time as an argument. And it is easier to type/pronounce! :)

wasnotrice · 2018-04-02T21:22:46Z

lib/benchee/benchmark/runner.ex

  alias Benchee.Benchmark.{Scenario, ScenarioContext}
  alias Benchee.Utility.{RepeatN, Parallel}
  alias Benchee.Configuration
+  alias Benchee.Benchmark.Measurer


Maybe place this alias with the others from Benchmark

ah damn me, totally - thanks Eric!

PragTob · 2018-04-03T08:08:55Z

Good idea on the naming too, thanks! I think that was my OOP mindset leaking through as in the thing that measures but just Measure is nicer I think - thanks!

devonestes

This is a great start!

My only concern at the moment is with naming. In our configuration, the keys time and memory_time don't make it clear that they're exclusive of one another. I could see people thinking that memory_time is how much of the time we want to devote to measuring memory.

That said, this might be able to be cleared up just with documentation. It would be awesome if we could come up with something that didn't require documentation to make that clear, but nothing is coming to me yet this morning. I'll keep thinking about it, though 😄

PragTob · 2018-04-04T18:02:57Z

Yeah nothing comes to my mind as well. That was one of my concerns with these... we could rename time to run_time but I don't think that really helps or what do you think? (it'd be breaking though, could be a fall back but I dunno I still see it as the main use case)

devonestes · 2018-04-05T07:37:49Z

Since we're still pre-1.0, now would be the time to make those breaking changes if we feel they're the best way to go 😉

However, run_time is similarly confusing, so I don't think that's a great change to go with. What about memory_duration, run_time_duration, and (later on) reductions_duration? That's the best synonym for time I've been able to come up with, but I'm not really in love with it.

PragTob · 2018-04-05T08:06:07Z

but what good would chaning it from time to duration be?

I think it's better if the times always just add to each other - because if they sort of divide the time amongst each other all sorts of edge cases pop up... :(

PragTob · 2018-04-07T10:43:01Z

Ok this should be done now from me... but one of the builds failed (edit: ok was just a hex hiccup) I'll see real quick what that is about hopefully.

Reviews appreciated 🙏

🐰 🐰 🐰

* Fixes #206 * Also make sure that we're not printing that we are "benchmarking"

PragTob · 2018-04-08T10:44:42Z

Includes a fix for #206 now :)

edit: as it includes another fix now but they are all relatively separate, reviewing commits separately might help :)

devonestes

This looks great! Just one little documentation thing that I think might be helpful, and then it's ready to go.

devonestes · 2018-04-10T10:31:09Z

lib/benchee/output/benchmark_printer.ex

-    IO.puts "Available memory: #{available_memory}"
-    IO.puts "Elixir #{elixir_version}"
-    IO.puts "Erlang #{erlang_version}"
+    IO.puts """


Thank you 👍

devonestes · 2018-04-10T10:50:18Z

README.md

@@ -143,6 +145,7 @@ The available options are the following (also documented in [hexdocs](https://he

 * `warmup` - the time in seconds for which a benchmarking job should be run without measuring times before real measurements start. This simulates a _"warm"_ running system. Defaults to 2.
 * `time` - the time in seconds for how long each individual benchmarking job should be run and measured. Defaults to 5.


Maybe this should be updated to make it super clear that this is only to measure the runtime performance?

ah definitely, good catch - thank you! I'll do that tomorrow :)

devonestes · 2018-04-10T10:51:03Z

README.md

 ```

+Memory time can be specified separately as it will often be constant - so it might not need as much measuring time.


PragTob · 2018-04-11T18:09:16Z

Assuming that this is good now --> (devon's comments where documentation related I think I fixed those and if not we can always adjust again)

PragTob added 2 commits April 2, 2018 18:15

temporarily comment out function until I need it again (see TODO)

b95b198

PragTob commented Apr 2, 2018

View reviewed changes

Drop bad memory measurements and fix test

eec8a42

wasnotrice reviewed Apr 2, 2018

View reviewed changes

devonestes reviewed Apr 4, 2018

View reviewed changes

Feedback from Eric! :)

fc2974d

PragTob added 3 commits April 7, 2018 12:36

take memory time into account during calculation of estimated times etc.

c3b82c7

mix format on the benchmark printer (not too many changes)

8de1e02

et le documentation

7fb5593

This was referenced Apr 7, 2018

Memory measurements - weird behaviour #205

Closed

Allow running only memory measurements #206

Closed

Measure runtime with best available precision #207

Closed

Make sure we can opt out of running any type (time, memory etc.)

8ae652b

* Fixes #206 * Also make sure that we're not printing that we are "benchmarking"

devonestes reviewed Apr 10, 2018

View reviewed changes

Improve docs to make clear run time vs. memory time

fb0aa72

PragTob merged commit 2bdb361 into master Apr 11, 2018

PragTob deleted the separate-memory-and-runtime-measurement branch April 11, 2018 18:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First version of separating memory and runtime measurements #204

First version of separating memory and runtime measurements #204

PragTob commented Apr 2, 2018 •

edited

Loading

PragTob Apr 2, 2018

PragTob Apr 2, 2018

wasnotrice left a comment

wasnotrice Apr 2, 2018

PragTob Apr 3, 2018

PragTob commented Apr 3, 2018

devonestes left a comment

PragTob commented Apr 4, 2018

devonestes commented Apr 5, 2018

PragTob commented Apr 5, 2018

PragTob commented Apr 7, 2018 •

edited

Loading

PragTob commented Apr 8, 2018 •

edited

Loading

devonestes left a comment

devonestes Apr 10, 2018

devonestes Apr 10, 2018

PragTob Apr 10, 2018

devonestes Apr 10, 2018

PragTob commented Apr 11, 2018

		@@ -143,6 +145,7 @@ The available options are the following (also documented in [hexdocs](https://he

		* `warmup` - the time in seconds for which a benchmarking job should be run without measuring times before real measurements start. This simulates a _"warm"_ running system. Defaults to 2.
		* `time` - the time in seconds for how long each individual benchmarking job should be run and measured. Defaults to 5.

		```

		Memory time can be specified separately as it will often be constant - so it might not need as much measuring time.

First version of separating memory and runtime measurements #204

First version of separating memory and runtime measurements #204

Conversation

PragTob commented Apr 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wasnotrice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PragTob commented Apr 3, 2018

devonestes left a comment

Choose a reason for hiding this comment

PragTob commented Apr 4, 2018

devonestes commented Apr 5, 2018

PragTob commented Apr 5, 2018

PragTob commented Apr 7, 2018 • edited Loading

PragTob commented Apr 8, 2018 • edited Loading

devonestes left a 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 Apr 11, 2018

PragTob commented Apr 2, 2018 •

edited

Loading

PragTob commented Apr 7, 2018 •

edited

Loading

PragTob commented Apr 8, 2018 •

edited

Loading