Skip to content
This repository has been archived by the owner on Oct 31, 2023. It is now read-only.

limit benchmark-based usage factors #4803

Closed
wants to merge 1 commit into from
Closed

Conversation

mbenke
Copy link
Contributor

@mbenke mbenke commented Oct 18, 2019

Sometimes usage benchmark results can get misreported. This PR introduces sanity checks for usage factors based on these benchmark results, limiting them to the [0.25,1.5] range.

Additionally, we prevent dividing by 0 (or other very small value of the usage benchmark).

@codecov
Copy link

codecov bot commented Oct 18, 2019

Codecov Report

Merging #4803 into develop will decrease coverage by 0.05%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #4803      +/-   ##
===========================================
- Coverage    89.41%   89.35%   -0.06%     
===========================================
  Files          226      216      -10     
  Lines        20529    20103     -426     
===========================================
- Hits         18355    17964     -391     
+ Misses        2174     2139      -35

usage_factor = 1.0

# Sanity check against misreported benchmarks
usage_factor = min(max(usage_factor, 0.25), 1.5)
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we assuming that the usage cannot be farther away than that from our own? can't a really slow or really fast CPU have such - out of range - results?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can, but if it is, it will be caught by the usage factor adjustment. It may make sense to widen the limits though.

@mbenke
Copy link
Contributor Author

mbenke commented Oct 24, 2019

Abandoned in favour of #4827, which is a version of this PR rebased to develop after mewrging #4802

@mbenke mbenke closed this Oct 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants