-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
gensupport: Allow user to provide his own buffer used for uploading #632
base: main
Are you sure you want to change the base?
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
@zimnx Can you please create an issue for this PR. We like to discuss features on issues before implementing them. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion. I'd also appreciate a feature request issue opened so we can discuss on there.
This seems like a good start to this feature, but since these changes are only for gensupport it would require additional work to expose this option through the generated clients and subsequently the manual google-cloud-go clients.
Added #638 |
42cd33c
to
e5cfc33
Compare
any chance to merge it soon? Other MediaOptions are not directly used in generated clients, there are only mentions about them in comments around Media function where they can be used. |
Ahh I see I was mistaken about that-- so it will just be the manual client writer that will need to be updated for storage (and maybe bigquery). I'm going to do a bit more discussion with my colleagues and let you know. You'd also definitely have to add a test for the new media option to
|
e66a82a
to
ecefbdc
Compare
Hi folks, any updates regarding merging this? :) |
This patch adds the hability to specify a user-defined buffer to be used for file uploads to Google Storage in order to reduce memory allocations. The user needs to keep track of this buffer somehow, normally with sync.Buffer, and ensure it's not used by multiple goroutines. This patch depends on this one from googleapis: googleapis/google-api-go-client#632
👋 Regularly seeing high heap/allocations which this PR could help. Anything else needed to get this merged? |
If it helps anyone: we "fixed" this by setting ChunkSize=0 on all our Writers, which disables use of the buffer entirely. Since we do our own retries elsewhere that seems fine. |
When uploading lots of files using ObjectStorage.Insert(), each file allocates his own buffer. This results in increased CPU usage and high RSS memory.
WithBuffer allows user to provide his own buffer which will be used for streaming.
Graph below shows how many allocations were done during upload of ~30k of 10MB files:
![Selection_132](https://user-images.githubusercontent.com/3951334/90792638-ff434c00-e30a-11ea-9088-240c5d204252.png)
![Selection_135](https://user-images.githubusercontent.com/3951334/90792695-14b87600-e30b-11ea-9668-cc4abcd47ad6.png)
With this change, there're no allocations at all on library side (pool.New is code on caller side)