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

Provide objectSize for S3 upload to reduce memory usage #1933

Closed
AndreasSko opened this issue Jul 28, 2021 · 4 comments · Fixed by #1940
Closed

Provide objectSize for S3 upload to reduce memory usage #1933

AndreasSko opened this issue Jul 28, 2021 · 4 comments · Fixed by #1940

Comments

@AndreasSko
Copy link
Contributor

When using OCIS with the S3ng storage enabled, I noticed relatively high memory usage when uploading files. This could be more than 1GB for a single user. I did a bit of digging and noticed that in

_, err := bs.client.PutObject(context.Background(), bs.bucket, key, reader, -1, minio.PutObjectOptions{ContentType: "application/octet-stream"})
no objectSize is provided, but rather just a -1. According to minio/minio-go#1496 this can result in the observed memory usage.
I did some testing on my own and checked what will happen if the correct size is provided. In my testing, the memory usage of OCIS was reduced significantly - from >1GB down to <300MB.
If you like, I'm happy to open a PR to fix this issue. The question is: What would be the best way? In AndreasSko@be1e7de I changed the interfaces for
WriteBlob(key string, reader io.Reader) error
and
Upload(key string, reader io.Reader) error
to expect an *os.File instead of an io.Reader. But I could imagine to also simply provide the fileSize as an int64 directly. I'm open for feedback here 🙂

Related issue: owncloud/ocis#2326

@ishank011
Copy link
Contributor

@aduffeck @butonic this looks like a pretty nice improvement. Can you take a look at @AndreasSko's changes?

@aduffeck
Copy link
Contributor

aduffeck commented Jul 30, 2021

@AndreasSko nice find. I think it would be good to keep the io.Reader interface in the signature though.

Did you try specifying the PartSize in the upload options by any chance? That should also limit the memory usage by using smaller chunks (by default it uploads 512MB chunks apparently). Maybe something like

	_, err := bs.client.PutObject(context.Background(), bs.bucket, key, reader, size, minio.PutObjectOptions{
		ContentType: "application/octet-stream",
		PartSize:    100 * 1024 * 1024,
	})

would work.

@aduffeck
Copy link
Contributor

If that's not an option maybe we could try to infer the file size dynamically if possible instead, e.g. like so (untested)

	size := int64(-1)
	if file, ok := reader.(*os.File); ok {
		info, err := file.Stat()
		if err != nil {
			return err
		}
		size = int64(info.Size())
	}

	_, err := bs.client.PutObject(context.Background(), bs.bucket, key, reader, size, minio.PutObjectOptions{ContentType: "application/octet-stream"})

@AndreasSko
Copy link
Contributor Author

AndreasSko commented Jul 30, 2021

If that's not an option maybe we could try to infer the file size dynamically if possible instead, e.g. like so (untested)

Yeah, that's a good idea! I will try that out and open a PR if it looks good. Thank you! 🙂

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

Successfully merging a pull request may close this issue.

3 participants