Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Conversation

dpodder
Copy link

@dpodder dpodder commented Oct 25, 2016

Enable PGO for release/1.1.0 on Windows x64

Port changes from #7423 to release branch, which adds support for PGO to
coreclr.dll and clrjit.dll. Apply additional changes to actually
download and consume optimization profile data during a release build.

These changes together enable PGO on coreclr.dll and clrjit.dll, with
the following performance wins (measured on ASP.NET MusicStore):

  • Improve dotnet build by about 12%
  • Improve dotnet application.dll time-to-main by 15%
  • Improve first-time code paths (requiring JIT) by close to 20%

dpodder and others added 2 commits October 25, 2016 14:14
(Porting commit 114b588 to release/1.1.0)

Update the cmake build system to enable support for Profile Guided
Optimization (PGO) on Windows, and enable this feature for two target
binaries (coreclr and clrjit).

With this change, toggle between instrumented and profile-optimized
settings for target binaries by passing pgoinstrument argument to the build.cmd
Assume profile-optimized mode by default. Fall back to regular non-PGO
optimized builds if profile data is not available.
Restore optimization data from nuget prior to the native build. Update
PGO implementation to consume the data from the restored package.
@dpodder dpodder added this to the 1.1.0 milestone Oct 25, 2016
@dpodder
Copy link
Author

dpodder commented Oct 25, 2016

@brianrob FYI

@gkhanna79
Copy link
Member

@dotnet-bot test Windows_NT arm64 release

@brianrob
Copy link
Member

LGTM

@dpodder dpodder changed the title Enable PGO for release/1.1.0 Enable PGO for release/1.1.0 on Windows x64 Oct 25, 2016
)

# Enable PGO only for optimized configs
set(ConfigTypeList RELEASE RELWITHDEBINFO)
Copy link
Member

Choose a reason for hiding this comment

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

This would have no effect on single config generators like Unix make generator. In that case, there is just single LINK_FLAGS property, not LINK_FLAGS_xxx per each config.
So for Unix, you'll need to use if (UPPERCASE_CMAKE_BUILD_TYPE STREQUAL RELEASE OR UPPERCASE_CMAKE_BUILD_TYPE STREQUAL RELWITHDEBINFO) and add to LINK_FLAGS if it is true.
So I would just put the whole body of the add_pgo under if(WIN32) for now.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the feedback, @janvorli. Unfortunate behavior, but I agree with the conclusion. Fixed here, and we'll make the change in master in a subsequent PR.

@dpodder
Copy link
Author

dpodder commented Oct 26, 2016

@gkhanna79 ARM64 test seems to have failed with an infra issue. Here's the previous result; could you help take a look? https://ci.dot.net/job/dotnet_coreclr/job/release_1.1.0/job/arm64_cross_release_windows_nt_prtest/1/

@gkhanna79
Copy link
Member

@jashook Have you seen the above failure in your CI resumption effort?

@janvorli
Copy link
Member

LGTM

@janvorli
Copy link
Member

@dpodder I am really sorry, I have missed the fact that this is a port of the previous change to the release branch. In that case, we should not be making the change I've suggested here. Could you please revert it?
We can do it later in the main branch.

@dpodder
Copy link
Author

dpodder commented Oct 26, 2016

@janvorli No worries, easy enough. I rolled back the last commit, and will make the change later in master as per your suggestion. Thanks again for the feedback!

@dpodder dpodder merged commit 1602161 into dotnet:release/1.1.0 Oct 26, 2016
@dpodder dpodder deleted the pgo-opt branch October 26, 2016 19:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants