feat(storage): reduce copies in InsertObject()
#11014
Conversation
We can avoid a data copy if we use `absl::string_view` to contain the object payload.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #11014 +/- ##
=======================================
Coverage 93.77% 93.77%
=======================================
Files 1724 1724
Lines 155205 155294 +89
=======================================
+ Hits 145546 145631 +85
- Misses 9659 9663 +4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| handle_.SetOption(CURLOPT_UPLOAD, 0L); | ||
| if (!payload.empty()) { | ||
| handle_.SetOption(CURLOPT_POSTFIELDSIZE, payload.length()); | ||
| handle_.SetOption(CURLOPT_POSTFIELDS, payload.c_str()); | ||
| handle_.SetOption(CURLOPT_POSTFIELDS, payload.data()); |
There was a problem hiding this comment.
string_view::data() without string_view::size() is a red flag.
There was a problem hiding this comment.
There was a .length() already, changed to .size().
There was a problem hiding this comment.
Ahh ... sorry I missed that.
| inline std::string ComputeMD5Hash(char const* payload) { | ||
| auto p = | ||
| payload == nullptr ? absl::string_view{} : absl::string_view{payload}; | ||
| return ComputeMD5Hash(std::move(p)); |
There was a problem hiding this comment.
Moving a string_view is an anti-pattern, I believe (et seq).
There was a problem hiding this comment.
Fixed (everywhere I hope).
| @@ -80,7 +81,16 @@ inline std::string UrlsafeBase64Encode(Collection const& bytes) { | |||
| StatusOr<std::vector<std::uint8_t>> UrlsafeBase64Decode(std::string const& str); | |||
|
|
|||
| /// Compute the MD5 hash of @p payload | |||
| std::vector<std::uint8_t> MD5Hash(std::string const& payload); | |||
| std::vector<std::uint8_t> MD5Hash(absl::Span<char const> payload); | |||
There was a problem hiding this comment.
Why did we need to introduce a Span overload?
There was a problem hiding this comment.
Removed. I am thinking that these should be receiving "bytes" as input, but we should do that in a separate PR.
| handle_.SetOption(CURLOPT_UPLOAD, 0L); | ||
| if (!payload.empty()) { | ||
| handle_.SetOption(CURLOPT_POSTFIELDSIZE, payload.length()); | ||
| handle_.SetOption(CURLOPT_POSTFIELDS, payload.c_str()); | ||
| handle_.SetOption(CURLOPT_POSTFIELDS, payload.data()); |
There was a problem hiding this comment.
Ahh ... sorry I missed that.
We can avoid a data copy if we use
absl::string_viewto contain the object payload.This change is