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

Give bench_local the option to run stable benchmarks #1778

Closed
wants to merge 1 commit into from

Conversation

Nadrieril
Copy link
Member

In #1777 I noticed that I couldn't run category: stable benchmarks locally; this PR makes this possible.

We can now specify primary/secondary/stable in --include and --exclude. If there's no --include/--exclude it defaults to --exclude stable which preserves today's behavior. This does change behavior for anyone running bench_local --exclude ...: they would need to add --exclude ...,stable to bench the same set as today.

@Nadrieril Nadrieril force-pushed the bench-local-categories branch from eabc24c to aedb577 Compare December 19, 2023 12:46
@Kobzol
Copy link
Contributor

Kobzol commented Dec 19, 2023

I think that unless @nnethercote objects, we could just allow stable benchmarks by default, by removing the line that filters them out.

But for what it's worth, I don't think that it is intuitive to deal with categories in --include/--exclude, since these flags are about benchmark names, not categories. I would rather add a new flag, --category/--categories, which would have primary,secondary as the default value.

@Nadrieril
Copy link
Member Author

Hm, a different flag does not make sense to me, I could want --include primary,encoding

@Kobzol
Copy link
Contributor

Kobzol commented Dec 19, 2023

--include filters benchmarks by name, not category, so --include primary would have to mean "include the whole primary category". But that would also mean that there cannot be any overlap between benchmark names and categories, and is, well, a horrible hack :)

@Nadrieril
Copy link
Member Author

Yeah that's how I thought about it. Doesn't feel more a hack than lint groups and lints using an identical namespace like #[allow(unused)]/#[allow(dead_code)] 🤷‍♀️ I'm fine with anything that lets me run stable benches tbh

@nnethercote
Copy link
Contributor

Some thoughts:

  • I like that stable benchmarks aren't run by default.
    • The style-servo benchmark was particularly large, and when it was part of the normal suite I almost always excluded it when running locally.
    • There is some overlap between the stable benchmarks and the primary, e.g. two different versions of regex, html5ever, piston-image, syn.
    • I think the stable benchmarks are only really useful for the long-term performance picture, and I would almost never run them locally myself.
  • Having said that, I agree it's reasonable to be want to run them locally.
  • My initial feeling is that a separate flag --category would make more sense than this PR's approach. It would default to Primary,Secondary. --include/--exclude would then be applied within the chosen categories.

Hm, a different flag does not make sense to me, I could want --include primary,encoding

Would that run all the primary benchmarks + encoding from the stable benchmarks? Hmm, is that likely to happen in practice?

We also have a problem that some of the stable benchmark names are prefixes of some of the primary benchmark names, e.g. the ones I mentioned above, so it would be impossible to run the primary benchmark syn-1.0.89 without also running the stable benchmark syn.

Also, if it's done with just --include/--exclude how are conflicting category/individual specifications combined? E.g. --exclude stable --include syn?

@Kobzol
Copy link
Contributor

Kobzol commented Dec 20, 2023

I agree with @nnethercote, and I would suggest #1780 as an alternative solution to your problem.

@Nadrieril
Copy link
Member Author

Hmm, is that likely to happen in practice?

Probably not tbf. I always use an explicit list myself.

#1780 works for me!

@Nadrieril Nadrieril closed this Dec 20, 2023
@Nadrieril Nadrieril deleted the bench-local-categories branch December 20, 2023 22:14
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