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

Fastcheck for correctness of benchmark implementations #136

Closed
ig-sinicyn opened this issue Apr 3, 2016 · 6 comments · Fixed by #737
Closed

Fastcheck for correctness of benchmark implementations #136

ig-sinicyn opened this issue Apr 3, 2016 · 6 comments · Fixed by #737

Comments

@ig-sinicyn
Copy link
Contributor

Use case: prevent users from comparing methods that do different things.

It's very easy to leave bug unnoticed in one of the benchmark methods. I just did it and had to spent some time to find that one of the competitors was exiting in the middle.

Proposed solution is fragile, does not proofs that all code is correct but it's still better than nothing.
So, the proposal:

If benchmark method has a return value, collect it and check that all another non-void methods return the same value. The value serves as a checksum, so, if it differs benchmark should fail.
Ensuring that result of the benchmark is calculated correctly and that the result does not produce false positives is responsibility of the author of the benchmark.

I think that this has to be opt-in feature and it's enough to check the results only on first run.

@adamsitnik adamsitnik self-assigned this Apr 16, 2016
@adamsitnik
Copy link
Member

I have some idea, will try to implement it in form of a Validator that will be called before benchmark run.

@adamsitnik
Copy link
Member

@ig-sinicyn Some progress: I have implemented ExecutionValidator that checks whether it is possible to run all benchmarks first. If you do not want to continue execution if single benchmark fails validation you need to use ExecutionValidator.FailOnError

The feature is not done yet, I will have to store and compare the results. I need to consider collections as well, but I will most probably only derive CorrectnessValidator from ExecutionValidator .

@ig-sinicyn
Copy link
Contributor Author

@adamsitnik ok, thanks for it! :)

@mattwarren
Copy link
Contributor

BTW, after using BenchmarkDotNet @FransBouma also asked for this feature, see https://twitter.com/FransBouma/status/723818104501997568

So consider that another +1 for it.

@ig-sinicyn
Copy link
Contributor Author

@adamsitnik, @AndreyAkinshin

Upping. With #356 merged there's an API that can be easily extended with result reporting.

Initial idea is following: last invocation result should be passed as a member of RunResults and should be exposed via BenchmarkReport. I'd suggest to use Convert.ChangeType or TypeConverter.ConvertFromString(), both should work in .Net Core.

@AndreyAkinshin AndreyAkinshin self-assigned this Mar 3, 2017
@adamsitnik
Copy link
Member

@AndreyAkinshin I believe that we should not implement this feature. We should focus on performance related things, we don't have enough time to implement this one.

People should use unit test frameworks to check the correctness, it's not our responsibility.

@adamsitnik adamsitnik removed this from the v0.10.x milestone Sep 2, 2017
@adamsitnik adamsitnik removed the wontfix label May 3, 2018
@adamsitnik adamsitnik added this to the v0.11.0 milestone May 3, 2018
@adamsitnik adamsitnik reopened this May 3, 2018
adamsitnik pushed a commit that referenced this issue May 3, 2018
* Split ExecutionValidator

* Add ReturnValueValidator

* Add validator attributes

* Add custom equality comparer to ReturnValueValidator

* Improve exception messages in ExecutionValidator

* Don't output results multiple times in ReturnValueValidator
@adamsitnik adamsitnik changed the title [Feature request] Fastcheck for correctness of benchmark implementations? Fastcheck for correctness of benchmark implementations May 3, 2018
alinasmirnova pushed a commit to alinasmirnova/BenchmarkDotNet that referenced this issue Sep 22, 2018
* Split ExecutionValidator

* Add ReturnValueValidator

* Add validator attributes

* Add custom equality comparer to ReturnValueValidator

* Improve exception messages in ExecutionValidator

* Don't output results multiple times in ReturnValueValidator
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants