Add PGO for F# compiler
This should eventually make the FSC about 30% faster.
The latest FCS package contains these MIBC files:
In the SDK part of it, we need to unpack them, apply to the compiler, and repack.
This will also require adding mibc dependencies via darc, so they are auto-updated.
This will also require adding mibc dependencies via darc, so they are auto-updated.
Right. I added them to Version.Details.xml - is the right way to go?
This will also require adding mibc dependencies via darc, so they are auto-updated.
Right. I added them to Version.Details.xml - is the right way to go?
Yes, but they also should be enabled in darc for the msbuild, so they are updated automatically. But this should explicitly be agreed upon with sdk folks. @baronfel is it the way to go? Or should we pack profiles with our nuget (or separate nuget) and when repacking, use them from there?
I would personally say the latter is a better way to go, since we probably also want to use those profiles when building proto.
I'm not sure what's best from an engineering PoV - @joeloff do you have an opinion here?
Thinking again about this, I think us packing profiles should be better, since we know how to manage them and which channel is correct for which branch (we had to use 8 before and now switched to 9).
We were looking at this with @KevinRansom and most likely F# is the first one to try to do this via SDK (there is no notion of embed-pgo-data and not even a way to give extra args to xgen2). Hence the precise way to do this should indeed be agreed on in case some other .NET component decides to copypaste get inspired by this.
For reference, here is the alternative approach in the F# repo, in case we decide to do it from from within the compiler itself.
I vote we distribute profiles with our packages (maybe new package), since
- We know which channel to consume them from for any given branch (it's going to be 9 for a while for all branches, then eventually 10 for net10).
- Ae want to use them when we build proto compiler, and to not duplicate effort, targets, etc, we download them once in our CI, use for proto and pack them for product, and when repacking here, we just assume they're part of our package(s).
Any other opinions here? @baronfel @joeloff @KevinRansom? We'd need to decide soon-ish if we want to get this into .NET 9 which would be definitely nice to have.
I am biased since we kind of have a working thing in F# whereas this PR will take some time to finalize so ofc laziness is involved but it's important to do things "right" here.
Certainly we can also merge the F# repo implementation and then revert later if needed.
I thought about it over the weekend, and probably going to insist that we do it in our repo:
- Download profiles there, as we already do, use it on Proto compiler
- Pack them when publishing product compiler, from signed CI (together or separate package, doesn't really matter).
- Unpack them in this repo, and reapply while repacking.
This way we are flexible to do whatever we want without making changes to this repo. We know exactly which channel to pull packages from for every given branch. We can transparently change implementation if we want it (either use runtime provided ones, or generate our own), without changes to this repo. It is unlikely that we will be forgetting to update darc for sdk every time we update branches, etc.
/azp run
Azure Pipelines successfully started running 4 pipeline(s).
/azp run
Azure Pipelines successfully started running 4 pipeline(s).
CI seems to be flaky but overall this looks like a viable approach. The MIBC files are present in the packages already:
@KevinRansom @vzarytovskii please rereview.
Okay, this is green 😎
Okay, that's green again... @mmitche @vzarytovskii @baronfel @KevinRansom how does this look like to you?
@KevinRansom do you have some thoughts here?
This was targeted at main, so now it's slated for .NET 10 - do y'all want to try and take this for 9 GA?
This was targeted at main, so now it's slated for .NET 10 - do y'all want to try and take this for 9 GA?
Probably not ga, maybe for 200+
This was targeted at main, so now it's slated for .NET 10 - do y'all want to try and take this for 9 GA?
Probably not ga, maybe for 200+
One downside to this is that Source Build won't make use of the feature.
Yes, we can't use blobs there unfortunately