-
Notifications
You must be signed in to change notification settings - Fork 107
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
Update progress bar only once every 100ms #717
Conversation
Reviewer's Guide by SourceryThe progress bar update was changed to only update every 100ms to reduce CPU usage. Sequence diagram for throttled progress bar updatessequenceDiagram
participant Client
participant ProgressBar
loop Every data chunk
Client->>Client: Read 1024 bytes
Client->>Client: Check time elapsed
alt >100ms since last update
Client->>ProgressBar: Update progress
Note right of ProgressBar: Progress updated
else <100ms since last update
Note right of Client: Skip update
end
end
State diagram for progress bar update logicstateDiagram-v2
[*] --> ReadingData
ReadingData --> CheckTime: Read 1024 bytes
CheckTime --> UpdateProgress: >100ms elapsed
CheckTime --> ReadingData: <100ms elapsed
UpdateProgress --> ReadingData
ReadingData --> [*]: No more data
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @ericcurtin - I've reviewed your changes - here's some feedback:
Overall Comments:
- The progress tracking needs to accumulate all bytes downloaded since the last update, not just the last chunk. Currently it only passes the most recent chunk size to update_progress(), which will make the progress reporting inaccurate when throttled.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 LGTM
@ericcurtin waiting for you to update based on Sorcery update. Otherwise merge. |
6b514de
to
1126431
Compare
To help with CPU usage Signed-off-by: Eric Curtin <[email protected]>
1126431
to
79c6d19
Compare
This is ready @rhatdan @maxamillion @jhjaggars |
To help with CPU usage
Summary by Sourcery
Enhancements: