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

Add a feature for using the same number of loops as a previous run #327

Merged
merged 9 commits into from
Feb 2, 2024

Conversation

mdboom
Copy link
Contributor

@mdboom mdboom commented Jan 30, 2024

Motivation:

On the Faster CPython team, we often collect pystats (counters of various interpreter events) by running the benchmark suite. It is very useful to compare the stats between two commits to see how a pull request affects the interpreter. Unfortunately, with pyperformance's default behavior where the number of loops is automatically calibrated, each benchmark may not be run the same number of times from run-to-run, making the data hard to compare.

This change adds a new argument to the run command which will use the same number of loops as a previous run. The loops for each benchmark is looked up from the metadata in the .json output of that previous run, and passed to the underlying call to pyperf using the --loops argument.

Additionally, this modifies one of the benchmarks, sqlglot to be compatible with that scheme. sqlglot is the only run_benchmark.py script that runs multiple benchmarks within it in a single call to the script. This makes it impossible to set the number of loops independently for each of these benchmarks. It's been updated to use the pattern from other "suites" of benchmarks (e.g. async_tree) where each benchmark has its own .toml file and is run independently. This should still be backward compatible with older data collected from this benchmark, but doing pyperformance run -b sqlglot will now only run a single benchmark.

Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

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

Looks good in general. I'll let Eric approve though.

pyperformance/run.py Show resolved Hide resolved
Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

LGTM

I left a couple of minor comments that you can take or leave.

It might be worth splitting out the bm_sqlglot changes into a separate PR, but it isn't a big deal. I'll leave that up to you.

pyperformance/run.py Show resolved Hide resolved
pyperformance/run.py Outdated Show resolved Hide resolved
@mdboom
Copy link
Contributor Author

mdboom commented Feb 1, 2024

@ericsnowcurrently: I think this is good to merge, but I don't have merge rights, if you don't mind... :)

@ericsnowcurrently ericsnowcurrently merged commit 79f80a4 into python:main Feb 2, 2024
11 checks passed
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.

3 participants