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

Make MaxIterationCount configurable, keep current value as default #763

Closed
adamsitnik opened this issue May 25, 2018 · 8 comments
Closed

Comments

@adamsitnik
Copy link
Member

As of today, the MaxIterationCount in EngineTargetStage is constant and equal 100.

It would be nice to have a possibility to configure it.

Reasoning: I have some multimodal benchmarks and they run 100 iterations as of today. It would be nice if I could set it for my purpose to let's say 15 and have the results ready sooner.

@adamsitnik adamsitnik self-assigned this May 27, 2018
@adamsitnik adamsitnik added this to the v0.11.0 milestone May 27, 2018
adamsitnik added a commit that referenced this issue May 27, 2018
…MultimodalDistributionAnalyzer is not going to work, #763
@adamsitnik
Copy link
Member Author

@AndreyAkinshin I also added hint for case when the IterationCount < 15 and MultimodalDistributionAnalyzer is not going to work

image

@AndreyAkinshin
Copy link
Member

@adamsitnik, I don't think that we should display such hint. We provide too many features, some of them can be disabled in some cases, and it's OK. We shouldn't print a hint for each feature which is disabled for the current benchmark; we should keep the output as small as possible and display information which is essential for the ongoing performance investigation.

@adamsitnik
Copy link
Member Author

@AndreyAkinshin I agree with you that we should not print too many hints, however, I think that this particular hint is going to be helpful.

Typically, when the benchmark has multimodal distribution, our engine runs it the default max iteration count (100). This is when some users might want to use the new settings - MaxTargetIterationCount. However, if they set it to < 15, the awesome MultimodalDistributionAnalyzer will be silent. I think that it's important to warn the users about this.

When I was testing MaxTargetIterationCount and I set it to 10, I hit this problem and I had no idea why I don't get any info from MultimodalDistributionAnalyzer.

To make sure that we print this hint only once I implemented IEquatable for Conclusion and used Distinct to prevent from duplicates. So this hint will be printed once only for users who play with MaxTargetIterationCount

@AndreyAkinshin does it convince you?

@AndreyAkinshin
Copy link
Member

@adamsitnik

  1. MultimodalDistributionAnalyzer is not the only feature which doesn't work properly in this case. If you have a multimodal distribution with custom MaxTargetIterationCount<15, all columns like Scaled, Rank, StdDev will probably show useless random values. If we want to warn people that they can get incorrect results in these cases, you should print a general warning instead of a MultimodalDistributionAnalyzer is fixed.
  2. By default, everything works fine. MaxTargetIterationCount is designed for people who understand what's going on with their benchmarks. You should check the distribution first (probably, we can add it here and there in the docs). If you know that the distribution looks OK, you can decrease MaxTargetIterationCount to optimize the total benchmark suite time. In such situations, it will be very annoying to look at the useless warning constantly.

@AndreyAkinshin
Copy link
Member

@adamsitnik ?

@adamsitnik
Copy link
Member Author

@AndreyAkinshin I think that we have two options:

  1. I can remove MultimodalDistributionAnalyzer
  2. You can use your knowledge about statistics and our engine and create new analyzer which will give some good warnings when people do stupid things with the config.

@AndreyAkinshin what do you think?

@AndreyAkinshin
Copy link
Member

@adamsitnik, I like both options. =)
Let's remove the MultimodalDistributionAnalyzer in current version because it's pretty annoying. Meanwhile, I will work on better analyzer which will print warning only if it makes sense.

@adamsitnik
Copy link
Member Author

👍 I am going to remove it right now

alinasmirnova pushed a commit to alinasmirnova/BenchmarkDotNet that referenced this issue Sep 22, 2018
alinasmirnova pushed a commit to alinasmirnova/BenchmarkDotNet that referenced this issue Sep 22, 2018
…MultimodalDistributionAnalyzer is not going to work, dotnet#763
alinasmirnova pushed 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
Projects
None yet
Development

No branches or pull requests

2 participants