search

bencheeorg / benchee

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

Promote use of keywords lists instead maps for job list #333

Open eksperimental opened 3 years ago

eksperimental commented 3 years ago

Hi @PragTob, first of all, thank you for such an amazing library, it helps me every now and then to make Elixir an even better language.

I would like to suggest to change from maps to keywords lists for the list of jobs. The reason is that since map is not an ordered structure, it may change the order of the execution depending on the name of the key. As of now, Benchee.run/2 works with either maps and keyword lists since it takes any enumerable. The specs for this function should also be updated.

The examples in the README file would also look like this.

Benchee.run(
  [
    "flat_map": fn -> Enum.flat_map(list, map_fun) end,
    "map.flatten": fn -> list |> Enum.map(map_fun) |> List.flatten() end
  ]
)

This is the way I set all my benchmarks. Thank you.

PragTob commented 2 years ago

:wave:

Hello, I believe I already gave you the "Hands, sorry" talk so I'm gonna skip it :grin:

Specs absolutely should be updated. Iirc we even have tests to make sure this works. As for making it the default.... I'm not sure.

Order of execution shouldn't really matter and the results are sorted by performance anyhow. I think we could probably highlight better that it's an option but I don't see a huge reason here to make it the default.

I like maps because they immediately convey that the keys/names shall not be duplicated :)

eksperimental commented 2 years ago

Hi. I can't remember the use case because this was almost a year ago, but order made a difference. I think it was it was that I wanted to see some results before the whole suite was tested. Made some changes, see if it worked and if not stop it straight away. But with maps the order is not guaranteed, so I had to move to keyword lists in order to achieve this.

PragTob commented 2 years ago

Ah I see, the pre_check feature that runs every job once wouldn't help? :thinking: