PE-4892: refactor the progress update uploading files #1460
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- refactors the class moving the most important class at the top of the file - refactors the method to update the upload progress - removes a unused class
- Save the file on database when its upload finishes. - Introduces the `onCompleteTask` API on `UploadController`
remove noisy prints
|
Visit the preview URL for this PR (updated for commit 93023f1): https://ardrive-web--pr1460-pe-4892-refactor-the-gbbew1tv.web.app (expires Thu, 09 Nov 2023 18:59:05 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: a224ebaee2f0939e7665e7630e7d3d6cd7d0f8b0 |
refactor the total size calculation
karlprieb
reviewed
Nov 2, 2023
| @@ -708,7 +481,7 @@ class Worker { | |||
| wallet: wallet, | |||
| driveKey: task.encryptionKey, | |||
| onStartBundleCreation: () { | |||
| print('Creating bundle'); | |||
| // print('Creating bundle'); | |||
There was a problem hiding this comment.
can you remove this comment and the one below?
karlprieb
reviewed
Nov 2, 2023
web/index.html
Outdated
| @@ -19,6 +19,7 @@ | |||
| <script defer src="js/arconnect.js"></script> | |||
| <script defer src="js/is_document_focused.js"></script> | |||
| <script defer src="js/StreamSaver.min.js"></script> | |||
| <script defer src="https://unpkg.com/web-streams-polyfill/dist/polyfill.min.js"></script> | |||
There was a problem hiding this comment.
why are you adding polyfills?
matibat
reviewed
Nov 2, 2023
- fixes a memory leak on web uploads using Turbo - blocks the UI from update while uploading files, and updates in the end of the upload
karlprieb
approved these changes
Nov 2, 2023
matibat
approved these changes
Nov 2, 2023
| @@ -227,7 +227,7 @@ class UploadInProgressUsingNewUploader extends UploadState { | |||
| }); | |||
|
|
|||
| @override | |||
| List<Object?> get props => [progress, totalProgress, equatableBust]; | |||
| List<Object?> get props => [equatableBust]; | |||
There was a problem hiding this comment.
In previous tasks, I've had to add stuff to the props of some other state class for equality checks. Perhaps we should start leaving a comment explaining why these are being excluded if there's a reason. Just to avoid breaking this in the future.
thiagocarvalhodev
deleted the
PE-4892-refactor-the-progress-update-uploading-files
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This ticket will apply a few refactors to improve the upload progress performance. The main points on where the upload performance is impacted:
See Details & Requirements for more info.
Details & Requirements
Saving the files as they are uploaded individually
Currently, since the v2.22.0 (large uploads release) we are batching all the files for then, when the upload finishes, save them on the data database. With that approach, we have some problems:
For large files, the modal will get stuck while saving all the files without any feedback from the user
Large amount of memory usage when saving all files at the same time
Now, we will save the files as each one finalizes. For example, if the user is uploading 1000 files, we will call the database 1000 times, a call for each file as soon as that file finishes the upload.
It fixes an issue mentioned by our user. See notes section.
Algorithm for updating the progress
Refactor the UploadController and UploadCubit classes to improve the performance when updating the progress. It requires some refactors on the methods we use for updating the progress. For example, instead of iterating through the entire list of files to get the total size, start the total size at zero and increment as we add files to the upload. It’s very technical and the changes can be reviewed on the PR.
Memory usage
We found a memory leak in our upload process. The stream passed for the Dio client was not being closed (we thought Dio would handle that, but it did not). Now, we are properly closing the stream.
Important: This only happens on TURBO!
UI updates while uploading
Before the large upload feature, each time we save a file on the database we update the entire drive tree. It’s expensive as we need to regenerate the entire drive tree multiple times while uploading the files. Now, it will update the drive explorer only once when the upload finishes, either with success or failure.
--- Releases ---
Android release: https://appdistribution.firebase.google.com/testerapps/1:305132849030:android:6cf0cd5ec064fad3ffce07/releases/1gurqhnqemr78