muxinc / upchunk

Uploads Chunks! Takes big files, splits them up, then uploads each one with care (and PUT requests).
MIT License
336 stars 46 forks source link

Browser memory usage #89

Closed ylt closed 1 year ago

ylt commented 2 years ago

Been finding that when uploading large files (i.e. 6gb), the browsers RAM usage increases to include the filesize. This can exceed the users RAM and end up with the users browsers tab crashing or their computer crashing.

Looking at this myself, I've been able to reproduce it on both Firefox (102) and Chrome (103) on both Windows and Mac M1.

Firefox's dev tools are able to identify a bunch of ArrayBuffers that take up as much RAM as the filesize, but is unable to attribute it to a js call stack(maybe browser internals). Chrome dev tools however are unable to see the memory at all.

image

Googling, I'm not finding much authoritive, but it does seem like file.slice as done by Upchunk can be problematic. https://github.com/muxinc/upchunk/blob/1ba86a985f0df4a84207351992034ae1b8206b20/src/upchunk.ts#L220. Suggestions I'm finding appear to be to use a ReadableStream instead as that's stream based and is probably more suited for reading out chunks without loading in the whole file.

dylanjha commented 2 years ago

This is a really good find @ylt, we will dig into it and see what the best fix is.

Thanks for the clear debugging steps 🙏🏼

cjpillsbury commented 2 years ago

@ylt echoing @dylanjha thanks for the sleuthing. Confirmed this is currently an issue. I'll be investigating possible solutions here, where we'll want to make sure we're striking a good balance between performance/time to upload w/memory efficiency. I'll be sure to update here on any direction we end up taking.

digitalscream commented 2 years ago

@cjpillsbury - any has there been any update on this?

jfbaraky commented 2 years ago

We had some problems trying to upload some large files (+10GB) because we could only store up to 2GB inside the node Buffer. So, instead of using UpChunk, we changed the code to the "manual approach":

Hope it helps while UpChunk don't handle the stream directly;

cjpillsbury commented 2 years ago

@digitalscream None yet unfortunately. Our team is split across multiple projects, so this may be a few weeks out still before we can work on it in due diligence.

cjpillsbury commented 2 years ago

If anyone on this thread wants to take a stab at migrating upchunk to use read streams, PRs are always welcome!

cjpillsbury commented 1 year ago

Hey all! I know this has been a long time coming, but we've got a refactor of upchunk to use ReadableStream. I haven't yet cut a new release (will be a major release since it's a fundamental rearchitecture of upchunk), but I'll keep you posted post.

digitalscream commented 1 year ago

@cjpillsbury - thanks for that...I'll keep an eye out!

cjpillsbury commented 1 year ago

It's official: https://www.npmjs.com/package/@mux/upchunk https://github.com/muxinc/upchunk/releases/tag/v3.0.0