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

Remove annoying error message #5956

Merged
merged 2 commits into from
May 4, 2024

Conversation

kwxm
Copy link
Contributor

@kwxm kwxm commented May 4, 2024

The cost-model-budgeting-bench executable runs benchmarks for the builtins but also runs some no-op benchmarks to give us an idea of the overhead involved in calling a builtin. To get accurate results for the no-op benchmarks we run them with a bigger time limit, which involves calling Criterion twice. If you asked it to benchmark some subset of the builtins (eg, by passing EqualsString on the command line) then there would be an error with the message "none of the specified names matches a benchmark" when it ran the no-op benchmarks (because none of them do match). This was annoying and also meant that you couldn't run a subset of the benchmarks on the benchmarking machine and get the results back. This PR removes the error mesage. Now it's less annoying, but if you ask it to benchmark something that doesn't exist (maybe because you mistype a name) then it'll complete silently and leave you with a CSV file containing only a header. This might be a bit mystifying, but it's better than a spurious error message. We could maybe do better if we were to bypass the Criterion command-line processing code altogether, but that might be quite a lot of work for a small gain.

I don't think this needs to be reviewed, so I'll set it to auto-merge.

@kwxm kwxm added the No Changelog Required Add this to skip the Changelog Check label May 4, 2024
@kwxm kwxm enabled auto-merge (squash) May 4, 2024 12:13
@kwxm kwxm merged commit 067e74f into master May 4, 2024
5 of 6 checks passed
@kwxm kwxm deleted the kwxm/budgeting/remove-annoying-error-message branch May 4, 2024 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No Changelog Required Add this to skip the Changelog Check
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant