diagnostics icon indicating copy to clipboard operation
diagnostics copied to clipboard

[Tests] Update GC SegmentEvents validation

Open mdh1418 opened this issue 1 year ago • 3 comments

Fixes https://github.com/dotnet/diagnostics/issues/3143

With the new GC introduced in .NET 7, there are no longer any GC segments being freed, so the GCFreeSegment event is not guaranteed to fire during GC. As the GCFreeSegment event is documented to track when a garbage collection segment has been released, and because Perfview does not utilize GCFreeSegment/GCCreateSegment events, it seems more appropriate to adjust expectations for what GC events are fired rather than forcing GCFreeSegment events to fire for "analogous" events.

Moreover, there is a new CommittedUsage events that shows GC committed memory and breaks down what the memory is used for. As such, GCFreeSegment/GCCreateSegment is no longer needed.

mdh1418 avatar Sep 04 '24 21:09 mdh1418

@cshung Would it make sense to have an expectation for a positive number of CommittedUsage events now during a GC for .NET 8+? Wondering if there is a parallel with the old GC's expectation for positive number of GC*SegmentEvents for the new GC.

Adding a validator for CommittedUsage "worked", but I'm not sure if it is supposed to be a guarantee now during a normal GC

Func<EventPipeEventSource, Func<int>> _DoesTraceContainEvents = (source) => {
    int GCCommittedUsageEvents = 0;
    source.Clr.GCDynamicEvent.GCCommittedUsage += (eventData) => GCCommittedUsageEvents += 1;

    int GCAllocationTickEvents = 0;
    source.Clr.GCAllocationTick += (eventData) => GCAllocationTickEvents += 1;

    return () => {
        Logger.logger.Log("Event counts validation");

        bool GCCommittedUsageResult = GCCommittedUsageEvents > 0;
        Logger.logger.Log("GCCommittedUsageResult: " + GCCommittedUsageEvents);

        Logger.logger.Log("GCAllocationTickEvents: " + GCAllocationTickEvents);
        bool GCAllocationTickResult = GCAllocationTickEvents > 0;
        Logger.logger.Log("GCAllocationTickResult: " + GCAllocationTickResult);

        bool GCCollectResults = GCCommittedUsageResult && GCAllocationTickResult;
        Logger.logger.Log("GCCollectResults: " + GCCollectResults);

        return GCCollectResults ? 100 : -1;
    };
};

But in order for this to be added, it would need the Microsoft.Diagnostics.Tracing.TraceEvent version to be bumped. When might be reasonable to bump that in this repo @mikem8361 If neither that version should be bumped nor should there be a parallel expectation with CommittedUsage events, then I'll reduce the validation to just the GC allocation tick event

mdh1418 avatar Sep 06 '24 20:09 mdh1418

It sounds reasonable to me but I'm not an expert on TraceEvent.

mikem8361 avatar Sep 07 '24 17:09 mikem8361

@cshung Would it make sense to have an expectation for a positive number of CommittedUsage events now during a GC for .NET 8+?

Short answer: Yes. Long answer: It depends on version and platforms.

Here are some of the specifics:

Event counts:

  • CommittedUsage event is introduced in this commit. This commit is .NET 8.
  • We have two CommittedUsage events before and after a blocking GC for regions.
  • We have one CommittedUsage event before a background GC for regions.
  • We don't have any CommittedUsage event for segments before this commit. This commit is .NET 10
  • After that commit, we have exactly the same CommittedUsage events counts for segments as well.

Version and platforms:

  • For .NET 7+, 32 bit platforms (e.g. ARM/x86) or osx uses segments, other platforms uses regions by default.
  • Under .NET 7, NativeAOT used segments. Support for NativeAOT to use regions is available for .NET 8+ after this commit.

All of these can be overridden if user uses the standalone GC, you can even use regions for .NET 6.

cshung avatar Sep 09 '24 17:09 cshung