JIT: Synthesize profile data when invalid counts are discarded - #133206
JIT: Synthesize profile data when invalid counts are discarded#133206tannergooding wants to merge 1 commit into
Conversation
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
12220d4 to
0158547
Compare
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It changes JIT PGO/profile-synthesis behavior in a way that can broadly affect optimization decisions and should get a final human review for correctness and perf/regression risk.
Review tier: Lite
Findings: None
What changed in this PR
This PR adjusts how the JIT finalizes profile incorporation when sparse edge-count PGO reconstruction fails after incorporation has started. Instead of always “repairing” likelihoods, it now detects when incorporation has cleared fgPgoHaveWeights and performs a full reset + heuristic resynthesis of edge likelihoods/weights.
Changes:
- After incorporating edge or block counts, conditionally run
RepairLikelihoodsonly if profile weights remain valid. - If edge-count reconstruction discards the profile (clears
fgPgoHaveWeights), runResetAndSynthesizeto reinitialize likelihoods and recompute weights using normal heuristics.
| File | Description |
|---|---|
| src/coreclr/jit/fgprofile.cpp | Chooses between repairing likelihoods vs resetting + synthesizing when profile incorporation discards weights. |
|
CC. @AndyAyersMS, @EgorBo Ran across this when investigating a C# side change around the span slice logic. In particular, the following managed diff invalidates the static PGO data: - if ((ulong)(uint)start + (ulong)(uint)length > (ulong)(uint)_length)
+ if ((uint)start > (uint)_length || (uint)length > (uint)(_length - start))However, because of the current setup we were trying to repair them and this causes us to track the edges as having been seeded from PGO even though they weren't due to it being stale. So the repair has nothing to handle and it gives them initial likelihoods of This fixes it so that invalidated PGO no longer prevents normal heuristics from kicking in and allows things to work "as normal". It should ideally help mitigate issues from out of date PGO when such changes are made and is short lived since the next PGO update should "fix" things to no longer be invalidated |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The change is narrowly scoped, uses existing synthesis modes, and correctly avoids re-marking discarded count data as profile-derived by switching to reset+synthesis when fgPgoHaveWeights is cleared.
Review tier: Lite
Findings: None
|
Looks like there are substantial SPMI diffs -- though no details for x64 yet. Either this is kicking in a lot more than we think, or our static PGO data is really stale. Can you dig into some of these diffs? |
|
@AndyAyersMS looks like it's mostly because of inlining methods whose dynamic PGO edge counters were present but all zero.
Essentially, if dynamic PGO is active but no profile is available for a method, we already synthesize weights using the normal heuristics. Likewise, if profile counts are available and usable, we keep them and repair any inconsistencies. These diffs are primarily from the middle scenario where profile data existed but reconstruction rejected all of it. The flow graph initially gives conditional edges A breakdown of one of the more common samples done by GPT is below... One of the more common dynamic PGO cases looks like this: if (value == null)
{
FailArgumentNullException(parameterName);
}One ASP.NET SuperPMI context compiles Previously we kept the initial SuperPMI is replaying what the original process returned to the JIT here, rather than collecting new PGO itself. Inlinees are instrumented by default, so this is not an inherent limitation of dynamic PGO with inlining; these particular counters simply were not hit during collection. |
Interesting, yes it looks like this was making us overly aggressive in some cases. Thanks for the fix. |
Edge-count reconstruction can discard stale, malformed, all-zero, or unreconstructable PGO data after profile incorporation has already begun. Running
RepairLikelihoodsafterward preserves the default edge likelihoods as profile-derived, preventing later heuristics from adjusting cold throw paths.Use
ResetAndSynthesizewhen incorporation clearsfgPgoHaveWeights; successfully incorporated profiles continue through repair.Note
This pull request description was generated by GitHub Copilot.