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

Make --include and --exclude use prefix matching. #1279

Merged

Conversation

nnethercote
Copy link
Contributor

In #1138 I changed --include/--exclude to do exact matching of
benchmark names. This was a good idea at the time, but since then we
have renamed many benchmarks. There is now no benchmark with a name that
is a prefix of another benchmark's name. Also, we now have version
numbers in many benchmark names, which are a pain to remember and type.

This commit lets you specify a benchmark name by prefix. E.g.
--include=stm will match stm32f4-0.14.0.

In rust-lang#1138 I changed `--include`/`--exclude` to do exact matching of
benchmark names. This was a good idea at the time, but since then we
have renamed many benchmarks. There is now no benchmark with a name that
is a prefix of another benchmark's name. Also, we now have version
numbers in many benchmark names, which are a pain to remember and type.

This commit lets you specify a benchmark name by prefix. E.g.
`--include=stm` will match `stm32f4-0.14.0`.
@nnethercote nnethercote requested review from Kobzol and rylev and removed request for Kobzol and rylev April 6, 2022 04:47
Copy link
Contributor

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

I was quite fond of the new exact matching behaviour, but it's true that after TBBU (the big benchmark upgrade) it's quite annoying and prefixes are the way to go 👍

The PR and commit name seems to be wrong (it describes the previous change).

Copy link
Member

@rylev rylev left a comment

Choose a reason for hiding this comment

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

This makes a lot of sense. I am worried we'll end up hitting the old gotcha if we introduce something like serde_json as a benchmark...

@Kobzol
Copy link
Contributor

Kobzol commented Apr 6, 2022

The code could assert that the benchmarks don't share prefixes, this would be then caught in tests on CI if such benchmark would be introduced. But it would be a bit fragile (a prefix can also be a single char..) and it would not allow us to add serde-json, which is not good.

Maybe we could just print a warning/info log if a prefix matches more benchmarks? Although if the documentation/help explicitly says that it uses prefixes, that's kind of the expected behaviour.

@nnethercote
Copy link
Contributor Author

This makes a lot of sense. I am worried we'll end up hitting the old gotcha if we introduce something like serde_json as a benchmark...

Presumably that would be serde_json-1.0.79 or similar, so you could use serdeif you want to match multiple or serde-/serde_ if you just want to match one.

@rylev
Copy link
Member

rylev commented Apr 6, 2022

It's probably not that bad if someone accidentally matches more than benchmark so I'm fine with merging this. We can just keep an eye out if this causes issues in the future.

@rylev rylev merged commit f46d4db into rust-lang:master Apr 6, 2022
@nnethercote nnethercote changed the title Make --include and --exclude use exact matching. Make --include and --exclude use prefix matching. Apr 6, 2022
@nnethercote nnethercote deleted the include-exclude-prefix-matching branch April 6, 2022 20:43
@nnethercote
Copy link
Contributor Author

I was going to fix the commit message, but it's already been merged :)

@rylev
Copy link
Member

rylev commented Apr 6, 2022

Whoops! Sorry! 😀

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