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

New Instrumentation Support for .NET 6 #1481

Closed
wants to merge 3 commits into from
Closed

New Instrumentation Support for .NET 6 #1481

wants to merge 3 commits into from

Conversation

ivdiazsa
Copy link

Completes work item described in issue #1467.

For .NET 6.0, several new GC events were introduced. PerfView requires to know about them as well, so traces can be interpreted properly. This PR adds the support.

@ivdiazsa
Copy link
Author

Hi @brianrob! I've made good progress on this. However, for some reason I feel I'm missing stuff. But I'm not sure exactly what. Could you please take a look when you're able to, and give me pointers on what I might be missing? Thanks a lot!

@brianrob
Copy link
Member

brianrob commented Sep 1, 2021

@ivdiazsa, I am not familiar with all of the runtime side changes to be able to compare this against them, but overall, this change looks reasonable, and would basically make it such that you can effectively parse the changes to the events in the runtime. It looks like the CI is failing, but this is due to a compiler error, not a test error. Is there a particular area in which you have concern that I should focus on?

@ivdiazsa
Copy link
Author

ivdiazsa commented Sep 1, 2021

@brianrob Thanks for your feedback! I mainly focused on translating the new additions from the runtime's CLR Manifest File into classes, etc of the ClrTraceEventParser. So I followed the patterns of other templates into classes, and whatnot, but I wanted to make sure things glaringly don't look bad or I didn't miss anything to add to the TraceEvent parser.

The compilation error was a typo at some point. I found it yesterday while building locally but haven't uploaded the fix. Will do that soon. The other thing I wanted to ask you is if you could point me to docs or examples of existing tests so I can write some to ensure my new additions actually give coherent numbers.

@brianrob
Copy link
Member

brianrob commented Sep 1, 2021

For tests, I think you should add a new test trace that gets run through https://github.com/microsoft/perfview/blob/main/src/TraceEvent/TraceEvent.Tests/GeneralParsing.cs. This test basically opens each trace, string-ifies all events, and then diffs them against the checked in baseline. You can use this mechanism to make sure that once you verify a file manually, that it remains the same over time.

@ivdiazsa
Copy link
Author

ivdiazsa commented Sep 2, 2021

cc @Maoni0 @cshung @PeterSolMS

@Maoni0
Copy link
Contributor

Maoni0 commented Sep 3, 2021

there's actually quite a bit of work to make this complete. a couple of things are missing -

  • in TraceEvent, none of the variable sized fields are decoded - they will need to be otherwise they are not useful. please see how we decode the generation data in GCPerHeapHistoryTraceData for an example (this is the variable sized fields in GCPerHeapHistory_V3 and they are decoded in this method -
public GCPerHeapHistoryGenData GenData(Gens genNumber)
  • there needs to be changes in TraceManagedProcess.cs otherwise the events are not actually read and packaged into
    a consumable form. the changes in ClrTraceEventParser.cs are to decode the data. you need something to actually read in the events from the trace. this is done by implementing a delegate that gets invoked when we see the event, eg, this is how we read in the perheap hist events:
                source.Clr.GCPerHeapHistory += delegate (GCPerHeapHistoryTraceData data)
                {
                    var stats = currentManagedProcess(data);
                    GCStats.ProcessPerHeapHistory(stats, data);
                };

(ProcessPerHeapHistory also gives you an example of using GenData)

making the new info consumable means we'll need additional fields in the TraceGC class to expose it. feel free to propose these fields in TraceGC and I'd be happy to review them with you.

  • after those we should talk about what to expose in the GCStat view as that's how most people would view the GC events. we don't need to expose all of them since much of this is for GC team's benefit (and we consume TraceGC via the perf infra) but things like the GCSettings rundown event and the Time field in the new version of the globalhist event would be good to expose somewhere in GCStats.

@ivdiazsa ivdiazsa closed this Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants