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

High memory usage when uploading to Azure Storage #1031

Closed
jspath-ankored opened this issue Nov 7, 2023 · 12 comments · Fixed by #1070
Closed

High memory usage when uploading to Azure Storage #1031

jspath-ankored opened this issue Nov 7, 2023 · 12 comments · Fixed by #1070
Labels

Comments

@jspath-ankored
Copy link

jspath-ankored commented Nov 7, 2023

Describe the bug
I see very high memory usage when uploading large files to Azure Storage. For example, a 221MB file upload results in using over 221MB of memory. I see this behavior when uploading to actual Azure Storage as well as uploading to Azurite locally.

It seems like something in the tusd Azure Storage code may be retaining all of the file info in memory, instead of streaming it to Azure Storage or an Azure Storage emulator, like Azurite.

To Reproduce
Steps to reproduce the behavior:

  1. Setup tusd to upload to Azure Storage
  2. Ensure you have a way to monitor memory on the tusd server
  3. Upload a large file >= 221MB
  4. Look at the memory usage after

Expected behavior
I would expect the memory usage to stay relatively low.

Setup details
Please provide following details, if applicable to your situation:

  • Operating System: Running tusd docker image tusproject/tusd:v2.0
  • Used tusd version: 2.0
  • Used tusd data storage: Azure Storage
  • Used tusd configuration:
    • --azure-storage uploads --azure-endpoint https://AZURE-ACCOUNT.blob.core.windows.net --hooks-http MYWEBHOOKURL --hooks-http-forward-headers Authorization --hooks-enabled-events pre-create,pre-finish --cors-expose-headers X-Upload-Properties-Set,X-Upload-File-Path --behind-proxy --expose-pprof
      • I use --behind-proxy on Azure only, not locally. But I see the problem both locally and on Azure.
  • Used tus client library: Uppy.js
@jspath-ankored
Copy link
Author

here's a heap dump of me running it locally:

heap.txt

@Acconut
Copy link
Member

Acconut commented Nov 21, 2023

@Acconut
Copy link
Member

Acconut commented Nov 21, 2023

From the heap dump, it is apparent that there is a 128MB and a 64MB allocated at

#	0x1007dd07b	bytes.growSlice+0x8b									/usr/local/go/src/bytes/buffer.go:249
#	0x1007dca9b	bytes.(*Buffer).grow+0x12b								/usr/local/go/src/bytes/buffer.go:151
#	0x1007dcea3	bytes.(*Buffer).ReadFrom+0x43								/usr/local/go/src/bytes/buffer.go:209
#	0x100912e57	bufio.(*Reader).WriteTo+0xb7								/usr/local/go/src/bufio/bufio.go:533
#	0x100aa2dbf	github.com/tus/tusd/v2/pkg/azurestore.(*AzUpload).WriteChunk+0x13f			/Users/jspath/git/tusd/pkg/azurestore/azurestore.go:145

It does indeed seems like that azurestore is copying the entire upload data into a memory buffer before uploading it to Azure:

r := bufio.NewReader(src)
buf := new(bytes.Buffer)
n, err := r.WriteTo(buf)
if err != nil {
return 0, err
}

A new buffer is created and the entire request body is written into the buffer before the final size is examined and the data is uploaded. No data is streamed. I am not sure if that is possible with Azure. @omBratteng, do you know if there is a better way to avoid these allocations? For the s3store we save chunks of data on disk to avoid in-memory buffers, for example.

@jspath-ankored Does the memory get properly released once the upload is completed? Or is the memory kept alive after the upload, meaning we would have a memory leak.

@omBratteng
Copy link
Contributor

It's been a while since I worked with this and I have since gotten a new job.

But if I recall correctly, it only keeps the chunks in memory before it uploads it to blob storage. We have uploaded files of ~10TiB without any issues (though we didn't really have any memory constraints in our cluster).

Though, there shouldn't be any issues with save chunks of data on disk.

@jspath-ankored
Copy link
Author

jspath-ankored commented Nov 21, 2023

It looks like it's not a memory leak, and the memory does eventually get released, although it takes around five minutes:
Screenshot 2023-11-21 at 10 28 54 AM

It would still be preferable if the memory usage didn't grow like this ... I uploaded a 221MB file, and after the 2nd upload, we were using 922.8MB of memory. This makes me concerned about using this on production.

If it matters, I am measuring the memory using the Live Charts extension for Docker Desktop. So maybe I'm just measuring the memory that the Docker container is using.

I know you can stream to Azure Blob Storage in .NET ... not sure about Go.

@Acconut
Copy link
Member

Acconut commented Nov 22, 2023

the memory does eventually get released, although it takes around five minutes

I am no expert at Go's garbage collector, but Go might not release memory back to the OS immediately. The memory might be flagged internally as unused, but releasing it to the OS is another step that might only be taken when the memory is actually needed my another process. But please don't cite me on this :)

If it matters, I am measuring the memory using the Live Charts extension for Docker Desktop. So maybe I'm just measuring the memory that the Docker container is using.

If you want more insights into the memory usage as seen from the Go internals, you can use the metrics exposed by tusd (e.g. http://tusd.tusdemo.net/metrics). They contain many different memory-related metrics and might help you understand how much memory Go thinks is in use right now.

However, I agree that it would be ideal to avoid this in-memory buffering altogether. Would you be interested in trying out to stream the data directly to Azure (or stream it in chunks)?

@jspath-ankored
Copy link
Author

Would you be interested in trying out to stream the data directly to Azure (or stream it in chunks)?

Yes. We are not using tusd in production yet, and won't be a for a while yet, so I am open to trying out streaming.

Are you asking me to implement, or just try out some changes?

@Acconut
Copy link
Member

Acconut commented Nov 23, 2023

Are you asking me to implement

Yes, improving the memory usage of the Azure store would require some implementation changes. I personally do not use Azure on my own and don't have access to it, making me not the best person to tackle this.

@quality-leftovers
Copy link

We noticed that the body is ready into memory when reviewing the code before settling and tusd and decided to use 5 MiB Patch Chunks in the clients for now. I'd like to have tried taking a stab at improving it but didn't have time for it :(

@Acconut
Copy link
Member

Acconut commented Jan 15, 2024

Does this also occur if you use Azurite? If so, I can try to reproduce it locally and debug it.

@jspath-ankored
Copy link
Author

Does this also occur if you use Azurite? If so, I can try to reproduce it locally and debug it.

Yes, the same issue happens using Azurite locally:
https://community.transloadit.com/t/tusd-failing-on-large-file-upload/16883/4

@Acconut
Copy link
Member

Acconut commented Jan 23, 2024

I implemented the buffer in a temporary file on disk instead of in-memory. Please have a look at #1070, test it out, and let me know if it behaves as expected, especially in terms of memory usage. In my local tests, no memory growth was visible over time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants