Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ecto benchmarks #2607

Merged
merged 12 commits into from
Sep 27, 2018
Merged

Ecto benchmarks #2607

merged 12 commits into from
Sep 27, 2018

Conversation

tallysmartins
Copy link
Contributor

The initial benchmark suite for Ecto. Tests rely on the Benchee library.

michalmuskala and others added 7 commits June 25, 2018 19:39
- Adds initial setup
- Adds ElixirBench config.yml
- Adds benchmarks for Repo.insert!/1
- Adds benchmarks for Repo.all/1, Repo.load/2 and Repo.to_sql/2

Signed-off-by: Tallys Martins <tallysmartins@gmail.com>
Signed-off-by: Tallys Martins <tallysmartins@gmail.com>
Signed-off-by: Tallys Martins <tallysmartins@gmail.com>
- Adds bigger queries to to_sql_bench
- Adds more attribute types, such as datetimes, naivedate, time to load_bench

Signed-off-by: Tallys Martins <tallysmartins@gmail.com>
Signed-off-by: Tallys Martins <tallysmartins@gmail.com>
Signed-off-by: Tallys Martins <tallysmartins@gmail.com>
bench/README.md Outdated
To run the benchmarks tests just type in the console:

```
$ export BENCHMARKS_OUTPUT_PATH=bench/results && mix run bench/bench_helper.exs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use BENCHMARKS_OUTPUT_PATH=bench/results mix run bench/bench_helper.exs in POSIX-compatible shells and env BENCHMARKS_OUTPUT_PATH=bench/results mix run bench/bench_helper.exs in rest of them.

Signed-off-by: Tallys Martins <tallysmartins@gmail.com>
bench/README.md Outdated

```
# other shells
$ env BENCHMARKS_OUTPUT_PATH=bench/results && mix run bench/bench_helper.exs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In both cases without the && part.

Signed-off-by: Tallys Martins <tallysmartins@gmail.com>
@tallysmartins
Copy link
Contributor Author

Here are some of the results that I've gotten from running in my laptop.

Repo.insert!/1
Repo.all/2
Repo.to_sql/2
Repo.load/2

@PragTob
Copy link
Contributor

PragTob commented Jul 20, 2018

Any way we can help move this PR along @hauleth ? 😁

Signed-off-by: Tallys Martins <tallysmartins@gmail.com>

# Clean inserted data
Ecto.Bench.PgRepo.delete_all(User)
Ecto.Bench.MySQLRepo.delete_all(User)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we move the cleanup code to the beginning of the benchmark instead of the end? That way if something goes wrong during the benchmark the next run should be fine.

Copy link
Contributor Author

@tallysmartins tallysmartins Aug 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edited: I don't think we need to do a pre cleanup because in the setup we call storage_down and this already clean the db.. doesn't it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right

Copy link
Member

@michalmuskala michalmuskala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the small comment about setup & cleanup I think this is good to go

Signed-off-by: Tallys Martins <tallysmartins@gmail.com>
"MySQL Repo.all/2" => fn -> Ecto.Bench.MySQLRepo.all(User, limit: limit) end
}

path = System.get_env("BENCHMARKS_OUTPUT_PATH") || raise "I DON'T KNOW WHERE TO WRITE!!!"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about using bench/results as default value?

And maybe move it to Application env?

Application.put_env(
  :ecto, 
  :bench_results_path, 
  System.get_env("BENCHMARKS_OUTPUT_PATH") || "bench/results"
)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it is concerned to ElixirBench to set this variable in order to collect and clean the results, so raising here means that something went wrong in our runners. In the near future we want to wrap this kind of settings, so I don't think it will make big difference moving this now.

bench/README.md Outdated
$ env BENCHMARKS_OUTPUT_PATH=bench/results MIX_ENV=bench mix run bench/bench_helper.exs
```

Benchmarks are inside the benchmarks/ directory and are divided into two
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nitpicks:

  1. I wouldn't use backticks for Potgres and MySQL:
and need PostgreSQL and MySQL up and running.

Also, I would use "PostgreSQL" instead of "Postgres

  1. I would use backticks for the dir:
Benchmarks are inside the `benchmarks/` directory
  1. And maybe use bullet points for the list of categories?
* `micro benchmarks`: Operations that don't actually interface with the database,
but might need it up and running to start the Ecto agents and processes.
* `macro benchmarks`: Operations that are actually run in the database. This are
more likely to integration tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your considerations! ❤️

Signed-off-by: Tallys Martins <tallysmartins@gmail.com>
@josevalim josevalim merged commit 60d87a3 into elixir-ecto:master Sep 27, 2018
@josevalim
Copy link
Member

❤️ 💚 💙 💛 💜

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants