Benchmarkdotnet: [Feature request] Unseal some types

[ig-sinicyn]

Hi!

I'm going to resurrect my pet project (CodeJam.PerfTests, a test runner for BDN) and I'm blocked by some breaking changes in BDN. To name a few: the ImmutableConfig become sealed, MemoryDiagnose have no public constructor an so on.

Is it ok, if I'll prepare a PR that unseals/makes some members public to unblock creation of custom BDN runners? Currently I'm not ready to list all members that will be changed as my work is still in progress:)

[adamsitnik]

Hello @ig-sinicyn

We are ok with making more things public/unsealed as long as it does not break us on our side and helps with the design of anything build on top of BDN

MemoryDiagnose have no public constructor

https://github.com/dotnet/BenchmarkDotNet/blob/b5d3246d466c7c3086a68d52b0c80aab97338c26/src/BenchmarkDotNet/Diagnosers/MemoryDiagnoser.cs#L20

The main reason was to have a single instance of MemoryDiagnoser to allow for reference equality checks. An example:

https://github.com/dotnet/BenchmarkDotNet/blob/3bd18fc1fd84c70eedde1ced821df8777c710ec2/src/BenchmarkDotNet/Configs/ImmutableConfig.cs#L95

Would need to be changed to:

diagnosers.OfType<MemoryDiagnoser>().Any();

Why would you like to have a public ctor for it?

ImmutableConfig become sealed

This particular type does not expose any virtual methods or properties and I really want to make sure it's Immutable so I would prefere to keep it sealed.

I guess that you can't create an instance of it becasue it has an internal ctor? Would making the ctor public be enough?

https://github.com/dotnet/BenchmarkDotNet/blob/3bd18fc1fd84c70eedde1ced821df8777c710ec2/src/BenchmarkDotNet/Configs/ImmutableConfig.cs#L35

[ig-sinicyn]

The main reason was to have a single instance of MemoryDiagnoser to allow for reference equality checks. An example:

Ok, got it. Looks like I do not need custom memory diagnoser at all.
Previous version did override the IDiagnoser.GetColumnProvider() method to hide default memory-related columns from the output. This method was gone now so it looks like I'll need to find another way to override diagnoser output.

This particular type does not expose any virtual methods or properties and I really want to make sure it's Immutable so I would prefere to keep it sealed.

Well, this one really blocking me. I'm using custom interface derived from the IConfig to pass additional options into analysers/diagnosers. And I can not pass the final config into BenchmarkRunner.Run() method without deriving it from the ImmutableConfig.

[adamsitnik]

This method was gone now so it looks like I'll need to find another way to override diagnoser output.

Some time ago we have introduced a new concept of Metric. Now every Diagnoser returns an enumerable of Metrics and the DefaultConfig uses DefaultColumnProviders

https://github.com/dotnet/BenchmarkDotNet/blob/5070af4fac0299d6f27ce02a71f9093cf780f3c5/src/BenchmarkDotNet/Columns/DefaultColumnProvider.cs#L17-L19

which contains MetricsColumnProvider which creates a new column of every metric reported by the used diagnosers

https://github.com/dotnet/BenchmarkDotNet/blob/5070af4fac0299d6f27ce02a71f9093cf780f3c5/src/BenchmarkDotNet/Columns/DefaultColumnProvider.cs#L83-L90

So to customize the diagnoser output you would have to use a different ColumnProvider and implement some custom MetricsColumnProvider that does the desired filtering.

[adamsitnik]

I'm using custom interface derived from the IConfig to pass additional options into analysers/diagnosers. And I can not pass the final config into BenchmarkRunner.Run() method without deriving it from the ImmutableConfig.

I see. Then we can unseal ImmutableConfig. But perhaps there is something in your additional options that would be useful in general and we should just extend the current implementation?

[ig-sinicyn]

But perhaps there is something in your additional options

I'm afraid, all options are specific for my scenarios. There are options for source annotations (sources are auto-annotated with attributes with expected metric values), list of custom metric providers and so on. I'll try to use a BDN metric providers but i'm currently not sure if this will work for my use cases.

[adamsitnik]

i'm currently not sure if this will work for my use cases.

The concept of metric providers is still quite new and we can extend it if needed. So far I added only the things I needed for existing use cases.

[adamsitnik]

Hi @ig-sinicyn

I am currently doing a big cleanup of old issues in the repo.

If you need some changes, please go ahead and send a PR.

For now, I am going to close this issue as there is nothing actionable on our side.

Thanks,
Adam