-
Notifications
You must be signed in to change notification settings - Fork 354
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
[Tests] Update GC SegmentEvents validation #4906
base: main
Are you sure you want to change the base?
Conversation
The *ConcurrentThreadEvents count validation was removed as part of dotnet#689 due to reported flakiness of the event being fired before observing the first runtime event.
The new GC, supposedly introduced in .NET 7, stopped emitting GCFreeSegment events as it transition from performing virtual_free to virtual_decommit. As such, GCFreeSegments are no longer expected.
src/tests/eventpipe/GCEvents.cs
Outdated
// Disable checking GCFreeSegmentEvents on .NET 7.0 issue: https://github.com/dotnet/diagnostics/issues/3143 | ||
bool GCSegmentResult = GCCreateSegmentEvents > 0 && (GCFreeSegmentEvents > 0 || Environment.Version.Major >= 7); | ||
Logger.logger.Log("GCSegmentResult: " + GCSegmentResult); | ||
GCSegmentResult = GCCreateSegmentEvents > 0 && GCFreeSegmentEvents > 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also expect the GCCreateSegmentEvents not to be generated? The test appears to have stopped checking both but the comments only mention GCFreeSegment events stopped. Assuming CreateSegment events are still expected I'd suggest only removing the GCFreeSegmentEvents portion of the check.
I don't think we need to make it conditional on major version either. As long as the test won't start failing when run on older versions it should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the runtime, there are still technically instances of firing GCCreateSegment_V1
and GCFreeSegment_V1
events. However, from offline discussions summarized in #3143 (comment), this PR is driving towards removing expectations of those events being fired during the new GC.
Since both GCCreateSegment and GCFreeSegment are documented to specifically mean GC segments being created/released, and since the new GC uses regions, I figured that it would be better keep those events to specifically mean GC segments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if both of those events are affected thats np, the comments elsewhere had given a false impression that the FreeSegment events were the only one impacted.
@cshung Would it make sense to have an expectation for a positive number of Adding a validator for 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 |
It sounds reasonable to me but I'm not an expert on TraceEvent. |
Short answer: Yes. Here are some of the specifics: Event counts:
Version and platforms:
All of these can be overridden if user uses the standalone GC, you can even use regions for .NET 6. |
This reverts commit d18b2af.
Fixes #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.