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

Rework EntryUploader to use promises intead of setTimeout #3386

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

liamwhite
Copy link
Contributor

This requires phoenixframework/phoenix#5897 to work.

When combined with that PR, it allows the frontend to get properly bottlenecked on the server response speed when uploading large files with live_file_upload and a chunk_size of 1_048_576.

Upload performance goes from 50MB/s to 110MB/s in my testing.
The default chunk_size of 64_000 is way too low and probably needs to be increased in this repository, but I did not do that here.

Cowboy works best for performance testing. Bandit is much slower than expected, for reasons I have yet to investigate.

@chrismccord
Copy link
Member

Will this be backwards compatible for user without the phoenix change?

@liamwhite
Copy link
Contributor Author

@chrismccord as far as I can tell, EntryUploader is an internal API so there shouldn't be any compatibility issues. If there are compatibility issues I would actually expect them to be in the linked Phoenix PR.

@chrismccord
Copy link
Member

To be clear:
This requires phoenixframework/phoenix#5897 to work.

You mean it requires the Phoenix PR to realize the optimizations or it requires the phoenix PR to work at all?

@liamwhite
Copy link
Contributor Author

liamwhite commented Sep 30, 2024

It requires it to work at all. It will not work without the Phoenix PR because the Phoenix PR enables the most important part of it, the copyless socket sends. The upstream Phoenix code makes either two or three superfluous copies of the data depending on the transport used, and that PR makes it so that it makes zero superfluous copies.

@chrismccord
Copy link
Member

We would need the Phoenix PR to be backwards compatible but it's still not clear to me where the breaking change is happening on the Phoenix side. Can you clarify? Thanks!

@liamwhite
Copy link
Contributor Author

@chrismccord I tested the Phoenix PR but could not find a way in which it broke backwards compatibility, since it continues to pass all external API tests. But I'm not totally confident and I'd suggest also checking for yourself.

@chrismccord
Copy link
Member

Sorry for the back and forth, but I want to understand this statement:

It requires it to work at all.

If the Phoenix PR is supposed to be backwards compatible, then merging this and running on an old Phoenix should merely miss out on the optimizations, no?

@liamwhite
Copy link
Contributor Author

I'm really sorry for the confusion.

  • The Phoenix PR is intended to be backwards compatible with other Phoenix scripts. I tested it, and it doesn't cause any issues on older scripts from what I could tell, but it's possible it has some subtle issue.
  • This LiveView PR requires the new Phoenix script and does not merely miss out on an optimization, because the old Phoenix script breaks if you pass a Blob instead of an ArrayBuffer.
  • I am not aware of a way for the LiveView script to differentiate between versions of the Phoenix script, but if it could, then that could be used to fall back to passing an ArrayBuffer.

Does that clear it up?

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