Question/Suggestion is GcStats forcing a GC.Collect when it doesn't need to

[Lexcess]

In the method GetAllocatedBytes in GcStats this code line 128
There is the following code.

        private static long GetAllocatedBytes()
        {
            { mono section omitted }

            // "This instance Int64 property returns the number of bytes that have been allocated by a specific 
            // AppDomain. The number is accurate as of the last garbage collection." - CLR via C#
            // so we enforce GC.Collect here just to make sure we get accurate results
            GC.Collect();

#if CLASSIC
            return AppDomain.CurrentDomain.MonitoringTotalAllocatedMemorySize;
#elif NETSTANDARD2_0
            if (RuntimeInformation.IsFullFramework) // it can be a .NET app consuming our .NET Standard package
                return AppDomain.CurrentDomain.MonitoringTotalAllocatedMemorySize;
    {Rest of method omitted}

The code and the comments imply that the GC.Collect is only there to make the call to MonitoringTotalAllocatedMemorySize as accurate as possible. Given that only some runtimes use that method my question is shouldn't the above section instead read like:

        private static long GetAllocatedBytes()
        {
            { mono section omitted }

#if CLASSIC
            // "This instance Int64 property returns the number of bytes that have been allocated by a specific 
            // AppDomain. The number is accurate as of the last garbage collection." - CLR via C#
            // so we enforce GC.Collect here just to make sure we get accurate results
            GC.Collect();
            return AppDomain.CurrentDomain.MonitoringTotalAllocatedMemorySize;
#elif NETSTANDARD2_0
            if (RuntimeInformation.IsFullFramework)
            {
                // it can be a .NET app consuming our .NET Standard package
                GC.Collect();
                return AppDomain.CurrentDomain.MonitoringTotalAllocatedMemorySize;
            }
  {Rest of method omitted}

This would remove the unnecessary GC.Collect() calls when not running on FX. While the collects are happening outside of the measurement it seems superfluous and also potentially impacts other measurements you might want to make (if for example you wanted to see if performance degrades due to GC over lots of runs).

[adamsitnik]

hi @Lexcess , thanks for validating the correctness of BenchmarkDotNet!

While the collects are happening outside of the measurement it seems superfluous and also potentially impacts other measurements you might want to make (if for example you wanted to see if performance degrades due to GC over lots of runs).

The collects happen outside of measurements, in a dedicated iteration, which is the last thing we do before the process dies. (BDN spawns new process for every benchmark, so side effects are not an issue). So I don't think it's an issue.

However, we enforce a GC.Collect between every iteration when we run the benchmarks. This is the default behaviour which can be disabled by using job.WithGcForce(false) or [GcForce(false)]

[Lexcess]

Thanks for the quick response. That all makes sense and I've seen the GcForce option.

However in the code I linked, if it is running on the .Net Core GC.Collect will be called every time, so that setting seems like it wouldn't make a significant difference. Even though the GC.Collect calls only seem to be required when running on the .Net Framework.

I hope I'm making sense here.

Edit: Thinking some more, I think I see what you mean. The Force GCs between each iteration and the sampling is only happening outside of all the iterations. So it doesn't make a impactful difference.

One reason I was looking at this was that I was investigating calculating memory usage at runtime for another project. So for just for my own curiosity and understanding, the Gc.Collects, while not harmful, aren't actually needed except when running on .Net FX? That is to say they aren't bringing some extra benefit I'm missing?

[adamsitnik]

@Lexcess I have updated our pseudocode docs that I think makes it easier to understand

So how it works:

We perform one extra iteration to get the GC stats AFTER measuring the time:

Result ActualRun(Method method, Job job)
{
    Warmup(method, perfectInvocationCount, unrollFactor);
    var result = Target(method, perfectInvocationCount);

    if (MemoryDiagnoser.IsEnabled)
        var gcStats = MeasureGcStats(method, perfectInvocationCount, unrollFactor);

    return (result - Avg(overhead), gcStats);
}

So having an extra GC.Collect is not an issue, because we don't measure the time for this extra iteration:

GcStats MeasureGcStats(Method method, long invokeCount, long unrollFacto)
{
    // we enable monitoring after main target run, for this single iteration which is executed at the end
    // so even if we enable AppDomain monitoring in separate process
    // it does not matter, because we have already obtained the results!
    EnableMonitoring(); 

    IterationSetup();

    var initialGcStats = GcStats.ReadInitial();

    // we do NOT start any clock here, because the enabled monitoring might have some overhead
    // so we just get the gc stats and ignore the timing
    // it's last thing the process does before it dies, so also enabled monitoring is not an issue for next benchmarks
    // because each of them is going to be executed in a new process

    for (long i = 0; i < invokeCount / unrollFactor; i++)
    {
        // we perform manuall loop unrolling!!
        method(); // 1st call
        method(); // 2nd call

        method(); // (unrollFactor - 1)'th call
        method(); // unrollFactor'th call
    }

    var finalGcStats = GcStats.ReadFinal();

    IterationCleanup();

    return finalGcStats - initialGcStats; // the result is the difference between the stats collected after and before running the extra iteration
}

It't also not an issue, because it is the last thing we do before the process dies, and every benchmark runs in a dedicated process, so it does not affect next runs.

One reason I was looking at this was that I was investigating calculating memory usage at runtime for another project.

In your case it makes sense, because you would run Gc.Collect without really needing it and it could affect the GC self-tuning.

Btw it might change soon https://github.com/dotnet/corefx/issues/30644 (the .NET Core API returns the number only for current thread)

Once again thanks for being curious and validatign our correctness!

[Lexcess]

Thanks for taking the time to answer so thoroughly. I agree it isn't an issue for Benchmark.Net. Also thanks for the link to the other thread on exposing more GC information.

If it is of interest I was trying to adapt some of the GC code from Benchmark.Net to Scientist.Net where the use of GC.Collect does indeed matter. The PR is here: scientistproject/Scientist.net#111

I also made a proposal to add GC.GetAllocatedBytesForCurrentThread to vNext of .Net Standard dotnet/standard#771 which would make collecting these metrics way easier cross platform. It seems like that if the proposed GetAllocatedBytesForAllThreads makes the cut it could do with being added to .Net Standard as well.