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

fix: use TIME_FUDGE_FACTOR rather than rounding by decimal digits #881

Merged
merged 2 commits into from
Sep 23, 2020

Conversation

brandonocasey
Copy link
Contributor

@brandonocasey brandonocasey commented Jun 25, 2020

Description

Currently we ran into an issue with roundSignificantDigit because it is rounding integer whole numbers when it seems like it was only made to round floats. This can cause us to download multiple duplicate segments on slow connections. Instead I propose that we only round by TIME_FUDGE_FACTOR here. This will prevent us from having to do any rounding at all.

Why the current solution is bad:

Rounding a value of 2.111111 to 2.111112 is less than 1 milliseconds a very small change but rounding from 2.1 to 2.2 is 100ms. Both are possible with the current code.

New solution

All numbers are rounded up or down by 33 milliseconds

Possible Solutions:

  1. Cap the rounding at TIME_FUDGE_FACTOR for all numbers, but round less for longer floats. Not sure this one really makes sense as longer floats are not necessarily more accurate.
  2. Don't round for numbers less than 1 decimal digits, effectively caps rounding at 99ms, which is still very high.
  3. Don't round for numbers less than 2 decimal digits, effectively caps rounding at 9ms, which might not be enough? It seems to have been working until now, or we didn't notice that it wasn't

gkatsev
gkatsev previously approved these changes Jun 25, 2020
@gkatsev gkatsev added this to the 2.2 milestone Jul 8, 2020
@stale
Copy link

stale bot commented Sep 6, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the outdated label Sep 6, 2020
@gkatsev gkatsev removed the outdated label Sep 8, 2020
@gkatsev
Copy link
Member

gkatsev commented Sep 10, 2020

Since this is a bug fix and helps us request less segments, I think we can just bring it in.

Copy link
Contributor

@gesinger gesinger left a comment

Choose a reason for hiding this comment

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

Looks good, but would be good to have a test showing the behavior.

@gkatsev gkatsev merged commit 7eb112d into main Sep 23, 2020
@gkatsev gkatsev deleted the fix/bad-estimated-rounding branch September 23, 2020 19:19
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