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

Allow for benchmark categories composition using inheritance #2306

Closed
BladeWise opened this issue May 14, 2023 · 3 comments
Closed

Allow for benchmark categories composition using inheritance #2306

BladeWise opened this issue May 14, 2023 · 3 comments
Assignees
Milestone

Comments

@BladeWise
Copy link

I am writing some benchmarks over generic classes, and to do so I created base generic benchmark, something along these lines

[BenchmarkCategory("Number")]
public class MyBenchmark<T> where T : unmanaged, INumberBase<T>
{
    public T V1;
    public T V2;
    
    [BenchmarkCategory("Add")]
    public T Add()
    {
         return V1 + V2;
    }

    [BenchmarkCategory("Multiply")]
    public T Multiply()
    {
         return V1 * V2;
    }

   [GlobalSetup]
   public virtual void Setup()
   {
         V1 = T.One + T.One;
         V2 = T.One + T.One + T.One;
   }
}

Actual benchmarks are specialized over the generic type

[BenchmarkCategory("Float")]
public class MyFloatBenchmark : MyBenchmark<float>
{
    [BenchmarkCategory("Sqrt")]
    public float Sqrt()
    {
        return float.Sqrt(V1);
    }
}

The problem is that the category defined on the derived class is not assigned to the methods declared on the base class, because BenchmarkConverter only looks at the method declaring type. Moreover, the attribute on the derived class replaces the one defined on the base class, because the current resolution strategy ignores inherited attributes.

Is there a reason why method.ReflectedType is not used in this case and inherited attributes are explicitly ignored (both at the class and method level)?

So far, the only option is to replicate the categories on both the derived class and overridden method.

Would it be possible to add an extension point to the BenchmarkConverter (e.g., an ICategoryResolver or a CategoryResolver delegate) to plug-in a different categories discovery logic?

@AndreyAkinshin
Copy link
Member

@BladeWise Thanks for the feedback. In #2307, I have implemented the following:

  • Now categories are inherited by default. Since it's a breaking change, the old behavior can be restored using [CategoryDiscoverer(false)].
  • Now it's possible to override the category discovering strategy with any custom logic (see IntroCategoryDiscoverer and CategoriesTests in Introduce CategoryDiscoverer, fix #2306 #2307 for usage examples)

Does it solve your problem? Do you have any comments on the implementation?

@BladeWise
Copy link
Author

The PR solves the above issue perfectly, and the implementation is flexible enough to account for custom discovery strategies.

I am not sure if it would be worth to add an explicit configuration extension method to revert to the old behavior (or eventually opt-in to the new one), something like

public static ManualConfig WithCategoryDiscoverer(this IConfig config, bool inherit) => config.With(m => m.CategoryDiscoverer = new DefaultCategoryDiscoverer(inherit));

The CategoryDiscovererAttribute serves the same purpose, but an explicit extension method can be sometimes easier to discover.
Thanks a lot for the quick response and support.

@AndreyAkinshin
Copy link
Member

AndreyAkinshin commented May 14, 2023

but an explicit extension method can be sometimes easier to discover.

Well, with the current state of the PR, it's possible to do

config.WithCategoryDiscoverer(new DefaultCategoryDiscoverer(false))

I believe there is no need for an additional shortcut method.

The PR solves the above issue perfectly, and the implementation is flexible enough to account for custom discovery strategies.

Great, glad to hear that. Then I merge the PR.

Thanks a lot for the quick response and support.

You're welcome!

@AndreyAkinshin AndreyAkinshin added this to the v0.13.6 milestone May 14, 2023
@AndreyAkinshin AndreyAkinshin self-assigned this May 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants