bencheeorg / benchee

Easy and extensible benchmarking in Elixir providing you with lots of statistics!
MIT License
1.42k stars 66 forks source link

Adding `Benchee.CollectionData` struct #264

Closed devonestes closed 5 years ago

devonestes commented 5 years ago

There are two parts to this commit - adding a new Benchee.CollectionData struct, and renaming the previous measurers to collectors. Now when we have a new thing that we're benchmarking, we keep the raw samples and the calculated statistics together for that thing.

Resolves #259

devonestes commented 5 years ago

I keep going back and forth about where to put ‘CollectionData’ actually. My thinking is this:

In pattern matches, it’s best to pattern match on the “simplest” possible pattern to do what we need. Basically, if I’m seeing a pattern match on one struct type in one place, then I would expect to see a similar match on a different type right next to it.

Same with test data. I sort of like the idea of using the simplest test data that will work. If a bare map will work, then there’s no reason to over specify in the tests the data that our function accepts. It makes it (theoretically) easier to both change the implementation later on if the tests are as general as possible, and to refactor the existing implementation if the test isn’t as coupled to the current implementation.

The trade off there I guess is in explicitness and self-documentation.

I do think that in doctests I should probably specify the struct, since those tests serve a specific documentation purpose.

That said, those are just theoretical benefits/drawbacks and my personal taste. I don’t think it will make a big difference to the actual maintenance later on.

One thing I feel pretty strongly about is the “memory_usage_data” and “run_time_data” keys. Just “run_time” or “run_times” I think is much too general for the value for those keys. Otherwise you need to know the type of the data stored at that key to know what that “run_time” is referring to, and I think it should be the other way around - they key tells you (or at least gives you a hint about) what’s stored in that key.

So, I think I’m going to go back and add they struct type, but I also think we should keep those key names.

PragTob commented 5 years ago

:wave:

Didn't expect you back here yet :D

If you feel strongly about the key names, I'm happy to keep them! Was more a hunch with me :+1:

Regarding pattern matches, I agree - simplest thing that works seems good :+1:

As for test data, I used to think the same - aka just maps with the needed keys in tests and we're good. However, now I think we should use the right struct more because:

Cheerio!

29090720_339834176521801_6828362667602739200_n