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

Enable PGO - alternative approach #17317

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

Conversation

psfinaki
Copy link
Member

@psfinaki psfinaki commented Jun 17, 2024

Description

This should eventually make the FSC and FSI start about 30% faster. The implementation is inspired by the runtime targets.

Fixes #12636
Fixes #13328

Copy link
Contributor

✅ No release notes required

@psfinaki
Copy link
Member Author

Hi @EgorBo, so this is an alternative approach to enable PGO, without doing profile generation ourselves.
However - this doesn't seem to work so far, as in, benchmarks don't show any improvements.

Any ideas what could be wrong?

@vzarytovskii
Copy link
Member

@EgorBo any pointers on how to figure out why is profile we get from nuget is not applying, and if we generate it locally, it shows a difference?

@EgorBo
Copy link
Member

EgorBo commented Jun 19, 2024

@psfinaki @vzarytovskii I presume you test it on fsi? have you added fsi as a training scenario to dotnet-optimization repo? Otherwise it produces some MIBC trained to TechEmpower, console apps and for F# it's only "build Giraffe" scenario

@vzarytovskii
Copy link
Member

@psfinaki @vzarytovskii I presume you test it on fsi? have you added fsi as a training scenario to dotnet-optimization repo? Otherwise it produces some MIBC trained to TechEmpower, console apps and for F# it's only "build Giraffe" scenario

We have tested on fsc, since that's the only F# scenario in the optimization repo as of now.

@psfinaki did you try to build Giraffe with and without profile applied in this PR? Does it change anything?

@psfinaki
Copy link
Member Author

So far I have only tried running published fsc against an empty test.fsx script. That was enough to show the differece in that other PR with us creating the profile.

Let me try Giraffe.

@psfinaki
Copy link
Member Author

@vzarytovskii @EgorBo I'm afraid I don't see any difference in Giraffe compilation either :( it's the same execution time basically.

@EgorBo
Copy link
Member

EgorBo commented Jun 20, 2024

@vzarytovskii @EgorBo I'm afraid I don't see any difference in Giraffe compilation either :( it's the same execution time basically.

Do you a MIBC that works locally? can you send it to me? I'll check weigths in it

@vzarytovskii
Copy link
Member

vzarytovskii commented Jun 26, 2024

@EgorBo did you, by any chance, look at it? Curious what's the difference between those.

@EgorBo
Copy link
Member

EgorBo commented Jun 26, 2024

@EgorBo did you, by any chance, looked at it? Curious what's the difference between those.

The only @psfinaki collected looks extremely weird to me. I propose we merge this PR as is since it generally looks corret to me, the problem must be inside the dotnet-optimization repo, once we figure it out and fix it should propagate a correct MIBC and optimize things out, hopefully 🙂 I'll take a look on Friday

@psfinaki psfinaki marked this pull request as ready for review June 26, 2024 14:11
@psfinaki psfinaki requested a review from a team as a code owner June 26, 2024 14:11
@psfinaki psfinaki mentioned this pull request Jun 26, 2024
2 tasks
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 I get less sick we should talk about this.

@@ -54,4 +54,15 @@
<PackageReference Include="Microsoft.Build.Utilities.Core" Version="$(MicrosoftBuildUtilitiesCoreVersion)" />
</ItemGroup>

<Target Name="Add PGO" DependsOnTargets="Get MIBC data" BeforeTargets="Publish" Condition="'$(Configuration)' == 'Release'">
Copy link
Member

Choose a reason for hiding this comment

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

We need to ship fsc and fsi to the dotnetsdk as il assemblies not native compiled assemblies.

The sdk native compiles F# in this project here: https://github.com/dotnet/sdk/blob/main/src/Layout/tool_fsharp/tool_fsc.csproj

Copy link
Member

Choose a reason for hiding this comment

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

So imagine, we will need to work with the sdk team to enable the pgo stuff ... at least if it relies on native compilation.

Copy link
Member

Choose a reason for hiding this comment

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

We called certainly add the mibc files to the package we insert into the sdk.

Copy link
Member

@vzarytovskii vzarytovskii Jun 28, 2024

Choose a reason for hiding this comment

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

I don't think sdk does anything, @baronfel doesn't it just repack stuff from nugets?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was looking at the tool_fsc (and also this) and that was my impression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Add mibc/profile for R2R FSC/FSI for faster startup dotnet fsi startup overhead surprisingly high
4 participants