Benchmarkdotnet: [Feature request] Release builds only?

[ig-sinicyn]

Hi!

It's very easy to run benchmark on an assembly built as debug. Sometimes it is good and sometimes definitely not. I've learned that hard way as I just spent some time just to figure out our CI server uses debug builds for our perftests:)

So, I propose the following feature to be included as opt-in:

The ability to proof that benchmark assembly is not built as debug

To define what "debug" assembly mean: it's an assembly with

    [assembly:Debuggable(DebuggableAttribute.DebuggingModes.DisableOptimizations)]

attribute applied.

The API for the feature may look like this (names are preliminary)

        // What to do on debug build
        enum DebugBuildBehavior
        {
            Run = 0,                // Run as usual
            Default = Run,          // The same, used by default
            FailsWithException = 1, // BenchmarkRunner.Run() throws an exception
            DoesNotRun = 2          // Warning is logged, benchmark exits
        }

Also, if DebugBuildBehavior is set to FailsWithException or DoesNotRun and the benchmark assembly is not debug one additional check is performed:
all assemblies being loaded during benchmark run should be checked too (using the AppDomain.AssemblyLoad event to hook it up).

The DebugBuildBehavior enum should be exposed as a property of the IConfig interface so it can be setup individually for each benchmark.

Thanks!:)

[adamsitnik]

Very good point! I agree with you, we should not let the users use non-optimized dlls.

We already preload all dlls to check their target .NET version and build references for csproj so perhaps we could just extend this with the optimization check?

But I am not sure for the default behaviour. Personally I think that we should throw exception and stop. @AndreyAkinshin @mattwarren what do you think?

[AndreyAkinshin]

I think, we should throw an exception with a message like "if you really want to run it in DEBUG, do the following ..."

[ig-sinicyn]

@AndreyAkinshin

Also, should the runner throw the exception if the code is running under debugger?
*If you wonder how do you debug the benchmark, there's VS extension for child process debugging.

[AndreyAkinshin]

Don't sure about an exception. It would be cool to add a warning.
I think, if somebody try to debug a child benchmark process, then he should understand what he is doing.

[ig-sinicyn]

@AndreyAkinshin
Looks like I've asked it wrong:)

Ok, one more time (sorry:))
You're proposing that the exception for debug builds should be thrown by default.

My question is, should the exception be thrown when the code is being run under debugger?

[goldshtn]

The question really should be whether JIT optimizations are enabled, because that's what makes most of the difference between optimized and non-optimized builds. And, by default, there won't be JIT optimizations when running under a debugger.

[ig-sinicyn]

@goldshtn

Agreed. Failing even if debugger attached makes the debugging slightly harder, but I don't think that this is a real issue.

All in all, the code from benchmark always can be run (and debugged) directly, without using the BenchmarkRunner.

[adamsitnik]

Ok. I have implemented this feature. How it works:

• it is a standalone analyser (JitOptimizationsAnalyser)
• it is enabled by default
• if you want to disable it you just need to specify a Config class that does not inherit from DefaultConfig
• if you want to enable it in your custom Config class you just put Add(Analyzers.JitOptimizationsAnalyser.Instance);
• I take all assemblies referenced by assembly that defines benchmark and for each of these I look for DebuggableAttribute and read IsJITOptimizerDisabled flag. If the flag is set to false I display warning
• currently only Classic and Dnx runtimes are supported, Core is not (due to lack of Assembly.Load method and lack of any flags for DebuggableAttribute)

@ig-sinicyn could please let me know if this satisfies you as a user?

[ig-sinicyn]

@adamsitnik

If the flag is set to false I display warning

Thanks!
There're two drawbacks, however.

1. The check does not throw and it's hard to detect that it was failed. Especially in case when the benchmark is run as unit test from VS.
   There's currently no way to transform benchmark warnings into NUnit warnings, as the first ones are not exposed as member of the Summary class. And xUnit (AFAIK) does not support warning assertions at all.
2. The check is performed after the test is run. Again, this makes it hard to notice that something went wrong as the benchmark run could take a long time.

[adamsitnik]

@ig-sinicyn Thanks for very usefull comments. I will add Warnings to summary and somehow extend IAnalyser to allow prevalidation and post verification

[ig-sinicyn]

@adamsitnik great news!

And thanks for exposing the warnings! I'll try to pass these as NUnit warnings when done. There's no documented way to do this, however the NUnit's IgnoreException is threated as a warning.

[adamsitnik]

@ig-sinicyn @AndreyAkinshin @mattwarren

I have extended our current features with new thing: Validators

public interface IValidator
{
    IEnumerable<IValidationError> Validate(IList<Benchmark> benchmarks);
}

public interface IValidationError
{
    bool IsCritical { get; }
    string Message { get; } 
    Benchmark Benchmark { get; }
}

Validators are executed before running benchmarks. There is only one mandatory validator: BaselineValidator (checks wether class does not have more than one Baseline = true method). Summary has been extended and contains list of found errors. If any of the errors is critical, then none of the Benchmarks is executed and export is also ommited.

@ig-sinicyn In your case if you would like to fail on error when any of the dlls that defines benchmark was not optimized you can use Add(JitOptimizationsValidator.FailOnError); in your config. Please let me know if it fixes the problem

[ig-sinicyn]

@adamsitnik Wow! That's much better than my original proposal, thumbs up.

Please let me know if it fixes the problem

It definitely does:)