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

Create summary signature hash for benchmark tests without suite-level value. #7889

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gmierz
Copy link
Collaborator

@gmierz gmierz commented Dec 2, 2023

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.

… 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.
@gmierz gmierz marked this pull request as draft December 2, 2023 21:16
@gmierz
Copy link
Collaborator Author

gmierz commented Dec 2, 2023

@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-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5a4df58) 77.06% compared to head (7208818) 77.05%.

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.
📢 Have feedback on the report? Share it here.

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,
Copy link
Collaborator

@beatrice-acasandrei beatrice-acasandrei Jan 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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":
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@beatrice-acasandrei
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants