Support for tiered compilation #679
Conversation
Change the JitStats view to show data relevant to tiered compilation. Also cleaned up some of the output in general, moved some of the guidance into the user guide, and tried to make the text a little more applicable for customers that are using .Net Core.
Codecov Report
@@ Coverage Diff @@
## master #679 +/- ##
==========================================
- Coverage 17.52% 17.5% -0.02%
==========================================
Files 221 221
Lines 125874 126012 +138
Branches 12137 12153 +16
==========================================
Hits 22057 22057
- Misses 102949 103087 +138
Partials 868 868
Continue to review full report at Codecov.
|
|
Including @jeffschwMSFT FYI. Jeff, Noah is adding tiered jit information to the JIT Stats in case you care. |
| @@ -3391,19 +3415,34 @@ public class JITStats | |||
| /// Update method statistics | |||
| /// </summary> | |||
| /// <param name="_method"></param> | |||
| internal void Update(TraceJittedMethod _method) | |||
| public void Update(TraceJittedMethod _method) | |||
There was a problem hiding this comment.
Why the switch to public? The challenge with this method as written (current situation) is that it must be called exactly once per method - if called more than once it will result in over counting. As a tradeoff we kept this internal so that we had more control over the callers.
There was a problem hiding this comment.
In JitStatsEx.Create PerfView is rebuilding per-module filtered JitStats instances. Previously to construct these instances it had a duplicate copy-pasted implementation of Update() inlined in the for loops. I changed it to call Update() rather than duplicate all my logic changes in both places, mostly for expediency as I tinkered. I could restore the copy-paste style cloning, or perhaps a cleaner option you'd prefer is creating a constructor that takes as input a set of TraceJittedMethod and outputs a JitStats instance?
There was a problem hiding this comment.
I see.... between a rock and hard place. I agree duplicating code is less than ideal. I think a little more work is warranted to avoid the easy mistake of calling this internal implementation detail in the wrong situation which will result in overcounting. The constructor solution seems reasonable, if that works for your perfview.
There was a problem hiding this comment.
Hmm, after thinking about various options I have concluded that the current design, where JITStats is basically simply a 'container of fields' and is WRITABLE is useful (it allows users to make any kinds of statitics over JIT methods that they want). As such it is REASONABLE to have a public mutator that updates the statistics given a method (which is what UPDATE does) because that is HOW YOU USE THE CLASS (you accumulate the particular methods you want into the stats class).
Thus I think it is OK to expose the Update() method, but I would like it renamed to be 'AddMethodToStatistics' when we make it public.
Unless Jeff hates this idea, lets go with that...
| @@ -3586,6 +3638,13 @@ public struct InliningFailureResult | |||
| public string Reason; | |||
| } | |||
|
|
|||
| public enum CompilationThreadKind | |||
There was a problem hiding this comment.
If I recall correctly, we intentionally moved all enums to the top level namespace.
There was a problem hiding this comment.
Looking at the file, we defiantly have some enums in Microsoft.Diagnostics.Tracing.Analysis.GC. This is currently in Microsoft.Diagnostics.Tracing.Analysis.JIT which I think is fine. So I think it is OK to leave it.
| </ol> | ||
| <p><strong>What can go wrong with background JIT compilation.</strong></p> | ||
| <p>It your program has used SetProfileRoot and StartProfile, but JIT compilation (as shown in the JITStats view) does not show any or very little background JIT compilation, there are several issues that may be responsible. Fundamentally a important design goal was to insure that background JIT compilation did not change the behavior of the program under <strong>any circumstances</strong>. Unfortunately, it means that the algorithm tends to bail out quickly. In particular</p> | ||
| <p>It your program has used SetProfileRoot and StartProfile, but JIT compilation (as shown in the JITStats view) does not show any or very little background JIT compilation, there are several issues that may be responsible. Fundamentally a important design goal was to insure that background JIT compilation did not change the behavior of the program under <strong>any circumstances</strong>. Unfortunately, it means that the algorithm tends to bail out quickly. In particular</p> |
There was a problem hiding this comment.
I realize this is not your error, but Can you change
It your program -> If your program
| <li>The method could be jitted by a foreground thread just prior to its first execution, in which case it is contained in the 'Foreground' group</li> | ||
| <li>If the Background JIT feature is enabled the method's usage may have been accurately predicted and jitted in advance on the Multicore JIT background thread, in which case it is accounted for in the 'Multicore JIT Background' group</li> | ||
| </ol> | ||
| <p>In the individual method listings the column 'Trigger' contains the value 'TC' for each Tier1 background recompilation due to Tiered compilation</p> |
There was a problem hiding this comment.
nit -> go ahead and capitalize Compilation (since you capitalized Tiered and you use the abreviation TC).
| @@ -76,89 +76,249 @@ EndProject | |||
| Global | |||
| GlobalSection(SolutionConfigurationPlatforms) = preSolution | |||
| Debug|Any CPU = Debug|Any CPU | |||
| Debug|x64 = Debug|x64 | |||
| Debug|x86 = Debug|x86 | |||
There was a problem hiding this comment.
Do you know why VS added these configurations? I don't much care, as they are benign, but I am surprised your change triggered it.
There was a problem hiding this comment.
I don't offhand. I did not interact with the configuration manager UI or change the current config but VS inserted the entries none-the-less.
| /// </summary> | ||
| public bool IsBackGround; |
There was a problem hiding this comment.
Just want to check - removing this property is OK? This is a public property on a public type. If TraceEvent has similar versioning rules to other BCL libraries removing public surface area would be bad news, but I wasn't sure what rules TraceEvent was operating under.
There was a problem hiding this comment.
While we have wiggle room to break things, I don't like to use it unnecessarily. SInce it is so easy not to break things, that is my recommendation. We like to leave things there but mark them Obsolete to be clear it may go away in the future.
Thus put the IsBackground property back (as a property calling CompilationThreadKind != ForeGround), mark it as [Obsolete("Use CompilationThreadKind")], and you are done.
Change the JitStats view to show data relevant to tiered compilation. Also cleaned up some of the output in general, moved some of the guidance into the user guide, and tried to make the text a little more applicable for customers that are using .Net Core.
@vancem
I'm fairly sure this PR is playing fast and loose with changes in TraceEvent so I expect I'll need to do some further changes there. It wasn't certain what the versioning/API guidelines were for that assembly. I want to do a little more sophisticated analysis than what TraceEvent JitStats currently supports so I think the options are either to add that support in TraceEvent, or shift PerfView further away from TraceEvent's implementation and do more work in JitStatsEx.
I also wanted to get feedback on the UI changes. I think the overall design is roughly what we discussed but there is still lots of detail where I figured it would be fastest to just make a strawman decision and then adjust as needed.