Fix Append_Span benchmark #2244
Conversation
…an empty string rename the benchmark so ReportingSystem does not detect another regression
| [Benchmark] | ||
| public StringBuilder Append_Span() | ||
| public StringBuilder Append_NonEmptySpan() |
There was a problem hiding this comment.
Do we really need to rename it? Every time there's a bug in a test we need to rename the test, and then we're left with some tests that just don't have any more data after their fixed data?
There was a problem hiding this comment.
It depends on how the reported time changes. In this case, if we keep the old name the automation is going to recognize it as a regression. We would need to have a possibility to hint the automation that "it's not a regression, it's a fix" but looking at how rarely it happens I don't think it's worth the investment.
There was a problem hiding this comment.
We would need to have a possibility to hint the automation that "it's not a regression, it's a fix" but looking at how rarely it happens I don't think it's worth the investment.
Or just ignore the "regression". Just as we do when a regression issue is filed, we look at it, and we say "we don't care about that one".
Treating benchmarks as immutable any time there's an issue in a benchmark means we're building up debt over time, of one form or another. For example, @DrewScoggins added an 's' to one of the benchmark class names for some reason he's no longer sure of 😄 (https://github.com/dotnet/performance/pull/2162/files#diff-11613ef856acfeabb3170ef1a34bd32199c75f5e9e8dc51e245b842286d66cc9R44), and now both the old and new ones show up forever more in the reports.
There was a problem hiding this comment.
Treating benchmarks as immutable any time there's an issue in a benchmark means we're building up debt over time, of one form or another.
I agree that it's not a perfect solution.
Or just ignore the "regression"
The automation files issues for multiple configurations and we would most likely end up with few false alarm issues. Also the people who triage the issues (@AndyAyersMS @kunalspathak @tannergooding @EgorBo) would need to know about the "fix".
@AndyAyersMS @kunalspathak @tannergooding @EgorBo what is your opinion on this?
There was a problem hiding this comment.
It is difficult for us (the triage team) to memorize all the issues that fall under this category and ignore the regression. In the past, we have suggested to @DrewScoggins to add an ability to annotate a benchmark from the historical data page or something. That way, we can see the annotation in future and take required actions.
There was a problem hiding this comment.
Does it need to fall on you to remember and ignore it? The issue can be filed if needed and then the team that goes to investigate can say "yeah, the benchmark changed". Is that significantly different from a product change that's by design known by the team making the change to impact a benchmark?
There was a problem hiding this comment.
the team that goes to investigate - do you mean the triage team or the one that is investigating the regression?
There was a problem hiding this comment.
the team that goes to investigate - do you mean the triage team or the one that is investigating the regression?
The latter, e.g. not you, me :)
There was a problem hiding this comment.
e.g. not you, me :)
in that case, I don't have any problem :)
There was a problem hiding this comment.
I think in general I am less worried about us finding the regression once, and more about people looking at the graph, and seeing a big jump and not remembering why it happened. Like @kunalspathak said there is work on the backlog to add annotations for stuff like this, but we don't really have an ETA for when we would have it.
All that to say, I don't think it will be that big a deal as we don't regularly look at every test result and it's full history.
related to dotnet/runtime#64751