Create summary signature hash for benchmark tests without suite-level value. #7889
Conversation
… value. This patch creates a summary signature hash for benchmark tests without a suite-level value to prepare for moving away from using a suite-level value. Instead, the suite-level value will be stored as a subtest with replicates. With this change, removing the value from a benchmark won't reset it's signature anymore.
|
@beatrice-acasandrei I'm looking at starting to remove some suite-level values for benchmark tests (speedometer3 atm), but an issue we're hitting is that removing the value resets the signature/series for the subtests. What do you think about this approach for resolving that issue? An alternative is we remove the summary signature hash from the subtest signature creation but this would reset all of the existing signatures that already use a summary signature hash. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7889 +/- ##
==========================================
- Coverage 77.06% 77.05% -0.02%
==========================================
Files 541 541
Lines 26794 26795 +1
Branches 3364 3364
==========================================
- Hits 20650 20648 -2
- Misses 5977 5980 +3
Partials 167 167 ☔ View full report in Codecov by Sentry. |
| if suite.get('value') is not None: | ||
| # If we have a summary value, create or get its signature by all its subtest | ||
| # properties. For benchmarks, we need to get a summary signature hash so that | ||
| # the series doesn't reset when we start moving away from the summary value, |
There was a problem hiding this comment.
I haven't had tthe chance to work in this area yet, so I have some questions. What do you mean by resetting? Having a different series with another signature, instead of belonging to the initial series with the initial signature?
There was a problem hiding this comment.
Yes that's right, if the suite value gets removed, the benchmark data for the subtests would become a new series which is not something we would want. It'll also prevent us from migrating the "overall" suite-level score into a subtest.
| # properties. For benchmarks, we need to get a summary signature hash so that | ||
| # the series doesn't reset when we start moving away from the summary value, | ||
| # and start including that value as it's own subtest with replicates. | ||
| if suite.get('value') is not None or suite.get("type") == "benchmark": |
There was a problem hiding this comment.
I am wondering if it's possible to have summary signature hash for a benchmark test with a suite-level value. After removing the value from a benchmark isn't the function _get_signature_hash(summary_properties) going to generate a different hash (because of the missing value)? and as a consequence another signature will be created?
There was a problem hiding this comment.
No it won't create an additional one because the value is never added to the summary_properties dict so the hash isn't based on it.
| if suite_datum.should_mark_as_multi_commit(is_multi_commit, datum_created): | ||
| # keep a register with all multi commit perf data | ||
| MultiCommitDatum.objects.create(perf_datum=suite_datum) | ||
| if suite.get("value", None) is not None: |
There was a problem hiding this comment.
From this I understand that for benchmark tests without a suite-level value there won't have performance datums?
Are the subtests affected in the same way?
There was a problem hiding this comment.
That's correct, for benchmarks without a suite-level value, there won't be a suite-level PerformanceDatum created for it. However, the subtests will still have a PerformanceDatum created for them since that happens on line 235, outside of this if block.
|
Hi @gmierz! Because I haven't worked myself on this part of Treeherder I have some questions, hopefully the questions are not that bad. Thanks. |
This patch creates a summary signature hash for benchmark tests without a suite-level value to prepare for moving away from using a suite-level value. Instead, the suite-level value will be stored as a subtest with replicates. With this change, removing the value from a benchmark won't reset it's signature anymore.