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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Store & load previous runs #161

Merged
merged 22 commits into from Jan 4, 2018

Conversation

Projects
None yet
2 participants
@PragTob
Owner

PragTob commented Nov 19, 2017

Introduces the save option to store previous benchmarking runs in a file. Also introduces the load option to load them into the set of current scenarios.

Basic idea is to attach saved scenarios with a tag to identify them by, defaults to the time the benchmark started but can be any string - i.e. a branch name.

Can then be loaded again and the scenarios should just be displayed, not rerun.

This is in huge parts made possible so easily through our scenarios refactoring 馃帀
The beauty of this is also that it just works with all formatters 馃挴 (caveat, displaying the tag as part of the name but that's minimal)

I just noticed we don't even have an issue for this :trollface: However, it has been requested a couple of times and it's a distinguishing factor that other tools have which we don't have (yet).

Plus, I wanna use it myself to take a better look at #160

Missing:

  • documentation
  • sample
  • do not run statistics? (values should be there, could run into trouble though if we introduce new statistics)
  • Senario display name function + use it in the console formatter to show off the tag
  • only write out new/untagged scenarios (can go in a separate ticket)
  • fix bug about adding new scenarios (just the name is checked, tag needs to be checked as well)
  • potentially more load options
  • don't count loaded scenarios for runtime estimation purposes

Possible follow ups:

  • option to also store all loaded scenarios (not sure what the most expected behaviour is)
  • if we implement not recalculating statistics (which i think we should), we should implement an option to force statistics recalculation (solved by loading later and if people want it they can easily just load them earlier manually)
@PragTob

This comment has been minimized.

Owner

PragTob commented Nov 26, 2017

Noticed that as the html formatter works extensively with maps that name (job_name + tag) also should be unique, otherwise it doesn't work well. Just noticed that while doing the gc_experiment branch. E.g. took 2 samples for each case (normal, avoid gc, avoid gc + garbage collect all the time) but the formatter doesn't work that well anymore then.

Options:

  • Make sure/tell people not to duplicate tags
  • automatically add -1/-2/-3 etc. to a tag if it gets duplicated during saving (or the date)
  • save more meta data like the recorded time to de-duplicate scenarios
@PragTob

This comment has been minimized.

Owner

PragTob commented Dec 3, 2017

Given time will move the loading of the saved scenarios further down the line (before formatters) - resolves a ton of "should or shouldn't we do this now" scenarios.

Separate the Loader from the Config
* we can load at the latest possible and hence don't have to care
  about how already loaded scenarios behave for measure, statitics
  etc.

PICK ME UP: Now we can get rid off teste elsewhere but probably
should ad somme more for our newly introduced module :)
@PragTob

This comment has been minimized.

Owner

PragTob commented Dec 10, 2017

Did the above, this also saves us from more load options because if people want to optionally recalculate statitics they can switch to the more verbose API and just insert the loading step wherever they want to (right before statistics or even before measure to just take the name and function and run the benchmarks again)

@PragTob

This comment has been minimized.

Owner

PragTob commented Dec 10, 2017

Save the documentation this should be about done and could do with a review :)

img_20171201_190418 1

please

@PragTob

This comment has been minimized.

Owner

PragTob commented Dec 10, 2017

Ok, added some docs remembered two things:

  • do something like tag-2 if tags get duplicated
  • think about taking file interleaving out (so that the the tag doesn't automatically get added to the path)
@devonestes

In general this looks great! I just have one question/recommendation about the actual API the users will use that I think would be really helpful to have, but other than that I think this is at least ready to get into master so folks can start playing with it!

describe ".output/1" do
test "able to restore fully from file" do
try do

This comment has been minimized.

@devonestes

devonestes Dec 11, 2017

Collaborator

Not a big deal, but technically we don't need try here. There is an implicit try that will allow us to use the after when we use the test macro.

This comment has been minimized.

@PragTob

PragTob Dec 11, 2017

Owner

already started using that in a place as I saw you talk about it in another PR - didn't really know before, will do now :)

scenarios of the current run with a specified tag - can be used for storing
and loading the results of previous runs.
Automatically invoked and appended when specifiying the `save` option in the

This comment has been minimized.

@devonestes

devonestes Dec 11, 2017

Collaborator

Great documentation here! 馃憤

scenarios = [%Scenario{tag: "old"}]
suite = %Suite{scenarios: scenarios}
try do

This comment has been minimized.

@devonestes

devonestes Dec 11, 2017

Collaborator

Here as well we technically don't need try, but I'm cool with leaving it in if you want.

iex> Scenario.display_name(%Scenario{job_name: "flat_map"})
"flat_map"
iex> Scenario.display_name(%Scenario{job_name: "flat_map", tag: "master"})
"flat_map (master)"

This comment has been minimized.

@devonestes

devonestes Dec 11, 2017

Collaborator

This is a really nice UI - I like it! 馃憤

"map.flatten" => fn -> list |> Enum.map(map_fun) |> List.flatten end
},
time: 5,
load: "save_first-try.benchee"

This comment has been minimized.

@devonestes

devonestes Dec 11, 2017

Collaborator

I think it would be really helpful if users were able to pass the same keyword list for load as they do for save, don't you? That seems (to me) like a reasonable thing a user might want to do.

This comment has been minimized.

@PragTob

PragTob Dec 11, 2017

Owner

hm what do you mean? Like :path ? I planned on doing that but in the end there was no other option I wanted to pass to load so I thought it might be nicer. You're right the other option might be more consistent.

Or do you also mean passing tag again? I can see how it might be useful to "retag" all loaded scenarios but thought I'd rather avoid it. Not sure.

This comment has been minimized.

@devonestes

devonestes Dec 12, 2017

Collaborator

I think it would be helpful for users if they could do this:

list = Enum.to_list(1..10_000)
map_fun = fn(i) -> [i, i * i] end

Benchee.run(%{
  "flat_map"    => fn -> Enum.flat_map(list, map_fun) end,
  "map.flatten" => fn -> list |> Enum.map(map_fun) |> List.flatten end
},
  time: 5,
  save: [file: "save.benchee", tag: "first-try"]
)

Benchee.run(%{
  "flat_map"    => fn -> Enum.flat_map(list, map_fun) end,
  "map.flatten" => fn -> list |> Enum.map(map_fun) |> List.flatten end
},
  time: 5,
  load: [file: "save.benchee", tag: "first-try"]
)

Otherwise, we're sort of asking them to either 1) Do the conversion of what the file would be named in their head, or 2) Check their filesystem to find the right file they want to load. Do you think that might be confusing?

This comment has been minimized.

@PragTob

PragTob Dec 12, 2017

Owner

oh, I wanted to remove the whole "interleave the tag into the filename" feature as a whole, as I figured it'd be confusing. So no matter the tag, if you say file/path: "save.benchee" it'll land in save.benchee and NOT save_tag.benchee - what do you think?

This comment has been minimized.

@devonestes

devonestes Dec 13, 2017

Collaborator

Yeah, that'll work fine. I think as long as whatever we pass to save can also be passed to load, then that should work out. I would assume that for most users they'd be saving and then loading right away, so making that change easy for them would be helpful. They the only thing they need to do for that functionality is to replace save: with load: and they're good to go.

This comment has been minimized.

@PragTob

PragTob Dec 13, 2017

Owner

Hm I would still cut out the tag part as we don't need it, but I think I won't validate it so it won't do harm. Will implement it some time!

@PragTob

This comment has been minimized.

Owner

PragTob commented Dec 11, 2017

Fixed the try thing, not super sure what you mean with the API question - somehow it's also "greyed" out but we can discuss it inline, her or on Slack :)

edit: thanks for the review 馃帀

@PragTob

This comment has been minimized.

Owner

PragTob commented Jan 3, 2018

Okay got around to removing the weird interleaved file saving finally :)

@devonestes

This comment has been minimized.

Collaborator

devonestes commented Jan 4, 2018

Do you still want to add the tag-2 thing if tags get duplicated, or do you think this is ready to roll as is? I think it's great and would have no problem merging this as is! If we want to add the tag-2 thing for duplicated tags, we can always do that later

@PragTob

This comment has been minimized.

Owner

PragTob commented Jan 4, 2018

@devonestes would like to do it ater, I think only thing missing right now is README docs but we can already finally merge this and then do tag-x and README in another go :)

Thanks!

@PragTob PragTob merged commit 60d4726 into master Jan 4, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 94.824%
Details

@PragTob PragTob deleted the store-previous-runs branch Jan 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment