Conversation
|
@microsoft-github-policy-service agree company="Microsoft" |
|
Seems like something in your changes broke one of the tests. Please resolve before we review the changes. Good stuff though computing some more timing metrics after parsing. I'm looking forwards to seeing the new data in the graphs. |
The remaining problem is failing a master file comparison, which isn't exactly surprising. Can you help me with either running the test locally or getting a hold of the output file generated by the CI test? |
Tests are in the InstrumentsProcessorTests project. You are getting a null reference exception. |
There was a problem hiding this comment.
Pull request overview
This PR implements two main improvements to the Instruments processor:
Purpose: Normalize CPU usage graphs to 100% by inferring idle time for each processor, and improve event deserialization performance by moving type manipulations to happen once per schema rather than per event.
Changes:
- Refactored deserialization code to use static fields for property/deserializer mappings, reducing overhead during event processing (20% performance improvement)
- Added idle time tracking and synthesis for both TimeProfile and ThreadState events to normalize CPU usage graphs
- Introduced new CPU data model to parse core ID and formatted core name separately
- Renamed columns for clarity (e.g., "Duration" → "CPU Time", "CPU Time" → "Ready Time")
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| EventDeserializer.cs | Optimized deserialization by moving property/deserializer collection to static fields and pre-building setters array per schema |
| XmlNodeDeserializer.cs | Moved property deserializer collection to static initialization for performance |
| IEventDeserializer.cs | Removed schema parameter from Deserialize method (now handled in CanDeserialize) |
| TraceSourceParser.cs | Updated to call Deserialize without schema parameter |
| Schema.cs | Fixed spelling of "Mnemonic" property |
| CPU.cs | New data model to parse CPU core ID and formatted name from XML |
| String.cs, Integer.cs, Timestamp.cs, TimestampDelta.cs | Added constructors for programmatic creation of instances |
| Thread.cs, Process.cs | Added static idle instances and helper methods |
| TimeProfileEvent.cs, ThreadStateEvent.cs | Added MakeIdleEvent factory methods for synthesizing idle time events |
| IdleTimeTracker.cs | New class to track last activity time per CPU core and compute idle gaps |
| TimeProfileCooker.cs | Added idle time synthesis before adding events |
| ThreadStateCooker.cs | Added state-based filtering, ready/waiting time accumulation, and idle time synthesis |
| Projector.cs | Added null-safe access patterns and new projectors for CPU ID and thread name |
| CpuSamplingTable.cs | Added CPU ID and Thread Name columns, changed timestamp role from StartTime to EndTime, removed processor class column |
| CPUPreciseTable.cs | Renamed columns, added CPU ID and Thread Name columns, removed filtering logic (moved to cooker), added ResourceId column roles |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Normalize CPU usage graphs (Precise and Sampled) to 100% by inferring idle time for each processor. This is a better representation of overall CPU use - previously, these graphs had arbitrary maximums.
Make event deserialization more efficient by moving various type manipulations to happen once, rather than per event processed. This reduces source file load time by 20%