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

Add rate to files and queue #1015

Closed
wants to merge 2 commits into from
Closed

Conversation

RobbieTheWagner
Copy link
Member

This is a first attempt at allowing us to calculate the rate at which files are uploading, which can be used to calculated estimated time remaining for downloads.

@RobbieTheWagner
Copy link
Member Author

@gilest @mkszepp I have rebased this with the latest changes in master and attempted to add estimated time remaining to the website demo. However, the values seem off. I'm not sure if we need to update the simulateUpload method or if there are flaws in my implementation. Would you be able to take a look?

@gilest
Copy link
Collaborator

gilest commented Nov 17, 2023

Hey @RobbieTheWagner this is cool!

attempted to add estimated time remaining to the website demo. However, the values seem off. I'm not sure if we need to update the simulateUpload method or if there are flaws in my implementation.

The website demo bypasses the network and sets file state directly. This means anything that happens within the internal upload progress handlers would need to also be done in simulateUpload.

I've merged some improvements to the demo page so that simulateUpload also uses the internal upload progress handlers in #1022

Also included some other goodies ✨

Implementation

When investigating the "live" behaviour of this feature (using the demo page) I noticed there are some timing issues. I think this is due to how we store and read a file's upload rate.

For example when using the demo page the rate is reported as a numeric value and then very soon after as 0 – I think this is due to the use of next scheduling an update in the next runloop.

As far as I can tell, when file.loaded and file.bytesWhenProgressLastUpdated are set together the value of rate will be 0.

Suggestion

Even though it will make this package larger I think we might need to make slightly more involved recordings of upload rate.

For example we may want to record a series of rates (how long each chunk of progress took) and report the rate as a "weighted moving average" where the most recent chunk rate has the highest weighting. I've seen this used before to estimate bandwidth in video streaming.

I'm going to play around a bit more with the demo UI and your changes to see what I can come up with

@gilest
Copy link
Collaborator

gilest commented Nov 17, 2023

I did an attempt at keeping multiple rates w/ an average for each file. Also means you know the historical rate for each file, too.

Hope you don't mind 😅

In branch add-rate-calculation-rebase

Demo:

Screen.Recording.2023-11-17.at.5.50.06.PM.mov

Note I think the time estimated helper might be a bit off. But when I did some back-of-the-napkin math I think the bytes/ms value matched the synthetic upload rate.

Eg at "Fast 3G" setting the rate is 83 bytes/ms = 83,000 bytes/s

image

And Fast 3G is configured as 0.675 Mbps, so pretty close

@RobbieTheWagner
Copy link
Member Author

@gilest everything you've said sounds way smarter than my approach, so I would be in favor of using your branch. Thanks for working on this! I do agree the estimated time seems off though, but if you think the bytes/ms is pretty close to correct, I think that is the important thing for shipping this.

@gilest gilest mentioned this pull request Dec 1, 2023
@RobbieTheWagner
Copy link
Member Author

Superseded by #1029

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

Successfully merging this pull request may close these issues.

2 participants