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

Calculate upload rate #1029

Merged
merged 12 commits into from
Dec 1, 2023
Merged

Calculate upload rate #1029

merged 12 commits into from
Dec 1, 2023

Conversation

gilest
Copy link
Collaborator

@gilest gilest commented Dec 1, 2023

Continuation of work by @RobbieTheWagner in #1015

Methodology

  • Record rate data against each file as upload progress events occur
  • Calculate weighted moving average of rate per-file as File.rate property
  • Sum all uploading files as Queue.rate property

All rates are in bytes/ms.

Demo

Recent improvements to the demo page make this much easier to validate and test.

Screen.Recording.2023-12-01.at.3.07.05.PM.mov

@RobbieTheWagner
Copy link
Member

@gilest this is looking great! Quick question, should we be throttling this calculation instead of updating it on every event?

// Divide by elapsed time to get bytes per millisecond
const rate = bytesTransferredSinceLastUpdate / timeElapsedSinceLastUpdate;

file.rates = [...file.rates, rate];
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would it be more performant to use a TrackedArray?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure. I say leave it unless we have problems.

@@ -51,6 +52,11 @@ export class UploadFile {
this.#name = value;
}

/** The current speed in ms that it takes to upload one byte */
get rate() {
return estimatedRate(this.rates);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Work done in plain functions for unit-testing purposes.

@gilest
Copy link
Collaborator Author

gilest commented Dec 1, 2023

@gilest this is looking great! Quick question, should we be throttling this calculation instead of updating it on every event?

Interesting question 🤔

So each time a progress event occurs we update the following properties:

  • file.bytesWhenProgressLastUpdated
  • file.timestampWhenProgressLastUpdated
  • file.rates

Beyond that, no work is done. We'll only do a rate calculation if one of the rate getters is accessed. That's the beauty of lazy evaluation and derived state.

Obviously if a rate property is bound to a template or other reactive context then the rate calculation will run every time file.rates updates, but it only runs on 30 recorded rates per upload-in-progess. I doubt it's very computationally intensive.

So I don't think we need to do any throttling.

It's also worth noting that the demo simulation sends 10 progress events per second. Probably it's a bit less often in real-world HTTP uploads.

@gilest
Copy link
Collaborator Author

gilest commented Dec 1, 2023

Profiling with 6x CPU slowdown the estimatedRate function takes ~1ms at most

image

Comparison to date-fns intervalToDuration in the same period
image

@RobbieTheWagner
Copy link
Member

@gilest yeah seems performant enough and we can always kick the tires a bit and see if we need to tweak it later.

Copy link
Member

@RobbieTheWagner RobbieTheWagner left a comment

Choose a reason for hiding this comment

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

This looks great to me. Thanks for taking it over the finish line!

@RobbieTheWagner RobbieTheWagner merged commit 348ff04 into master Dec 1, 2023
15 checks passed
@RobbieTheWagner RobbieTheWagner deleted the feat/upload-rate branch December 1, 2023 19:45
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