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

Add PGO for F# compiler #42514

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Add PGO for F# compiler #42514

wants to merge 17 commits into from

Conversation

psfinaki
Copy link
Member

@psfinaki psfinaki commented Aug 2, 2024

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.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Infrastructure untriaged Request triage from a team member labels Aug 2, 2024
@vzarytovskii
Copy link
Member

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

@psfinaki
Copy link
Member Author

psfinaki commented Aug 2, 2024

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?

@vzarytovskii
Copy link
Member

vzarytovskii commented Aug 2, 2024

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.

@baronfel
Copy link
Member

baronfel commented Aug 2, 2024

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

@vzarytovskii
Copy link
Member

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

@psfinaki
Copy link
Member Author

psfinaki commented Aug 2, 2024

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.

@vzarytovskii
Copy link
Member

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

@psfinaki
Copy link
Member Author

psfinaki commented Aug 5, 2024

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.

@vzarytovskii
Copy link
Member

vzarytovskii commented Aug 5, 2024

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.

@psfinaki
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@psfinaki psfinaki marked this pull request as ready for review August 28, 2024 16:59
@psfinaki
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@psfinaki
Copy link
Member Author

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
Copy link
Member Author

Okay, this is green 😎

<PropertyGroup>
<MibcTargetOS>linux</MibcTargetOS>
<!-- We deliberately want to use linux mibc pgo data on macOS -->
<MibcTargetOS Condition="$([MSBuild]::IsOSPlatform('OSX'))">linux</MibcTargetOS>
Copy link
Member

Choose a reason for hiding this comment

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

When the sdk builds it builds cross target --- ie the tooling used to build the product doesnot necessarily match the build target.

When you do mibc you need to match the build target not the tooling platform.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm alright, I am trying to leverage NETCoreSdkRuntimeIdentifier now - is this the right thing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably not :)

Copy link
Member

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

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

When the sdk builds it builds cross target --- ie the tooling used to build the product doesnot necessarily match the build target.

When you do mibc you need to match the build target not the tooling platform.

@psfinaki
Copy link
Member Author

psfinaki commented Sep 12, 2024

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

<PropertyGroup>
<MibcPath>$(PkgMicrosoft_FSharp_Compiler)/contentFiles/mibc</MibcPath>
<MibcRid>$(Rid)</MibcRid>
<MibcRid Condition="$(MibcRid.Contains('musl-'))">$(MibcRid.Replace('musl-', ''))</MibcRid>
Copy link
Member

Choose a reason for hiding this comment

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

Is this valid @mmitche? How portable are PGO profiles, can they float across different libc ABIs?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure. I'm going to draw a random name out of the hat on that one and tag @agocke for general knowledge.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe @EgorBo?

@psfinaki
Copy link
Member Author

@KevinRansom do you have some thoughts here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Infrastructure untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants