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

perf: Copy files instead of sending over HTTP if Wave app and Wave server are running on the same machine. #982 #1765

Merged
merged 23 commits into from Jan 20, 2023

Conversation

mturoci
Copy link
Collaborator

@mturoci mturoci commented Dec 15, 2022

This is a rough proposal for further discussion. If approved, will finish the rest of the sync/async upload methods.

Rationale: When uploading files, we send them over HTTP with serialization/deserialization overhead that slows the whole method. However, most of the Wave apps run both Wave server (waved) and Wave app on the same machine which means a simple file copy should be enough.

Perf testing

HTTPX 0.16.1 is the original version from the time of creating issue #982.

File size HTTPX 0.16.1 HTTPX 0.23.1 (latest - Mac) cp (Mac) robocopy (Windows) xcopy (Windows) HTTPX 0.23.1 (latest - Windows)
1GB ~8.5s ~4.3s ~14ms ~50ms ~800ms ~5.8s
10 GB 10min timeout ~40s ~10ms ~150ms ~75s 10min timeout

Although we have public/private dirs, the concept of serving files and figuring out what is the correct URL can be troublesome for non-software engineering users so I would suggest keeping the current q.site.upload as is.

This change would impact most (if not all) Wave users who use q.site.upload.

Wdyt @lo5?

Closes #982

Copy link
Member

@lo5 lo5 left a comment

Choose a reason for hiding this comment

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

The implementation looks good.

However, because the implementation unconditionally resorts to copy-if-local, it's possible we might hit some unforeseen OS permissions/config where the copying might fail.

Possible safeguards are:
A. Fallback to HTTP if local copy fails (try-except).
B. Enable copy-if-local behavior only if an env var is set.
C. Enable copy-if-local by default, but allow disabling behavior if an env var is set (provides an escape hatch if existing installations get hosed).

Thoughts?

@mturoci
Copy link
Collaborator Author

mturoci commented Jan 10, 2023

Thanks for the suggestions @lo5! A combination of A and C seems like the way to go for me. Will do.

@mturoci
Copy link
Collaborator Author

mturoci commented Jan 13, 2023

The PR is done.

  • Tested against both Windows and Unix.
  • Made async (to not block the main thread).

@lo5 can you please let me know what you think about naming? (H2O_WAVE_NO_COPY_UPLOAD, H2O_WAVE_WAVED_DIR). Once resolved, can be merged.

Copy link
Member

@lo5 lo5 left a comment

Choose a reason for hiding this comment

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

Names look fine. Minor changes to the docs.

website/docs/configuration.md Outdated Show resolved Hide resolved
website/docs/configuration.md Outdated Show resolved Hide resolved
mturoci and others added 23 commits January 20, 2023 08:03
Co-authored-by: Prithvi <prithvi.prabhu@gmail.com>
Co-authored-by: Prithvi <prithvi.prabhu@gmail.com>
@mturoci mturoci merged commit 8e31e07 into master Jan 20, 2023
@mturoci mturoci deleted the perf/issue-982 branch January 20, 2023 07:18
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.

q.site.upload is slow
2 participants