Improve memory diagnoser accuracy

[timcassell]

This doesn't fix the zero-alloc measurement, but it does fix a bug from the threading diagnoser interfering with the results. The other changes give us the highest confidence that any errant measurements are not our fault. The rest is up to the runtime.

Fixes #1542

[timcassell]

It looks like I may have resolved the zero-alloc issue!

New results (using the same benchmark from #1543):

Method	Runtime	Allocated
Benchmark1	.NET 6.0	2 B
Benchmark2	.NET 6.0	2 B
Benchmark3	.NET 6.0	2 B
Benchmark4	.NET 6.0	2 B
Benchmark5	.NET 6.0	2 B
Benchmark1	.NET 7.0	-
Benchmark2	.NET 7.0	-
Benchmark3	.NET 7.0	-
Benchmark4	.NET 7.0	-
Benchmark5	.NET 7.0	-
Benchmark1	.NET 8.0	-
Benchmark2	.NET 8.0	-
Benchmark3	.NET 8.0	-
Benchmark4	.NET 8.0	-
Benchmark5	.NET 8.0	-

The resolution ended up being moving the GcStats measurement to its own method, and removing GC.Collect() from the allocation measurement. Doing only 1 of those still wasn't getting the expected results.

I was unable to repro the issue in a plain console application, so I'm not sure why GC.Collect() interferes with the results (I would have reported a bug in the runtime repo if I could).

PTAL @adamsitnik

[timcassell]

I added the thread sleep from #1543 in Core runtimes 3.0 to 6.0, and now I'm getting 0 measurements across the board:

Method	Runtime	Allocated
Benchmark1	.NET 6.0	-
Benchmark2	.NET 6.0	-
Benchmark3	.NET 6.0	-
Benchmark4	.NET 6.0	-
Benchmark5	.NET 6.0	-
Benchmark1	.NET 7.0	-
Benchmark2	.NET 7.0	-
Benchmark3	.NET 7.0	-
Benchmark4	.NET 7.0	-
Benchmark5	.NET 7.0	-
Benchmark1	.NET 8.0	-
Benchmark2	.NET 8.0	-
Benchmark3	.NET 8.0	-
Benchmark4	.NET 8.0	-
Benchmark5	.NET 8.0	-
Benchmark1	.NET Core 2.1	-
Benchmark2	.NET Core 2.1	-
Benchmark3	.NET Core 2.1	-
Benchmark4	.NET Core 2.1	-
Benchmark5	.NET Core 2.1	-
Benchmark1	.NET Core 3.0	-
Benchmark2	.NET Core 3.0	-
Benchmark3	.NET Core 3.0	-
Benchmark4	.NET Core 3.0	-
Benchmark5	.NET Core 3.0	-

[adamsitnik]

Big thanks for working on improving the accuracy of the memory diagnoser @timcassell !

Let's wait for the response from @Maoni0.

[timcassell]

Update: I found the culprit and opened an issue in dotnet/runtime. dotnet/runtime#101536

[timcassell]

I disabled the flaky tests and left a todo to re-enable them when that issue is fixed.

This is as accurate as we can make the memory diagnoser from our side. I think this should be good to merge now, unless you have any further feedback @adamsitnik.

[timcassell]

Ok, I re-enabled the tests and added a finalizer thread blocker to make the tests less flaky (nice idea @jkotas). This does not have any effect on real world measurements, though.

[timcassell]

Wow, even with tiered jit disabled and finalizer thread blocked, the CI is still flaky.

2024-04-28T00:50:55.0136465Z   Failed AllocationQuantumIsNotAnIssueForNetCore21Plus(toolchain: .NET 8.0) [9 s]
2024-04-28T00:50:55.0137358Z   Error Message:
2024-04-28T00:50:55.0137773Z    Assert.Equal() Failure: Values differ
2024-04-28T00:50:55.0138293Z Expected: 88
2024-04-28T00:50:55.0138634Z Actual:   176

I don't know what causes that behavior, but I just moved the MemoryDiagnoserTests to ManualRunning to stabilize the CI.

[jkotas]

Wow, even with tiered jit disabled and finalizer thread blocked, the CI is still flaky.

My guess is that the next problem are the event source background threads.

[timcassell]

My guess is that the next problem are the event source background threads.

Can you provide more details? I couldn't find information about it from google.

[jkotas]

EventSource and EventPipe create background threads that may allocate. These background threads should be only doing work if there is something listening to the events. It turns out that there is pretty much always something listening to .NET runtime events in typical machine configs these days.

[timcassell]

You mentioned disabling that in the other issue. How can we do that?

[timcassell]

@adamsitnik Do you have any more feedback here?

[neuecc]

It seems the MemoryDiagnoser is returning obviously strange values.
BenchmarkDotNet is generally used for values released to the public, but with this issue there's no credibility.
What's the current status of this fix? @adamsitnik

[timcassell]

Fixed the last remaining flaky test allocation in the last commit. @adamsitnik PTAL

[timcassell]

@AndreyAkinshin Can you review this if Adam is too busy?

[timcassell]

Merging this. If you have any feedback, I can address it as follow-up @adamsitnik @AndreyAkinshin.

[neuecc]

Thank you @timcassell
The values have significantly improved in several benchmarks.
For example, Where01 previously showed 147B allocations, but now it's zero.
Select4 was measuring 1B, but this is also zero now.
There still seem to be some cases where things that should be zero have a little remaining, but overall it looks good.

Before: https://github.com/Cysharp/ZLinq/actions/runs/15248058870
After: https://github.com/Cysharp/ZLinq/actions/runs/15697569428

[timcassell]

Awesome! It's nice to see improvements in real-world benchmarks.

There still seem to be some cases where things that should be zero have a little remaining, but overall it looks good.

Which benchmark do you expect to be zero that's not?

I did see you are using ShortRun job settings. Do note that we cannot guarantee allocation results for methods that are not tier 1 jitted. JIT is free to insert allocations in tier 0 methods that's impossible for user code to do anything about. If you need the most accurate tier 1 measurements, mitigation options are to ensure the benchmark method is invoked at least 60 times (undocumented number that may change at any point), or disable tiered jit.

[neuecc]

Count00 shows 2B.
https://github.com/Cysharp/ZLinq/blob/66448d45/sandbox/Benchmark/Benchmarks/LinqPerfBenchmarks.Count00.cs#L35-L47
If this is not an oversight in my code, this code is expecting 0B.

I did see you are using ShortRun job settings

Thank you for the additional detailed information, it's very helpful!
ShortRun was essential for mitigating execution time due to the large number of benchmarks and for faster iteration when experimenting locally, but I see that such side effects can exist.