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

ParamsSource #350

Closed
AndreyAkinshin opened this Issue Jan 20, 2017 · 9 comments

Comments

Projects
None yet
4 participants
@AndreyAkinshin
Member

AndreyAkinshin commented Jan 20, 2017

I suggest to add an ability to generate source for Params via additional methods. An example:

[ParamsSource(nameof(NSource))]
public int N;

public static IEnumerable<int> NSource()
{
    for (int i = 1; i <= 1024; i *= 2)
        yield return i;
}

The ParamsSource approach should also resolves #37, #256, #317.
@mattwarren, @adamsitnik, what do you think? Is the ParamsSource name is good enough for this attribute or we should choose another name?

@adamsitnik

This comment has been minimized.

Show comment
Hide comment
@adamsitnik

adamsitnik Jan 20, 2017

Member

I like the approach!

Member

adamsitnik commented Jan 20, 2017

I like the approach!

@mattwarren

This comment has been minimized.

Show comment
Hide comment
@mattwarren

mattwarren Jan 20, 2017

Contributor

Same here, this looks like a nice extension to the existing way of doing Params.

Is the ParamsSource name is good enough for this attribute or we should choose another name?

The only other one I can think of is ParamsGenerator, not sure if it's better though?

Contributor

mattwarren commented Jan 20, 2017

Same here, this looks like a nice extension to the existing way of doing Params.

Is the ParamsSource name is good enough for this attribute or we should choose another name?

The only other one I can think of is ParamsGenerator, not sure if it's better though?

@HenrikPoulsen

This comment has been minimized.

Show comment
Hide comment
@HenrikPoulsen

HenrikPoulsen Oct 16, 2017

Something to consider here is cases where the ParamSource method depends on things outside of itself that needs to be setup.
Say for example in GlobalSetup you have a IoC container, where you want your ParamsSource to use this container to resolve up all items of a particular interface and yield these.
Today though GlobalSetup is called after all the parameter values have been evaluated, so it would not be possible for the ParamsSource to depend on the output of GlobalSetup.

In my case we have a little tool that validates that code snippets follow a set of rules we have for our code base. Each validation has it's own class implementing our IRule interface.
So we want it to just automatically start running benchmarks on each new implementation of this interface as time goes along.
As it stands now we have to do this:

// This list is much longer in reality and we expect to have hundreds of rules over time
[Params(
    typeof(SomeRule),
    typeof(AnotherRule),
    typeof(ThirdRule))]
public Type ruleType;

public IRule rule;

[GlobalSetup]
public void Setup()
{
    // container setup and other stuff up here
    // ...  
    rule = container.Resolve(ruleType) as IRule;
}

[Benchmark]
public int Validate()
{
    return rule.Validate();
}

This way is no fun to maintain since we need to explicitly add every new rule added to the benchmark.
Now we essentially have to write a unit test which reflects up our benchmarks and checks the attribute against all the rules available in container so it fails if we forgot something. Far from ideal.

It would be nice if it could be something like this instead:

[ParamsSource("GetRules")]
public IRule rule;

public IEnumerable<IRule> GetRules()
{
    foreach(var rule in container.ResolveMany<IRule>())
    {
        yield return rule;
    }
}

[GlobalSetup]
public void Setup()
{
    // container setup and other stuff up here as usual
    // ...  
}

[Benchmark]
public int Validate()
{
    return rule.Validate();
}

Then we wouldn't need to manually maintain the attribute and can get rid of the ugly unit test.
But this would mean that there must be some way of calling GlobalSetup and GlobalCleanup without evaluating the ParamsSource values first (since in this case the container would not be instantiated yet).

I hope that makes sense.

Also is this one something that could be up for grabs by others? I could be tempted to spend my 20% time at work for a while on this if that were the case.

HenrikPoulsen commented Oct 16, 2017

Something to consider here is cases where the ParamSource method depends on things outside of itself that needs to be setup.
Say for example in GlobalSetup you have a IoC container, where you want your ParamsSource to use this container to resolve up all items of a particular interface and yield these.
Today though GlobalSetup is called after all the parameter values have been evaluated, so it would not be possible for the ParamsSource to depend on the output of GlobalSetup.

In my case we have a little tool that validates that code snippets follow a set of rules we have for our code base. Each validation has it's own class implementing our IRule interface.
So we want it to just automatically start running benchmarks on each new implementation of this interface as time goes along.
As it stands now we have to do this:

// This list is much longer in reality and we expect to have hundreds of rules over time
[Params(
    typeof(SomeRule),
    typeof(AnotherRule),
    typeof(ThirdRule))]
public Type ruleType;

public IRule rule;

[GlobalSetup]
public void Setup()
{
    // container setup and other stuff up here
    // ...  
    rule = container.Resolve(ruleType) as IRule;
}

[Benchmark]
public int Validate()
{
    return rule.Validate();
}

This way is no fun to maintain since we need to explicitly add every new rule added to the benchmark.
Now we essentially have to write a unit test which reflects up our benchmarks and checks the attribute against all the rules available in container so it fails if we forgot something. Far from ideal.

It would be nice if it could be something like this instead:

[ParamsSource("GetRules")]
public IRule rule;

public IEnumerable<IRule> GetRules()
{
    foreach(var rule in container.ResolveMany<IRule>())
    {
        yield return rule;
    }
}

[GlobalSetup]
public void Setup()
{
    // container setup and other stuff up here as usual
    // ...  
}

[Benchmark]
public int Validate()
{
    return rule.Validate();
}

Then we wouldn't need to manually maintain the attribute and can get rid of the ugly unit test.
But this would mean that there must be some way of calling GlobalSetup and GlobalCleanup without evaluating the ParamsSource values first (since in this case the container would not be instantiated yet).

I hope that makes sense.

Also is this one something that could be up for grabs by others? I could be tempted to spend my 20% time at work for a while on this if that were the case.

@adamsitnik

This comment has been minimized.

Show comment
Hide comment
@adamsitnik

adamsitnik Oct 17, 2017

Member

Also is this one something that could be up for grabs by others?

@HenrikPoulsen sure! Please let me know if you need any help!

Member

adamsitnik commented Oct 17, 2017

Also is this one something that could be up for grabs by others?

@HenrikPoulsen sure! Please let me know if you need any help!

@HenrikPoulsen

This comment has been minimized.

Show comment
Hide comment
@HenrikPoulsen

HenrikPoulsen Oct 17, 2017

@adamsitnik Cool. I will look at it then.
What would be nice to know is if you have any preferences for how you would want the scenario I mentioned in the previous post to be solved (if you want to support it at all).
An extra set of attributes which run before param evaluations for setup that doesn't depend on the params for example?

HenrikPoulsen commented Oct 17, 2017

@adamsitnik Cool. I will look at it then.
What would be nice to know is if you have any preferences for how you would want the scenario I mentioned in the previous post to be solved (if you want to support it at all).
An extra set of attributes which run before param evaluations for setup that doesn't depend on the params for example?

@adamsitnik

This comment has been minimized.

Show comment
Hide comment
@adamsitnik

adamsitnik Oct 21, 2017

Member

I had some spare time today and decided to implement it ;) Docs

public class IntroParamsSource
{
    [ParamsSource(nameof(ValuesForA))]
    public int A { get; set; } // property with public setter

    [ParamsSource(nameof(ValuesForB))]
    public int B; // public field

    public IEnumerable<int> ValuesForA => new[] { 100, 200 }; // public property

    public static IEnumerable<int> ValuesForB() => new[] { 10, 20 }; // public static method

    [Benchmark]
    public void Benchmark() => Thread.Sleep(A + B + 5);
}

@HenrikPoulsen I have reused the code we had for Params. So it works in the same way (order of method execution)

The problem we have is that the values have to be "serializable" because we generate and build a new project for every benchmark. So this is why you still have to return an collection of Type and build the given object at runtime in your [GlobalSetup].

I don't like this limitation, but currently I don't have any good idea how to solve it (at some point of time we generate C# file with T property = value and the value needs to be "code serializable")

Member

adamsitnik commented Oct 21, 2017

I had some spare time today and decided to implement it ;) Docs

public class IntroParamsSource
{
    [ParamsSource(nameof(ValuesForA))]
    public int A { get; set; } // property with public setter

    [ParamsSource(nameof(ValuesForB))]
    public int B; // public field

    public IEnumerable<int> ValuesForA => new[] { 100, 200 }; // public property

    public static IEnumerable<int> ValuesForB() => new[] { 10, 20 }; // public static method

    [Benchmark]
    public void Benchmark() => Thread.Sleep(A + B + 5);
}

@HenrikPoulsen I have reused the code we had for Params. So it works in the same way (order of method execution)

The problem we have is that the values have to be "serializable" because we generate and build a new project for every benchmark. So this is why you still have to return an collection of Type and build the given object at runtime in your [GlobalSetup].

I don't like this limitation, but currently I don't have any good idea how to solve it (at some point of time we generate C# file with T property = value and the value needs to be "code serializable")

@adamsitnik adamsitnik added this to the v0.10.10 milestone Oct 21, 2017

@adamsitnik

This comment has been minimized.

Show comment
Hide comment
@adamsitnik

adamsitnik Oct 22, 2017

Member

@HenrikPoulsen I created new thing, called IParam for non-compile time constants. Please take a look #571

Member

adamsitnik commented Oct 22, 2017

@HenrikPoulsen I created new thing, called IParam for non-compile time constants. Please take a look #571

@HenrikPoulsen

This comment has been minimized.

Show comment
Hide comment
@HenrikPoulsen

HenrikPoulsen Oct 30, 2017

@adamsitnik I played around with IParam today and it covers all my needs. Thanks!

HenrikPoulsen commented Oct 30, 2017

@adamsitnik I played around with IParam today and it covers all my needs. Thanks!

@adamsitnik

This comment has been minimized.

Show comment
Hide comment
@adamsitnik

adamsitnik Oct 30, 2017

Member

@HenrikPoulsen great, thanks for the feedback!

Member

adamsitnik commented Oct 30, 2017

@HenrikPoulsen great, thanks for the feedback!

alinasmirnova added a commit to alinasmirnova/BenchmarkDotNet that referenced this issue Sep 22, 2018

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