[Profiler] Duplicate GarbageCollectionFinished notification

[Liangjia0411]

Hi, @mdh1418 ,I use ICorProfiler to trace GC Start Event and GC End Event. I use the Event Mask COR-PRF-HIGH_SASIC_GC to track GC events, which will not disable concurrent GC.
Now I have found that Garbage Collection Finished may be called twice in a row.Is this a bug?
I don't seem to have found any relevant answers in #108273.

[Liangjia0411]

I also use EventListener to trace it, , and the result was the same

[mdh1418]

From Maoni's comment in #108273, it doesn't appear to be a bug.

it's perfectly normal to receive a GarbageCollectionFinished for a foreground ephemeral GC followed by another GarbageCollectionFinished for a BGC.

Yes, you are correct that COR_PRF_HIGH_BASIC_GC will not trigger the code paths in which the profiler will disable concurrent GC, which is checked by COR_PRF_MONITOR_GC as I had mentioned in #108273 (comment).

The profiler should be disabling concurrent GC with that event mask either when the profiler loads at startup or when the profiler is attaching and disabled here.

[Liangjia0411]

Based on the description in (https://learn.microsoft.com/en-us/dotnet/framework/unmanaged-api/profiling/icorprofilercallback2-garbagecollectionfinished-method), it appears that GarbageCollectionFinished only calls back when GC has completely ended, so should we optimize the document?

[mdh1418]

The GarbageCollectionFinishedCallback does get invoked at the end of GC

runtime/src/coreclr/vm/gcenv.ee.cpp

Lines 853 to 870 in 39a31f0

	void GCToEEInterface::DiagGCEnd(size_t index, int gen, int reason, bool fConcurrent)
	{
	#ifdef GC_PROFILING
	// We were only doing generation bounds and GC finish callback for non concurrent GCs so
	// I am keeping that behavior to not break profilers. But if BasicGC monitoring is enabled
	// we will do these for all GCs.
	if (!fConcurrent)
	{
	GCProfileWalkHeap(false);
	}
	if (CORProfilerTrackBasicGC() || (!fConcurrent && CORProfilerTrackGC()))
	{
	DiagUpdateGenerationBounds();
	GarbageCollectionFinishedCallback();
	}
	#endif // GC_PROFILING
	}

.

How should the language be changed? From my understanding, I think a foreground ephemeral GC occurring during a background GC is a different GC instance, so it seems correct that the callback is invoked when the foreground ephemeral GC instance ends and when the background GC instance ends.

[Liangjia0411]

I agree with your statement, but as users, they may not be familiar with the specific implementation of GC. They may only want to care about the beginning and end of a complete GC, and when "GCStart=>GCEnd=>GCEnd" appears, they may feel confused

[mdh1418]

Right, I think there is an opportunity to pass the args of DiagGCEnd into the callback as suggested in #108273 (comment). Not sure yet if there is a particular reason why it wasn't done previously.