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

Add validation warning for sealed classes containing benchmarks #2516

Open
JeppeSN opened this issue Feb 5, 2024 · 4 comments
Open

Add validation warning for sealed classes containing benchmarks #2516

JeppeSN opened this issue Feb 5, 2024 · 4 comments

Comments

@JeppeSN
Copy link

JeppeSN commented Feb 5, 2024

It is easy to create benchmarks, for example as shown in README.md:

// attributes here
public class Md5VsSha256
{
  // setups and benchmarks here
}

However, if I choose to seal my class, as in:

public sealed class Md5VsSha256

then if I use BenchmarkSwitcher.FromAssemblies(...).Run(...) to run my benchmarks, then the class is no longer discovered.

Is there any technical reason for this behavior?

It seems that if you go to src/BenchmarkDotNet/Extensions/ReflectionExtensions.cs line 159 and remove the .IsSealed criterion, then this should work?

In the project I am working in, we prefer to make all types sealed unless they are specifically designed to be base classes, and this practice improves performance in general, so it is a shame if classes containing benchmarks cannot also be sealed.

@timcassell
Copy link
Collaborator

Is there any technical reason for this behavior?

Your benchmark class cannot be sealed because BDN generates a class that inherits it in order to actually run the benchmarks.

public unsafe class Runnable_$ID$ : global::$WorkloadTypeName$

@timcassell timcassell closed this as not planned Won't fix, can't repro, duplicate, stale Feb 5, 2024
@timcassell
Copy link
Collaborator

Perhaps it would be better to display a warning rather than ignore it silently.

@timcassell timcassell reopened this Feb 5, 2024
@timcassell timcassell changed the title Allow classes containing benchmarks to be sealed Add validation warning for sealed classes containing benchmarks Feb 5, 2024
@JeppeSN
Copy link
Author

JeppeSN commented Feb 6, 2024

@timcassell, All right, my question was based on general ignorance, and this is clearly much more complex than I had imagined.

There are likely a variety of different ways BenchmarkDotNet could be used. In our use, it seems the template was not used. But then I did not actually have the attribute [SimpleJob] on my class (called Md5VsSha256 in the example).

After having seen the template Templates/BenchmarkType.txt, if I do include [SimpleJob], and also introduce an evil member like:

public int BenchmarkDotNet { get; set; }

to make sure the code in the template will not compile, I do see output indicating the compile-time error, so I can see that a template is used then. However (removing the evil BenchmarkDotNet property), if I make a check in the constructor:

public Md5VsSha256()
{
    if (GetType() != typeof(Md5VsSha256))
    {
        throw new InvalidOperationException("See, template type is instantiated.");
    }
}

then I do not see any indication that the line 6 from the template, BenchmarkDotNet.Autogenerated.Runnable_$ID$ instance = new BenchmarkDotNet.Autogenerated.Runnable_$ID$(); // do NOT change name "instance" (used in SmartParamameter), is run, in my use.

Anyway, this is probably not worth pursuing further. Thank you for your answer.

@adamsitnik
Copy link
Member

We should reconsider writing the code analyzers for BDN, similar to what xUnit provides.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants