-
Notifications
You must be signed in to change notification settings - Fork 823
Compiled code benchmarks: easy benchmarking of preview
against current
#16942
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
Conversation
* The compiled code benchmarks now default to using the selected benchmarks built with the current langversion as the baseline; the same benchmarks built with langversion preview are then automatically benchmarked against those for easy comparison.
✅ No release notes required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The infrastructure to compare new/old compiler is the right thing to have here, this is really making the benchmarks folder useful by showing how to benchmark compiler perf improvements.
I do not think we shall include all of the individual comparison instances in the repo forever - they are a useful measurement for your feature PR to demonstrate it brings improvements. But for keeping them in the repo, I think it would be better to just keep a few instances demonstrating the setup.
Also to make it clear that the purpose of the folder is to demonstrate the different setup mechanisms like local vs shipped compiler and the same for FSharp.Core, not to persist every benchmark executed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff Brian. Looking forward to try this in action, I will make a lot of use of it :)
As for the new benchmarks - I am a bit on the fence here. T-Gro's arguments make sense to me, in particular I think there are 2 reasons to have some specific benchmarks here:
- It is a good demonstration of a benchmarking technique not yet used here
- We plan to use them in the near future, as in, we continue working on some feature perf
I have recently started including F# micro perf to the dotnet/performance repo. We'll see how that plays out - in theory, this is the right place for keeping track of such things, and this is what C# does also. But it will take us some time to tune things out there.
Maybe you can keep one of the new benchmarks. Or all of them if you insist and nobody minds. Either way, it would be awesome if you add the README here, which can be a concatenation of the PR description and these comments, you can also take inspiration from other READMEs in the benchmarks folder.
Thank you!
tests/benchmarks/CompiledCodeBenchmarks/MicroPerf/Benchmarks.fs
Outdated
Show resolved
Hide resolved
tests/benchmarks/CompiledCodeBenchmarks/MicroPerf/Benchmarks.fs
Outdated
Show resolved
Hide resolved
Hmm. (Forgive me if I am gravely misunderstanding something; I don't know enough about the intent or ownership of dotnet/performance to know what role, if any, it should play here.) Out of curiosity, I browsed through some Roslyn PRs that introduced codegen changes to see if they had any useful practices to borrow... But they mostly seemed to just post benchmark source and results in comments, and the benchmarks themselves seemed to be independent benchmarks of what the new codegen was ostensibly doing. At a glance, I didn't see any benchmarks that actually used an older compiler's output as a baseline and benchmarked the new output against that.
Yeah, although, on the surface, the dotnet/performance repo currently seems geared much more towards testing (1) the performance of the runtime itself and (2) the performance of specific first-party libraries, than, say, Roslyn codegen. There are mentions of benching two different runtime versions against each other —
— which could perhaps be adapted to work with different FCS (or language, etc.) versions. If it makes sense to start using dotnet/performance as a way to test FCS codegen changes (and perhaps library changes for FSharp.Core, similar to how many System.* and Microsoft.* libraries are tested there), then maybe we can just come up with and document two new workflows there, each for both codegen and library (FSharp.Core) changes:
Or we could keep them separate — use dotnet/performance for long-lived benchmarks, and have some kind of documentation and maybe template(s) here, in this repo, for writing and running short-lived, disposable benchmarks during local development.
On the one hand, I can understand wanting to avoid accumulating a mess of outdated benchmarks in source: either they will never be run again, in which case they are (worse than) useless, or else they must be run at some frequency, which has development time, deployment time, and infrastructure costs. On the other hand, it seems to me that if a performance improvement is worth making and benchmarking in the first place, it might be worth keeping the benchmarks around to detect unintended regressions, at least for sufficiently significant or fundamental things. That is again only useful if the benchmarks are actually run at some interval after changes are made, but maybe that's what dotnet/performance is for? |
@brianrourkeboll, so first of all - no gravely misunderstanding here, we are trying to understand our options ourselves :) So this
... is roughly my current vision (and maybe the team's vision but not sure :D). Because this
is supposed to be the case. However, this is just a vision, I've just started adding some F# code there and currently watching the situation. Hence I don't mind having more benchmarks in our repo until we are convinced about dotnet/performance. |
All right. I went ahead and added a simple readme and left only one set of new benchmarks. Let me know if you'd rather I pared that down even further. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome. LGTM. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still do not like the amount of code being checked in, especially considering it is mostly duplicate.
But I will not block it - let's check it in.
I think a benchmark could be made generic (for the various integral types here), further reducing it down. Will just need to figure out how to turn the generic argument into a category-like column.
@T-Gro The duplication is necessary because, if the range type for I know because I tried to use a single generic benchmark class and use The benchmarks for testing the compile-time computation of constant counts also must be inlined and thus duplicated per type, i.e., at least the generic If you don't think it's useful to have these benchmarks checked in here at all, I'm fine with removing them altogether. Maybe they would be a better fit for dotnet/performance at some point? |
* PrettyNaming: make DoesIdentifierNeedBackticks public (#16613) * Update dependencies from https://github.com/dotnet/arcade build (#16944) Microsoft.DotNet.Arcade.Sdk From Version 8.0.0-beta.24170.6 -> To Version 8.0.0-beta.24172.5 Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> * Unskip a few tests (#16946) * Unskip a few tests * another one * Fix [<tailcall>] false positive with yield! (#16933) * add test case showing a false positive with yield! * add missing case in IsAppInLambdaBody that can happen with yield! * add release notes entry * update PR number * only bind to what we are interested in * add test expecting a warning for yield! in a list comprehension * add test case using yield! in a custom CE that overflows the stack * Add regression test for nameof type with generic parameters (#16827) Co-authored-by: Vlad Zarytovskii <vzaritovsky@hotmail.com> * Minor README update about proto (#16945) * Minor README update about proto It's Bootstrap actually. * Update DEVGUIDE.md Co-authored-by: Vlad Zarytovskii <vzaritovsky@hotmail.com> * Update DEVGUIDE.md --------- Co-authored-by: Vlad Zarytovskii <vzaritovsky@hotmail.com> * Compiled code benchmarks: easy benchmarking of `preview` against current (#16942) * Make it easy to bench preview against current * Revert EnableDefaultNoneItems false (#16953) * Exclude compiler service benchmark from VMR build when not building tests (#16955) * Exclude compiler service benchmark from VMR build when not building tests * remove Z --------- Co-authored-by: Petr <psfinaki@users.noreply.github.com> * Update dependencies from https://github.com/dotnet/arcade build 20240326.8 (#16957) Microsoft.DotNet.Arcade.Sdk From Version 8.0.0-beta.24172.5 -> To Version 8.0.0-beta.24176.8 Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> --------- Co-authored-by: Eugene Auduchinok <eugene.auduchinok@jetbrains.com> Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Petr <psfinaki@users.noreply.github.com> Co-authored-by: dawe <dawedawe@posteo.de> Co-authored-by: Florian Verdonck <florian.verdonck@outlook.com> Co-authored-by: Vlad Zarytovskii <vzaritovsky@hotmail.com> Co-authored-by: Brian Rourke Boll <brianrourkeboll@users.noreply.github.com> Co-authored-by: Kevin Ransom (msft) <codecutter@hotmail.com> Co-authored-by: Matt Mitchell <mmitche@microsoft.com>
Description
preview
against current.The compiled code benchmarks now default to using the selected benchmarks built with the current langversion as the baseline; the same benchmarks built with langversion
preview
are then automatically benchmarked against those for easy comparison.Ideally, it should be possible to run benchmark comparisons for compiled code built using any combination of:
preview
.Of those, the combinations most likely to be useful are probably:
preview
. This is what this PR adds.preview
. This is definitely achievable with BenchmarkDotNet, but I tried, and it won't work without updates to the current cascade of build configurations from all theDirectory.Build.props
, etc., at various levels, which I didn't want to undertake.start..finish
,start..step..finish
#16650 and Optimize simple range mappings:[for n in start..finish -> f n]
, &c. #16832.Benchmark switcher
Example output
Checklist