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

Tune upload perf, better cleanup logic #2532

Merged
merged 2 commits into from
Nov 17, 2021
Merged

Tune upload perf, better cleanup logic #2532

merged 2 commits into from
Nov 17, 2021

Conversation

nl0
Copy link
Member

@nl0 nl0 commented Nov 15, 2021

Description

  1. Limit hashing concurrency to prevent file access errors
  2. Optimize upload code to reduce memory pressure (avoid creating and storing a closure for every upload)
  3. Fall back to server-side hashing when it can't be done client-side

TODO

@nl0 nl0 requested review from fiskus and akarve November 15, 2021 14:13
@codecov
Copy link

codecov bot commented Nov 15, 2021

Codecov Report

Merging #2532 (c7a4447) into master (6889d4b) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

❗ Current head c7a4447 differs from pull request most recent head 4fdf92a. Consider uploading reports for the commit 4fdf92a to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2532      +/-   ##
==========================================
- Coverage   43.75%   43.74%   -0.02%     
==========================================
  Files         504      504              
  Lines       24257    24266       +9     
  Branches     3275     3276       +1     
==========================================
  Hits        10614    10614              
- Misses      12816    12825       +9     
  Partials      827      827              
Flag Coverage Δ
api-python 90.26% <ø> (ø)
catalog 14.67% <0.00%> (-0.01%) ⬇️
lambda 93.95% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...app/containers/Bucket/PackageDialog/FilesInput.tsx 0.00% <0.00%> (ø)
.../containers/Bucket/PackageDialog/PackageDialog.tsx 24.26% <0.00%> (+0.07%) ⬆️
...og/app/containers/Bucket/PackageDialog/Uploads.tsx 0.00% <0.00%> (ø)
catalog/app/utils/ResourceCache.js 19.14% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6889d4b...4fdf92a. Read the comment docs.

Copy link
Member

@akarve akarve left a comment

Choose a reason for hiding this comment

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

I will test tomorrow and circle back

Copy link
Member

@akarve akarve left a comment

Choose a reason for hiding this comment

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

I am currently testing this with a 6.9GB upload on Chrome / Mac OS / 2016 MacBook Pro.

  1. OK for separate PR: We should bump the warning size to idk 10GB (I got a warning for being over 1GB)
  2. The UI thread stalled for me as I was typing in the name field (after dragging and dropping my files) — about a 2.5 sec pause but then it was OK.
  3. My uplink is 21.5 Mbps and I seem to be ~ about saturating it; which is good. I'd say I'll finish 6.9GB in about 35 min total wall time.
  4. OK for separate PR: The progress optically appears to stall when the upload goes over 1GB but it's only because the number changes more slowly :) We should either always show more decimal places of precision above 1GB and/or we should show indeterminate progress
  5. OK for separate PR; it would be really nice to have some concurrency knobs (if meaningful) in the catalog settings so that different machines/users have some control and we can learn from the field how to optimize performance

@@ -148,6 +148,8 @@ function* handleInit({ resource, input, resolver }) {
}

function* cleanup() {
// TODO: refactor cleanup logic, so that the cleanup action is only dispatched
// when there's anything to cleanup (to avoid re-renders every 5 sec)
while (true) {
Copy link
Member

Choose a reason for hiding this comment

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

😬

@nl0
Copy link
Member Author

nl0 commented Nov 17, 2021

The UI thread stalled for me as I was typing in the name field (after dragging and dropping my files) — about a 2.5 sec pause but then it was OK.

it was processing the dropped files (synchronously), so it's kinda expected, tho not very nice, i guess

@nl0
Copy link
Member Author

nl0 commented Nov 17, 2021

OK for separate PR; it would be really nice to have some concurrency knobs (if meaningful) in the catalog settings so that different machines/users have some control and we can learn from the field how to optimize performance

@akarve i'm not really sure how to approach this, ping me in slack for a discussion if you feel this is important

@nl0 nl0 merged commit 3177e45 into master Nov 17, 2021
@nl0 nl0 deleted the tune-upload-perf branch November 17, 2021 06:31
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.

2 participants