sdk icon indicating copy to clipboard operation
sdk copied to clipboard

Add PGO for F# compiler

Open psfinaki opened this issue 1 year ago • 15 comments

This should eventually make the FSC about 30% faster.

The latest FCS package contains these MIBC files: image

In the SDK part of it, we need to unpack them, apply to the compiler, and repack.

psfinaki avatar Aug 02 '24 16:08 psfinaki

This will also require adding mibc dependencies via darc, so they are auto-updated.

vzarytovskii avatar Aug 02 '24 17:08 vzarytovskii

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?

psfinaki avatar Aug 02 '24 17:08 psfinaki

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.

vzarytovskii avatar Aug 02 '24 17:08 vzarytovskii

I'm not sure what's best from an engineering PoV - @joeloff do you have an opinion here?

baronfel avatar Aug 02 '24 17:08 baronfel

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).

vzarytovskii avatar Aug 02 '24 18:08 vzarytovskii

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.

psfinaki avatar Aug 02 '24 18:08 psfinaki

I vote we distribute profiles with our packages (maybe new package), since

  1. 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).
  2. 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).

vzarytovskii avatar Aug 02 '24 18:08 vzarytovskii

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.

psfinaki avatar Aug 05 '24 11:08 psfinaki

I thought about it over the weekend, and probably going to insist that we do it in our repo:

  1. Download profiles there, as we already do, use it on Proto compiler
  2. Pack them when publishing product compiler, from signed CI (together or separate package, doesn't really matter).
  3. 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.

vzarytovskii avatar Aug 05 '24 11:08 vzarytovskii

/azp run

psfinaki avatar Aug 27 '24 14:08 psfinaki

Azure Pipelines successfully started running 4 pipeline(s).

azure-pipelines[bot] avatar Aug 27 '24 14:08 azure-pipelines[bot]

/azp run

psfinaki avatar Aug 28 '24 16:08 psfinaki

Azure Pipelines successfully started running 4 pipeline(s).

azure-pipelines[bot] avatar Aug 28 '24 17:08 azure-pipelines[bot]

CI seems to be flaky but overall this looks like a viable approach. The MIBC files are present in the packages already: image

@KevinRansom @vzarytovskii please rereview.

psfinaki avatar Aug 28 '24 17:08 psfinaki

Okay, this is green 😎

psfinaki avatar Aug 29 '24 15:08 psfinaki

Okay, that's green again... @mmitche @vzarytovskii @baronfel @KevinRansom how does this look like to you?

psfinaki avatar Sep 12 '24 17:09 psfinaki

@KevinRansom do you have some thoughts here?

psfinaki avatar Sep 19 '24 16:09 psfinaki

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?

baronfel avatar Oct 17 '24 13:10 baronfel

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+

vzarytovskii avatar Oct 17 '24 13:10 vzarytovskii

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.

baronfel avatar Oct 17 '24 13:10 baronfel

Yes, we can't use blobs there unfortunately

vzarytovskii avatar Oct 17 '24 14:10 vzarytovskii