-
Notifications
You must be signed in to change notification settings - Fork 385
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
feat(storage): lazy allocation for upload buffer #11633
feat(storage): lazy allocation for upload buffer #11633
Conversation
Allocating large upload buffers upfront can be slow if the application is going to send a small amount of data. Better grow the buffer as needed.
}); | ||
} | ||
if (!last_status_.ok()) return traits_type::eof(); | ||
if (count + actual_size < max_buffer_size_) { |
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.
Q. Is there any reason to rewrite the test to avoid any possibility of overflow?
For example, if we know actual_size <= max_buffer_size_
, then we might instead say...
if (count < max_buffer_size_ - actual_size) {
Aside: Should the test be <=
?
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.
Q. Is there any reason to rewrite the test to avoid any possibility of overflow?
For example, if we know
actual_size <= max_buffer_size_
, then we might instead say...if (count < max_buffer_size_ - actual_size) {
Thanks, done.
Aside: Should the test be
<=
?
I think it should be <
: we want to flush the data when count == max_buffer_size - actual_size
because at that point the buffer is large enough to justify the cost of said flush.
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.
I see. It's not "is there space", but rather "is there space plus some". Thanks.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #11633 +/- ##
=======================================
Coverage 93.78% 93.78%
=======================================
Files 1824 1824
Lines 164394 164396 +2
=======================================
+ Hits 154171 154184 +13
+ Misses 10223 10212 -11
☔ View full report in Codecov by Sentry. |
Allocating large upload buffers upfront can be slow if the application is going to send a small amount of data. Better grow the buffer as needed.
This change is