feat(storage): Client Setup & REST feature tracking#16175
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a FeatureTracker mechanism to track and report client-side optimizations and configuration choices in the Google Cloud Storage C++ client via a custom x-goog-storage-cpp-features header. The feedback highlights two key improvements: first, optimizing EncodeFeatureTrackerBitmask by replacing std::vectorstd::uint8_t with std::string to leverage Small String Optimization (SSO) and avoid heap allocations on the hot path; second, correcting a test assertion in stub_test.cc where ElementsAre is incorrectly used on a string value, which would cause a compilation or matching failure.
| std::vector<std::uint8_t> bytes; | ||
| for (int i = 0; i < 4; ++i) { | ||
| auto b = static_cast<std::uint8_t>((mask >> (24 - i * 8)) & 0xFF); | ||
| if (!bytes.empty() || b != 0) { | ||
| bytes.push_back(b); | ||
| } | ||
| } | ||
| return Base64Encode(bytes); |
There was a problem hiding this comment.
Using std::vector<std::uint8_t> to accumulate the bytes for the bitmask causes a heap allocation on every call to EncodeFeatureTrackerBitmask (which is invoked on every REST request via AddHeaders).
Since the maximum size of the bitmask is 4 bytes, we can use std::string instead. Due to Small String Optimization (SSO), std::string will avoid heap allocation entirely, improving performance on the hot path.
std::string bytes;
for (int i = 0; i < 4; ++i) {
auto b = static_cast<char>((mask >> (24 - i * 8)) & 0xFF);
if (!bytes.empty() || b != 0) {
bytes.push_back(b);
}
}
return Base64Encode(bytes);| Contains(Pair(kFeatureTrackerHeaderName, | ||
| ElementsAre(tracker->HeaderValue())))))) |
There was a problem hiding this comment.
Since RestRequest::headers() returns a collection of std::pair<std::string, std::string>, the value of the pair is a std::string, not a container. Using ElementsAre here is incorrect and will cause a compilation or matching failure. It should match the string directly.
Contains(Pair(kFeatureTrackerHeaderName,
tracker->HeaderValue())))3786379 to
6fea414
Compare
No description provided.