Skip to content
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

Dispose parameters, kill benchmarking process when it hangs after printing the results #1571

Merged
merged 7 commits into from
Oct 26, 2020

Conversation

adamsitnik
Copy link
Member

@adamsitnik adamsitnik commented Oct 23, 2020

#1383 contained a very unusual bug: benchmarks were executing fine, but hanging after printing "// AfterAll" to the console

I've dug into that and found out (#1383 (comment)) that there is a locking finalizer which prevents the process from exiting which was a thing until it got fixed in .NET 5 (dotnet/runtime#314)

The problem was that disposing the finalizable instance in the [GlobalCleanup] was not enough: https://github.com/JohnMasen/VirusSimulator/blob/432e185866b80aac69b95abbeb00d1bb2e3735c1/src/ComputeShaderTest/ComputeShaderPerformanceILGPU.cs#L63

For two reasons:

  • when we are using [ParamsSource] or [ArgumentsSource] the boilerplate code was so far calling .ToArray()[index] on it, where index was the index of the test case. It meant that if the benchmark had 3 test cases, we were always creating 3 of them in the benchmarking process. The solution was to introduce a new method ParameterExtractor.GetParameter that enumerates over the arguments source and disposes unused arguments. It also does not create more than needed. So if we want just the first argument, it stops iterating after first occurrence and hence creates only one instance of the given type.
  • BenchmarkConverter is also materializing all objects from [ParamsSource] or [ArgumentsSource] to be able to run the benchmarks. The solution was to add a Dispose after running the benchmarks. Thanks to that, the host process stopped hanging after printing // * Artifacts cleanup * at the end

Last but not least, the fix was specific to finalizable arguments. To make it more robust, I've extended the executors with the following logic:

  • when the benchmark process sends the last signal (AfterAll) we stop processing the output stream of the benchmark process. we know it has finished, there is no need to try to read the stream (and hung while trying to do that).

BenchmarkDotNet.Engines.HostExtensions.AfterAll(host);

  • provide a timeout when waiting for the benchmarking process to finish. If it does not quit within 250ms, we are force killing it.

I know that the changes are quite complex, but I have tested them very carefully and ensured that with this extra logic it will be now much harder to hang BenchmarkDotNet.

Fixes #1383
Fixes #828

the benchmark process is catching all exceptions and writing to standard output, so it's OK (this is what the class Executor does already)
so we need to dispose them after we are done running the benchmarks

see #1383 and dotnet/runtime#314 for more
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants