From 9c5199f7b706ad6b79c91613a43a875d608d91f4 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Wed, 28 Oct 2020 13:33:13 -0700 Subject: [PATCH 01/46] buffer: improve read reservations to efficiently handle multiple slices Signed-off-by: Greg Greenway --- include/envoy/buffer/buffer.h | 36 ++++++++++ source/common/buffer/buffer_impl.cc | 66 +++++++++++++++++++ source/common/buffer/buffer_impl.h | 45 ++++++++++--- source/common/buffer/watermark_buffer.cc | 5 ++ source/common/buffer/watermark_buffer.h | 1 + .../common/network/io_socket_handle_impl.cc | 12 +--- source/common/network/raw_buffer_socket.cc | 2 +- .../transport_sockets/tls/ssl_socket.cc | 34 +++++----- .../transport_sockets/tls/ssl_socket.h | 2 +- test/common/buffer/buffer_fuzz.cc | 14 ++++ test/common/network/connection_impl_test.cc | 2 +- .../postgres_proxy/postgres_decoder_test.cc | 2 + 12 files changed, 182 insertions(+), 39 deletions(-) diff --git a/include/envoy/buffer/buffer.h b/include/envoy/buffer/buffer.h index 3ab150504ccd..cc5edc825584 100644 --- a/include/envoy/buffer/buffer.h +++ b/include/envoy/buffer/buffer.h @@ -71,6 +71,8 @@ class SliceData { using SliceDataPtr = std::unique_ptr; +class Reservation; + /** * A basic buffer abstraction. */ @@ -201,6 +203,9 @@ class Instance { */ virtual uint64_t reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iovecs) PURE; + virtual Reservation reserve(uint64_t preferred_length) PURE; + virtual void commit(Reservation& reservation, uint64_t length) PURE; + /** * Search for an occurrence of data within the buffer. * @param data supplies the data to search for. @@ -411,5 +416,36 @@ class WatermarkFactory { using WatermarkFactoryPtr = std::unique_ptr; +/** + * Holds an in-progress addition to a buffer. + * + * @note For performance reasons, this class is passed by value to + * avoid an extra allocation, so it cannot have any virtual methods. + */ +class Reservation final { +public: + Reservation(Reservation&&) = default; + ~Reservation() { + if (!slices_.empty()) { + buffer_.commit(*this, 0); + } + } + RawSlice* slices() { return slices_.data(); } + uint64_t numSlices() const { return slices_.size(); } + void commit(uint64_t length) { + buffer_.commit(*this, length); + slices_.clear(); + owned_slices_.clear(); + } + + // private: + friend class Instance; + Reservation(Instance& buffer) : buffer_(buffer) {} + Instance& buffer_; + static constexpr uint32_t num_elements_ = 9; + absl::InlinedVector slices_; + absl::InlinedVector owned_slices_; +}; + } // namespace Buffer } // namespace Envoy diff --git a/source/common/buffer/buffer_impl.cc b/source/common/buffer/buffer_impl.cc index 556faf73d638..4e775b147b02 100644 --- a/source/common/buffer/buffer_impl.cc +++ b/source/common/buffer/buffer_impl.cc @@ -18,6 +18,9 @@ namespace { constexpr uint64_t CopyThreshold = 512; } // namespace +thread_local absl::InlinedVector, OwnedSlice::free_list_max_> + OwnedSlice::free_list_; + void OwnedImpl::addImpl(const void* data, uint64_t size) { const char* src = static_cast(data); bool new_slice_needed = slices_.empty(); @@ -378,6 +381,69 @@ uint64_t OwnedImpl::reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iove return num_slices_used; } +Reservation OwnedImpl::reserve(uint64_t length) { + Reservation reservation(*this); + if (length == 0) { + return reservation; + } + + // Remove any empty slices at the end. + while (!slices_.empty() && slices_.back()->dataSize() == 0) { + slices_.pop_back(); + } + + uint64_t bytes_remaining = length; + + // Check whether there are any empty slices with reservable space at the end of the buffer. + uint64_t reservable_size = slices_.empty() ? 0 : slices_.back()->reservableSize(); + if (reservable_size >= length || reservable_size >= (OwnedSlice::default_slice_size_ / 8)) { + auto& last_slice = slices_.back(); + const uint64_t reservation_size = std::min(last_slice->reservableSize(), bytes_remaining); + auto slice = last_slice->reserve(reservation_size); + reservation.slices_.push_back(slice); + reservation.owned_slices_.push_back(nullptr); + bytes_remaining -= slice.len_; + } + + while (bytes_remaining != 0) { + const uint64_t size = OwnedSlice::default_slice_size_; + auto slice = OwnedSlice::create(size); + reservation.slices_.push_back(slice->reserve(size)); + reservation.owned_slices_.emplace_back(std::move(slice)); + bytes_remaining -= std::min(reservation.slices_.back().len_, bytes_remaining); + } + + ASSERT(reservation.slices_.size() <= reservation.num_elements_, + "The absl::InlineVector should be sized so that out-of-line storage isn't needed."); + ASSERT(reservation.slices_.size() == reservation.owned_slices_.size()); + return reservation; +} + +void OwnedImpl::commit(Reservation& reservation, uint64_t length) { + ASSERT(reservation.slices_.size() == reservation.owned_slices_.size()); + uint64_t bytes_remaining = length; + for (uint32_t i = 0; i < reservation.slices_.size(); i++) { + ASSERT((reservation.owned_slices_[i] != nullptr) == + (dynamic_cast(reservation.owned_slices_[i].get()) != nullptr)); + std::unique_ptr owned_slice( + static_cast(reservation.owned_slices_[i].release())); + + if (bytes_remaining > 0) { + if (owned_slice != nullptr) { + slices_.emplace_back(std::move(owned_slice)); + } + reservation.slices_[i].len_ = + std::min(reservation.slices_[i].len_, bytes_remaining); + bool success = slices_.back()->commit(reservation.slices_[i]); + ASSERT(success); + length_ += reservation.slices_[i].len_; + bytes_remaining -= reservation.slices_[i].len_; + } else { + OwnedSlice::free(std::move(owned_slice)); + } + } +} + ssize_t OwnedImpl::search(const void* data, uint64_t size, size_t start, size_t length) const { // This implementation uses the same search algorithm as evbuffer_search(), a naive // scan that requires O(M*N) comparisons in the worst case. diff --git a/source/common/buffer/buffer_impl.h b/source/common/buffer/buffer_impl.h index cfbaadd82323..d6857d284d87 100644 --- a/source/common/buffer/buffer_impl.h +++ b/source/common/buffer/buffer_impl.h @@ -261,9 +261,18 @@ class OwnedSlice final : public Slice, public InlineStorage { * @param capacity number of bytes of space the slice should have. * @return an OwnedSlice with at least the specified capacity. */ - static SlicePtr create(uint64_t capacity) { + static std::unique_ptr create(uint64_t capacity) { + ASSERT(sliceSize(default_slice_size_) == default_slice_size_); + uint64_t slice_capacity = sliceSize(capacity); - return SlicePtr(new (slice_capacity) OwnedSlice(slice_capacity)); + std::unique_ptr slice; + if (slice_capacity == default_slice_size_ && !free_list_.empty()) { + slice = std::move(free_list_.back()); + free_list_.pop_back(); + } else { + slice.reset(new (slice_capacity) OwnedSlice(slice_capacity)); + } + return slice; } /** @@ -274,13 +283,28 @@ class OwnedSlice final : public Slice, public InlineStorage { * the internal implementation) have a nonzero amount of reservable space at the end. */ static SlicePtr create(const void* data, uint64_t size) { - uint64_t slice_capacity = sliceSize(size); - std::unique_ptr slice(new (slice_capacity) OwnedSlice(slice_capacity)); + std::unique_ptr slice(create(size)); memcpy(slice->base_, data, size); slice->reservable_ = size; return slice; } + static constexpr uint32_t default_slice_size_ = 16384; + + static void free(std::unique_ptr slice) { + if (slice == nullptr) { + return; + } + + if (slice->capacity_ == default_slice_size_ && slice->reservable_ == 0 && + free_list_.size() < free_list_max_) { + free_list_.emplace_back(std::move(slice)); + ASSERT(slice == nullptr); + } else { + slice.reset(); + } + } + private: OwnedSlice(uint64_t size) : Slice(0, 0, size) { base_ = storage_; } @@ -291,12 +315,15 @@ class OwnedSlice final : public Slice, public InlineStorage { * @param data_size the minimum amount of data the slice must be able to store, in bytes. * @return a recommended slice size, in bytes. */ - static uint64_t sliceSize(uint64_t data_size) { - static constexpr uint64_t PageSize = 4096; - const uint64_t num_pages = (sizeof(OwnedSlice) + data_size + PageSize - 1) / PageSize; - return num_pages * PageSize - sizeof(OwnedSlice); + constexpr static uint64_t sliceSize(uint64_t data_size) { + constexpr uint64_t PageSize = 4096; + const uint64_t num_pages = (data_size + PageSize - 1) / PageSize; + return num_pages * PageSize; } + static constexpr uint32_t free_list_max_ = 8; + static thread_local absl::InlinedVector, free_list_max_> free_list_; + uint8_t storage_[]; }; @@ -561,6 +588,7 @@ class OwnedImpl : public LibEventInstance { void prepend(absl::string_view data) override; void prepend(Instance& data) override; void commit(RawSlice* iovecs, uint64_t num_iovecs) override; + void commit(Reservation& reservation, uint64_t length) override; void copyOut(size_t start, uint64_t size, void* data) const override; void drain(uint64_t size) override; RawSliceVector getRawSlices(absl::optional max_slices = absl::nullopt) const override; @@ -570,6 +598,7 @@ class OwnedImpl : public LibEventInstance { void move(Instance& rhs) override; void move(Instance& rhs, uint64_t length) override; uint64_t reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iovecs) override; + Reservation reserve(uint64_t preferred_length) override; ssize_t search(const void* data, uint64_t size, size_t start, size_t length) const override; bool startsWith(absl::string_view data) const override; std::string toString() const override; diff --git a/source/common/buffer/watermark_buffer.cc b/source/common/buffer/watermark_buffer.cc index 0503266085b7..ed28bac75265 100644 --- a/source/common/buffer/watermark_buffer.cc +++ b/source/common/buffer/watermark_buffer.cc @@ -36,6 +36,11 @@ void WatermarkBuffer::commit(RawSlice* iovecs, uint64_t num_iovecs) { checkHighAndOverflowWatermarks(); } +void WatermarkBuffer::commit(Reservation& reservation, uint64_t length) { + OwnedImpl::commit(reservation, length); + checkHighAndOverflowWatermarks(); +} + void WatermarkBuffer::drain(uint64_t size) { OwnedImpl::drain(size); checkLowWatermark(); diff --git a/source/common/buffer/watermark_buffer.h b/source/common/buffer/watermark_buffer.h index 23c7c32854d2..d975b4dc6cb2 100644 --- a/source/common/buffer/watermark_buffer.h +++ b/source/common/buffer/watermark_buffer.h @@ -36,6 +36,7 @@ class WatermarkBuffer : public OwnedImpl { void move(Instance& rhs, uint64_t length) override; SliceDataPtr extractMutableFrontSlice() override; uint64_t reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iovecs) override; + void commit(Reservation& reservation, uint64_t length) override; void postProcess() override { checkLowWatermark(); } void appendSliceForTest(const void* data, uint64_t size) override; void appendSliceForTest(absl::string_view data) override; diff --git a/source/common/network/io_socket_handle_impl.cc b/source/common/network/io_socket_handle_impl.cc index fb1c2e120ec5..79b5c1ede6aa 100644 --- a/source/common/network/io_socket_handle_impl.cc +++ b/source/common/network/io_socket_handle_impl.cc @@ -92,17 +92,11 @@ Api::IoCallUint64Result IoSocketHandleImpl::read(Buffer::Instance& buffer, uint6 if (max_length == 0) { return Api::ioCallUint64ResultNoError(); } - constexpr uint64_t MaxSlices = 2; - Buffer::RawSlice slices[MaxSlices]; - const uint64_t num_slices = buffer.reserve(max_length, slices, MaxSlices); - Api::IoCallUint64Result result = readv(max_length, slices, num_slices); + Buffer::Reservation reservation = buffer.reserve(max_length); + Api::IoCallUint64Result result = readv(max_length, reservation.slices(), reservation.numSlices()); uint64_t bytes_to_commit = result.ok() ? result.rc_ : 0; ASSERT(bytes_to_commit <= max_length); - for (uint64_t i = 0; i < num_slices; i++) { - slices[i].len_ = std::min(slices[i].len_, static_cast(bytes_to_commit)); - bytes_to_commit -= slices[i].len_; - } - buffer.commit(slices, num_slices); + reservation.commit(bytes_to_commit); return result; } diff --git a/source/common/network/raw_buffer_socket.cc b/source/common/network/raw_buffer_socket.cc index c539add2f71a..f010ed3189bf 100644 --- a/source/common/network/raw_buffer_socket.cc +++ b/source/common/network/raw_buffer_socket.cc @@ -19,7 +19,7 @@ IoResult RawBufferSocket::doRead(Buffer::Instance& buffer) { bool end_stream = false; do { // 16K read is arbitrary. TODO(mattklein123) PERF: Tune the read size. - Api::IoCallUint64Result result = callbacks_->ioHandle().read(buffer, 16384); + Api::IoCallUint64Result result = callbacks_->ioHandle().read(buffer, 131072); if (result.ok()) { ENVOY_CONN_LOG(trace, "read returns: {}", callbacks_->connection(), result.rc_); diff --git a/source/extensions/transport_sockets/tls/ssl_socket.cc b/source/extensions/transport_sockets/tls/ssl_socket.cc index 50b2d27926e2..f66549cfb85f 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.cc +++ b/source/extensions/transport_sockets/tls/ssl_socket.cc @@ -93,16 +93,13 @@ SslSocket::ReadResult SslSocket::sslReadIntoSlice(Buffer::RawSlice& slice) { ASSERT(static_cast(rc) <= remaining); mem += rc; remaining -= rc; - result.commit_slice_ = true; + result.bytes_read_ += rc; } else { result.error_ = absl::make_optional(rc); break; } } - if (result.commit_slice_) { - slice.len_ -= remaining; - } return result; } @@ -122,17 +119,16 @@ Network::IoResult SslSocket::doRead(Buffer::Instance& read_buffer) { PostIoAction action = PostIoAction::KeepOpen; uint64_t bytes_read = 0; while (keep_reading) { + uint64_t bytes_read_this_iteration = 0; // We use 2 slices here so that we can use the remainder of an existing buffer chain element // if there is extra space. 16K read is arbitrary and can be tuned later. - Buffer::RawSlice slices[2]; - uint64_t slices_to_commit = 0; - uint64_t num_slices = read_buffer.reserve(16384, slices, 2); - for (uint64_t i = 0; i < num_slices; i++) { - auto result = sslReadIntoSlice(slices[i]); - if (result.commit_slice_) { - slices_to_commit++; - bytes_read += slices[i].len_; - } + Buffer::Reservation reservation = read_buffer.reserve(16384); + // Buffer::RawSlice slices[2]; + // uint64_t slices_to_commit = 0; + // uint64_t num_slices = read_buffer.reserve(16384, slices, 2); + for (uint64_t i = 0; i < reservation.numSlices(); i++) { + auto result = sslReadIntoSlice(reservation.slices()[i]); + bytes_read_this_iteration += result.bytes_read_; if (result.error_.has_value()) { keep_reading = false; int err = SSL_get_error(rawSsl(), result.error_.value()); @@ -162,13 +158,13 @@ Network::IoResult SslSocket::doRead(Buffer::Instance& read_buffer) { } } - if (slices_to_commit > 0) { - read_buffer.commit(slices, slices_to_commit); - if (callbacks_->shouldDrainReadBuffer()) { - callbacks_->setReadBufferReady(); - keep_reading = false; - } + reservation.commit(bytes_read_this_iteration); + if (bytes_read_this_iteration > 0 && callbacks_->shouldDrainReadBuffer()) { + callbacks_->setReadBufferReady(); + keep_reading = false; } + + bytes_read += bytes_read_this_iteration; } ENVOY_CONN_LOG(trace, "ssl read {} bytes", callbacks_->connection(), bytes_read); diff --git a/source/extensions/transport_sockets/tls/ssl_socket.h b/source/extensions/transport_sockets/tls/ssl_socket.h index 4c5f38e0fb14..e1721c0c096d 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.h +++ b/source/extensions/transport_sockets/tls/ssl_socket.h @@ -77,7 +77,7 @@ class SslSocket : public Network::TransportSocket, private: struct ReadResult { - bool commit_slice_{}; + uint64_t bytes_read_{0}; absl::optional error_; }; ReadResult sslReadIntoSlice(Buffer::RawSlice& slice); diff --git a/test/common/buffer/buffer_fuzz.cc b/test/common/buffer/buffer_fuzz.cc index 4128ceea866d..a4910533c479 100644 --- a/test/common/buffer/buffer_fuzz.cc +++ b/test/common/buffer/buffer_fuzz.cc @@ -110,6 +110,11 @@ class StringBuffer : public Buffer::Instance { size_ += iovecs[0].len_; } + void commit(Buffer::Reservation&, uint64_t length) override { + size_ += length; + FUZZ_ASSERT(start_ + size_ + length <= data_.size()); + } + void copyOut(size_t start, uint64_t size, void* data) const override { ::memcpy(data, this->start() + start, size); } @@ -152,6 +157,15 @@ class StringBuffer : public Buffer::Instance { return 1; } + Buffer::Reservation reserve(uint64_t length) override { + Buffer::RawSlice slice; + reserve(length, &slice, 1); + + Buffer::Reservation reservation(*this); + reservation.slices_.push_back(slice); + return reservation; + } + ssize_t search(const void* data, uint64_t size, size_t start, size_t length) const override { UNREFERENCED_PARAMETER(length); return asStringView().find({static_cast(data), size}, start); diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index aab097ee10b8..e073c5061d7a 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -2618,7 +2618,7 @@ TEST_P(ReadBufferLimitTest, SomeLimit) { // Envoy has soft limits, so as long as the first read is <= read_buffer_limit - 1 it will do a // second read. The effective chunk size is then read_buffer_limit - 1 + MaxReadSize, // which is currently 16384. - readBufferLimitTest(read_buffer_limit, read_buffer_limit - 1 + 16384); + readBufferLimitTest(read_buffer_limit, read_buffer_limit - 1 + (8 * 16384)); } class TcpClientConnectionImplTest : public testing::TestWithParam { diff --git a/test/extensions/filters/network/postgres_proxy/postgres_decoder_test.cc b/test/extensions/filters/network/postgres_proxy/postgres_decoder_test.cc index e787a18f2d5b..9f3495a16255 100644 --- a/test/extensions/filters/network/postgres_proxy/postgres_decoder_test.cc +++ b/test/extensions/filters/network/postgres_proxy/postgres_decoder_test.cc @@ -529,6 +529,8 @@ class FakeBuffer : public Buffer::Instance { MOCK_METHOD(void, move, (Instance&), (override)); MOCK_METHOD(void, move, (Instance&, uint64_t), (override)); MOCK_METHOD(uint64_t, reserve, (uint64_t, Buffer::RawSlice*, uint64_t), (override)); + MOCK_METHOD(Buffer::Reservation, reserve, (uint64_t preferred_length), (override)); + MOCK_METHOD(void, commit, (Buffer::Reservation & reservation, uint64_t length), (override)); MOCK_METHOD(ssize_t, search, (const void*, uint64_t, size_t, size_t), (const, override)); MOCK_METHOD(bool, startsWith, (absl::string_view), (const, override)); MOCK_METHOD(std::string, toString, (), (const, override)); From ae815f16ce438a6da3fcb3b5e7eb2bbbcc8df223 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Wed, 2 Dec 2020 10:19:14 -0800 Subject: [PATCH 02/46] honor high watermark Signed-off-by: Greg Greenway --- source/common/buffer/watermark_buffer.cc | 23 +++++++++++++++++++++++ source/common/buffer/watermark_buffer.h | 1 + source/common/common/utility.h | 22 ++++++++++++++++++++-- test/common/common/utility_test.cc | 20 ++++++++++++++++++++ 4 files changed, 64 insertions(+), 2 deletions(-) diff --git a/source/common/buffer/watermark_buffer.cc b/source/common/buffer/watermark_buffer.cc index ed28bac75265..73aacfbc9eee 100644 --- a/source/common/buffer/watermark_buffer.cc +++ b/source/common/buffer/watermark_buffer.cc @@ -62,6 +62,29 @@ SliceDataPtr WatermarkBuffer::extractMutableFrontSlice() { return result; } +// Adjust the reservation size based on space available before hitting +// the high watermark to avoid overshooting by a lot and thus violating the limits +// the watermark is imposing. +Reservation WatermarkBuffer::reserve(uint64_t preferred_length) { + uint64_t adjusted_length = preferred_length; + + if (high_watermark_ > 0 && preferred_length > 0) { + const uint64_t current_length = OwnedImpl::length(); + if (current_length >= high_watermark_) { + // Always allow a read of at least some data. The API doesn't allow returning + // a zero-length reservation. + adjusted_length = OwnedSlice::default_slice_size_; + } else { + const uint64_t available_length = high_watermark_ - current_length; + adjusted_length = std::min(available_length, preferred_length); + adjusted_length = + IntUtil::roundUpToMultiple(adjusted_length, OwnedSlice::default_slice_size_); + } + } + + return OwnedImpl::reserve(adjusted_length); +} + uint64_t WatermarkBuffer::reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iovecs) { uint64_t bytes_reserved = OwnedImpl::reserve(length, iovecs, num_iovecs); checkHighAndOverflowWatermarks(); diff --git a/source/common/buffer/watermark_buffer.h b/source/common/buffer/watermark_buffer.h index d975b4dc6cb2..e52b02003694 100644 --- a/source/common/buffer/watermark_buffer.h +++ b/source/common/buffer/watermark_buffer.h @@ -36,6 +36,7 @@ class WatermarkBuffer : public OwnedImpl { void move(Instance& rhs, uint64_t length) override; SliceDataPtr extractMutableFrontSlice() override; uint64_t reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iovecs) override; + Reservation reserve(uint64_t preferred_length) override; void commit(Reservation& reservation, uint64_t length) override; void postProcess() override { checkLowWatermark(); } void appendSliceForTest(const void* data, uint64_t size) override; diff --git a/source/common/common/utility.h b/source/common/common/utility.h index e4eba5b37117..30adb9f5bba1 100644 --- a/source/common/common/utility.h +++ b/source/common/common/utility.h @@ -179,6 +179,24 @@ class DateUtil { static uint64_t nowToMilliseconds(TimeSource& time_source); }; +/** + * Utility routines for working with integers. + */ +class IntUtil { +public: + /** + * Round `val` up to the next multiple. Examples: + * roundUpToMultiple(3, 8) -> 8 + * roundUpToMultiple(9, 8) -> 16 + * roundUpToMultiple(8, 8) -> 8 + */ + static uint64_t roundUpToMultiple(uint64_t val, uint32_t multiple) { + ASSERT(multiple > 0); + ASSERT((val + multiple) >= val, "Unsigned overflow"); + return ((val + multiple - 1) / multiple) * multiple; + } +}; + /** * Utility routines for working with strings. */ @@ -619,8 +637,8 @@ template struct TrieLookupTable { } /** - * Finds the entry associated with the longest prefix. Complexity is O(min(longest key prefix, key - * length)) + * Finds the entry associated with the longest prefix. Complexity is O(min(longest key prefix, + * key length)). * @param key the key used to find. * @return the value matching the longest prefix based on the key. */ diff --git a/test/common/common/utility_test.cc b/test/common/common/utility_test.cc index 6f4f8a2a628b..2a3936a03fbf 100644 --- a/test/common/common/utility_test.cc +++ b/test/common/common/utility_test.cc @@ -27,6 +27,26 @@ using testing::Not; namespace Envoy { +TEST(IntUtil, roundUpToMultiple) { + // Round up to non-power-of-2 + EXPECT_EQ(3, IntUtil::roundUpToMultiple(1, 3)); + EXPECT_EQ(3, IntUtil::roundUpToMultiple(3, 3)); + EXPECT_EQ(6, IntUtil::roundUpToMultiple(4, 3)); + EXPECT_EQ(6, IntUtil::roundUpToMultiple(5, 3)); + EXPECT_EQ(6, IntUtil::roundUpToMultiple(6, 3)); + EXPECT_EQ(21, IntUtil::roundUpToMultiple(20, 3)); + EXPECT_EQ(21, IntUtil::roundUpToMultiple(21, 3)); + + // Round up to power-of-2 + EXPECT_EQ(0, IntUtil::roundUpToMultiple(0, 4)); + EXPECT_EQ(4, IntUtil::roundUpToMultiple(3, 4)); + EXPECT_EQ(4, IntUtil::roundUpToMultiple(4, 4)); + EXPECT_EQ(8, IntUtil::roundUpToMultiple(5, 4)); + EXPECT_EQ(8, IntUtil::roundUpToMultiple(8, 4)); + EXPECT_EQ(24, IntUtil::roundUpToMultiple(21, 4)); + EXPECT_EQ(24, IntUtil::roundUpToMultiple(24, 4)); +} + TEST(StringUtil, strtoull) { uint64_t out; const char* rest; From 6af8023570d5d499b38dfc7d8f5ccdaa7f5b1556 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Fri, 4 Dec 2020 10:10:37 -0800 Subject: [PATCH 03/46] fix test expectation Signed-off-by: Greg Greenway --- test/common/buffer/owned_impl_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/buffer/owned_impl_test.cc b/test/common/buffer/owned_impl_test.cc index c23fca2602f9..465cd287aefa 100644 --- a/test/common/buffer/owned_impl_test.cc +++ b/test/common/buffer/owned_impl_test.cc @@ -1166,7 +1166,7 @@ TEST_F(OwnedImplTest, ReserveZeroCommit) { ASSERT_EQ(os_sys_calls.close(pipe_fds[1]).rc_, 0); ASSERT_EQ(previous_length, buf.search(data.data(), rc, previous_length, 0)); EXPECT_EQ("bbbbb", buf.toString().substr(0, 5)); - expectSlices({{5, 0, 4096}, {1953, 2143, 4096}}, buf); + expectSlices({{5, 0, 4096}, {1953, 14431, 16384}}, buf); } TEST_F(OwnedImplTest, ReadReserveAndCommit) { From c8c9cce4d20c4b11e8d459c7686bcf498bba8a9b Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Fri, 4 Dec 2020 12:07:38 -0800 Subject: [PATCH 04/46] fix incorrect merge conflict resolution Signed-off-by: Greg Greenway --- source/extensions/transport_sockets/tls/ssl_socket.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/transport_sockets/tls/ssl_socket.cc b/source/extensions/transport_sockets/tls/ssl_socket.cc index 39d4914a23f0..1e06d7e6e80e 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.cc +++ b/source/extensions/transport_sockets/tls/ssl_socket.cc @@ -160,7 +160,7 @@ Network::IoResult SslSocket::doRead(Buffer::Instance& read_buffer) { reservation.commit(bytes_read_this_iteration); if (bytes_read_this_iteration > 0 && callbacks_->shouldDrainReadBuffer()) { - callbacks_->setReadBufferReady(); + callbacks_->setTransportSocketIsReadable(); keep_reading = false; } From a0b68b7a95345d27cfc2078129a914d3a212f199 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Fri, 4 Dec 2020 12:07:57 -0800 Subject: [PATCH 05/46] remove commented out code Signed-off-by: Greg Greenway --- source/extensions/transport_sockets/tls/ssl_socket.cc | 5 ----- 1 file changed, 5 deletions(-) diff --git a/source/extensions/transport_sockets/tls/ssl_socket.cc b/source/extensions/transport_sockets/tls/ssl_socket.cc index 1e06d7e6e80e..cc763d171781 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.cc +++ b/source/extensions/transport_sockets/tls/ssl_socket.cc @@ -120,12 +120,7 @@ Network::IoResult SslSocket::doRead(Buffer::Instance& read_buffer) { uint64_t bytes_read = 0; while (keep_reading) { uint64_t bytes_read_this_iteration = 0; - // We use 2 slices here so that we can use the remainder of an existing buffer chain element - // if there is extra space. 16K read is arbitrary and can be tuned later. Buffer::Reservation reservation = read_buffer.reserve(16384); - // Buffer::RawSlice slices[2]; - // uint64_t slices_to_commit = 0; - // uint64_t num_slices = read_buffer.reserve(16384, slices, 2); for (uint64_t i = 0; i < reservation.numSlices(); i++) { auto result = sslReadIntoSlice(reservation.slices()[i]); bytes_read_this_iteration += result.bytes_read_; From ff13426c6ebddbca1e6101170ae5faeeb688bb25 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Mon, 7 Dec 2020 15:05:24 -0800 Subject: [PATCH 06/46] Remove old reserve/commit API; migrate all uses to new API Signed-off-by: Greg Greenway --- include/envoy/buffer/buffer.h | 38 +-- source/common/buffer/buffer_impl.cc | 81 ++++-- source/common/buffer/buffer_impl.h | 5 +- source/common/buffer/watermark_buffer.cc | 8 +- source/common/buffer/watermark_buffer.h | 4 +- source/common/grpc/common.cc | 20 +- source/common/http/http2/metadata_encoder.cc | 10 +- source/common/network/utility.cc | 55 ++-- .../quiche/envoy_quic_client_stream.cc | 7 +- .../quiche/envoy_quic_server_stream.cc | 7 +- .../platform/quic_mem_slice_storage_impl.cc | 19 +- .../stat_sinks/common/statsd/statsd.cc | 19 +- .../stat_sinks/common/statsd/statsd.h | 2 +- .../lightstep/lightstep_tracer_impl.cc | 10 +- test/common/buffer/buffer_fuzz.cc | 49 ++-- test/common/buffer/buffer_speed_test.cc | 21 +- test/common/buffer/owned_impl_test.cc | 250 +++++++----------- test/common/buffer/watermark_buffer_test.cc | 22 +- .../postgres_proxy/postgres_decoder_test.cc | 5 +- .../transport_sockets/tls/ssl_socket_test.cc | 8 +- .../tls/tls_throughput_benchmark.cc | 17 +- .../socket_interface_integration_test.cc | 9 +- 22 files changed, 306 insertions(+), 360 deletions(-) diff --git a/include/envoy/buffer/buffer.h b/include/envoy/buffer/buffer.h index c2dbf9412dca..3ed4e0a2d472 100644 --- a/include/envoy/buffer/buffer.h +++ b/include/envoy/buffer/buffer.h @@ -131,16 +131,6 @@ class Instance { */ virtual void prepend(Instance& data) PURE; - /** - * Commit a set of slices originally obtained from reserve(). The number of slices should match - * the number obtained from reserve(). The size of each slice can also be altered. Commit must - * occur once following a reserve() without any mutating operations in between other than to the - * iovecs len_ fields. - * @param iovecs supplies the array of slices to commit. - * @param num_iovecs supplies the size of the slices array. - */ - virtual void commit(RawSlice* iovecs, uint64_t num_iovecs) PURE; - /** * Copy out a section of the buffer. * @param start supplies the buffer index to start copying from. @@ -210,11 +200,16 @@ class Instance { * @param num_iovecs supplies the size of the slices array. * @return the number of iovecs used to reserve the space. */ - virtual uint64_t reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iovecs) PURE; + // virtual uint64_t reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iovecs) PURE; virtual Reservation reserve(uint64_t preferred_length) PURE; + virtual Reservation reserveSingleSlice(uint64_t length, bool separate_slice = false) PURE; + +private: + friend Reservation; virtual void commit(Reservation& reservation, uint64_t length) PURE; +public: /** * Search for an occurrence of data within the buffer. * @param data supplies the data to search for. @@ -440,6 +435,7 @@ class Reservation final { } } RawSlice* slices() { return slices_.data(); } + const RawSlice* slices() const { return slices_.data(); } uint64_t numSlices() const { return slices_.size(); } void commit(uint64_t length) { buffer_.commit(*this, length); @@ -447,13 +443,23 @@ class Reservation final { owned_slices_.clear(); } - // private: - friend class Instance; + // Tuned to allow reads of 128k, using 16k slices, and including one partially-used + // initial slice from a previous allocation. + static constexpr uint32_t NUM_ELEMENTS_ = 9; + +private: Reservation(Instance& buffer) : buffer_(buffer) {} Instance& buffer_; - static constexpr uint32_t num_elements_ = 9; - absl::InlinedVector slices_; - absl::InlinedVector owned_slices_; + absl::InlinedVector slices_; + absl::InlinedVector owned_slices_; + +public: + // The following are for use only by implementations of Buffer. Because c++ + // doesn't allow inheritance of friendship, these are just trying to make + // misuse easy to spot in a code review. + static Reservation bufferImplUseOnlyConstruct(Instance& buffer) { return Reservation(buffer); } + decltype(slices_)& bufferImplUseOnlySlices() { return slices_; } + decltype(owned_slices_)& bufferImplUseOnlyOwnedSlices() { return owned_slices_; } }; } // namespace Buffer diff --git a/source/common/buffer/buffer_impl.cc b/source/common/buffer/buffer_impl.cc index 5a60bbde9cb1..af0d7b47aaa4 100644 --- a/source/common/buffer/buffer_impl.cc +++ b/source/common/buffer/buffer_impl.cc @@ -84,7 +84,7 @@ void OwnedImpl::prepend(Instance& data) { other.postProcess(); } -void OwnedImpl::commit(RawSlice* iovecs, uint64_t num_iovecs) { +/*void OwnedImpl::commit(RawSlice* iovecs, uint64_t num_iovecs) { if (num_iovecs == 0) { return; } @@ -123,7 +123,7 @@ void OwnedImpl::commit(RawSlice* iovecs, uint64_t num_iovecs) { } ASSERT(num_slices_committed > 0); -} + }*/ void OwnedImpl::copyOut(size_t start, uint64_t size, void* data) const { uint64_t bytes_to_skip = start; @@ -341,7 +341,7 @@ void OwnedImpl::move(Instance& rhs, uint64_t length) { other.postProcess(); } -uint64_t OwnedImpl::reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iovecs) { +/*uint64_t OwnedImpl::reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iovecs) { if (num_iovecs == 0 || length == 0) { return 0; } @@ -390,10 +390,10 @@ uint64_t OwnedImpl::reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iove ASSERT(num_slices_used <= num_iovecs); ASSERT(bytes_remaining == 0); return num_slices_used; -} + }*/ Reservation OwnedImpl::reserve(uint64_t length) { - Reservation reservation(*this); + Reservation reservation = Reservation::bufferImplUseOnlyConstruct(*this); if (length == 0) { return reservation; } @@ -411,44 +411,77 @@ Reservation OwnedImpl::reserve(uint64_t length) { auto& last_slice = slices_.back(); const uint64_t reservation_size = std::min(last_slice->reservableSize(), bytes_remaining); auto slice = last_slice->reserve(reservation_size); - reservation.slices_.push_back(slice); - reservation.owned_slices_.push_back(nullptr); + reservation.bufferImplUseOnlySlices().push_back(slice); + reservation.bufferImplUseOnlyOwnedSlices().push_back(nullptr); bytes_remaining -= slice.len_; } while (bytes_remaining != 0) { - const uint64_t size = OwnedSlice::default_slice_size_; + uint64_t size = OwnedSlice::default_slice_size_; + if (bytes_remaining > size && + (reservation.bufferImplUseOnlySlices().size() + 1) == reservation.NUM_ELEMENTS_) { + size = bytes_remaining; + } auto slice = OwnedSlice::create(size); - reservation.slices_.push_back(slice->reserve(size)); - reservation.owned_slices_.emplace_back(std::move(slice)); - bytes_remaining -= std::min(reservation.slices_.back().len_, bytes_remaining); + reservation.bufferImplUseOnlySlices().push_back(slice->reserve(size)); + reservation.bufferImplUseOnlyOwnedSlices().emplace_back(std::move(slice)); + bytes_remaining -= std::min(reservation.bufferImplUseOnlySlices().back().len_, bytes_remaining); } - ASSERT(reservation.slices_.size() <= reservation.num_elements_, + ASSERT(reservation.bufferImplUseOnlySlices().size() <= reservation.NUM_ELEMENTS_, "The absl::InlineVector should be sized so that out-of-line storage isn't needed."); - ASSERT(reservation.slices_.size() == reservation.owned_slices_.size()); + ASSERT(reservation.bufferImplUseOnlySlices().size() == + reservation.bufferImplUseOnlyOwnedSlices().size()); + return reservation; +} + +Reservation OwnedImpl::reserveSingleSlice(uint64_t length, bool separate_slice) { + Reservation reservation = Reservation::bufferImplUseOnlyConstruct(*this); + if (length == 0) { + return reservation; + } + + // Remove any empty slices at the end. + while (!slices_.empty() && slices_.back()->dataSize() == 0) { + slices_.pop_back(); + } + + // Check whether there are any empty slices with reservable space at the end of the buffer. + uint64_t reservable_size = + (separate_slice || slices_.empty()) ? 0 : slices_.back()->reservableSize(); + if (reservable_size >= length) { + auto& last_slice = slices_.back(); + auto slice = last_slice->reserve(length); + reservation.bufferImplUseOnlySlices().push_back(slice); + reservation.bufferImplUseOnlyOwnedSlices().push_back(nullptr); + } else { + auto slice = OwnedSlice::create(length); + reservation.bufferImplUseOnlySlices().push_back(slice->reserve(length)); + reservation.bufferImplUseOnlyOwnedSlices().emplace_back(std::move(slice)); + } + return reservation; } void OwnedImpl::commit(Reservation& reservation, uint64_t length) { - ASSERT(reservation.slices_.size() == reservation.owned_slices_.size()); + auto& slices = reservation.bufferImplUseOnlySlices(); + auto& owned_slices = reservation.bufferImplUseOnlyOwnedSlices(); + ASSERT(slices.size() == owned_slices.size()); uint64_t bytes_remaining = length; - for (uint32_t i = 0; i < reservation.slices_.size(); i++) { - ASSERT((reservation.owned_slices_[i] != nullptr) == - (dynamic_cast(reservation.owned_slices_[i].get()) != nullptr)); - std::unique_ptr owned_slice( - static_cast(reservation.owned_slices_[i].release())); + for (uint32_t i = 0; i < slices.size(); i++) { + ASSERT((owned_slices[i] != nullptr) == + (dynamic_cast(owned_slices[i].get()) != nullptr)); + std::unique_ptr owned_slice(static_cast(owned_slices[i].release())); if (bytes_remaining > 0) { if (owned_slice != nullptr) { slices_.emplace_back(std::move(owned_slice)); } - reservation.slices_[i].len_ = - std::min(reservation.slices_[i].len_, bytes_remaining); - bool success = slices_.back()->commit(reservation.slices_[i]); + slices[i].len_ = std::min(slices[i].len_, bytes_remaining); + bool success = slices_.back()->commit(slices[i]); ASSERT(success); - length_ += reservation.slices_[i].len_; - bytes_remaining -= reservation.slices_[i].len_; + length_ += slices[i].len_; + bytes_remaining -= slices[i].len_; } else { OwnedSlice::free(std::move(owned_slice)); } diff --git a/source/common/buffer/buffer_impl.h b/source/common/buffer/buffer_impl.h index f31c7911d32f..5f01f30a5521 100644 --- a/source/common/buffer/buffer_impl.h +++ b/source/common/buffer/buffer_impl.h @@ -587,7 +587,7 @@ class OwnedImpl : public LibEventInstance { void add(const Instance& data) override; void prepend(absl::string_view data) override; void prepend(Instance& data) override; - void commit(RawSlice* iovecs, uint64_t num_iovecs) override; + // void commit(RawSlice* iovecs, uint64_t num_iovecs) override; void commit(Reservation& reservation, uint64_t length) override; void copyOut(size_t start, uint64_t size, void* data) const override; void drain(uint64_t size) override; @@ -598,8 +598,9 @@ class OwnedImpl : public LibEventInstance { void* linearize(uint32_t size) override; void move(Instance& rhs) override; void move(Instance& rhs, uint64_t length) override; - uint64_t reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iovecs) override; + // uint64_t reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iovecs) override; Reservation reserve(uint64_t preferred_length) override; + Reservation reserveSingleSlice(uint64_t length, bool separate_slice = false) override; ssize_t search(const void* data, uint64_t size, size_t start, size_t length) const override; bool startsWith(absl::string_view data) const override; std::string toString() const override; diff --git a/source/common/buffer/watermark_buffer.cc b/source/common/buffer/watermark_buffer.cc index 73aacfbc9eee..eaabc1735639 100644 --- a/source/common/buffer/watermark_buffer.cc +++ b/source/common/buffer/watermark_buffer.cc @@ -31,10 +31,10 @@ void WatermarkBuffer::prepend(Instance& data) { checkHighAndOverflowWatermarks(); } -void WatermarkBuffer::commit(RawSlice* iovecs, uint64_t num_iovecs) { +/*void WatermarkBuffer::commit(RawSlice* iovecs, uint64_t num_iovecs) { OwnedImpl::commit(iovecs, num_iovecs); checkHighAndOverflowWatermarks(); -} + }*/ void WatermarkBuffer::commit(Reservation& reservation, uint64_t length) { OwnedImpl::commit(reservation, length); @@ -85,11 +85,11 @@ Reservation WatermarkBuffer::reserve(uint64_t preferred_length) { return OwnedImpl::reserve(adjusted_length); } -uint64_t WatermarkBuffer::reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iovecs) { +/*uint64_t WatermarkBuffer::reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iovecs) { uint64_t bytes_reserved = OwnedImpl::reserve(length, iovecs, num_iovecs); checkHighAndOverflowWatermarks(); return bytes_reserved; -} + }*/ void WatermarkBuffer::appendSliceForTest(const void* data, uint64_t size) { OwnedImpl::appendSliceForTest(data, size); diff --git a/source/common/buffer/watermark_buffer.h b/source/common/buffer/watermark_buffer.h index e52b02003694..cb4d8eaab54b 100644 --- a/source/common/buffer/watermark_buffer.h +++ b/source/common/buffer/watermark_buffer.h @@ -30,12 +30,12 @@ class WatermarkBuffer : public OwnedImpl { void add(const Instance& data) override; void prepend(absl::string_view data) override; void prepend(Instance& data) override; - void commit(RawSlice* iovecs, uint64_t num_iovecs) override; + // void commit(RawSlice* iovecs, uint64_t num_iovecs) override; void drain(uint64_t size) override; void move(Instance& rhs) override; void move(Instance& rhs, uint64_t length) override; SliceDataPtr extractMutableFrontSlice() override; - uint64_t reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iovecs) override; + // uint64_t reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iovecs) override; Reservation reserve(uint64_t preferred_length) override; void commit(Reservation& reservation, uint64_t length) override; void postProcess() override { checkLowWatermark(); } diff --git a/source/common/grpc/common.cc b/source/common/grpc/common.cc index 8b7551d8bea2..237647144632 100644 --- a/source/common/grpc/common.cc +++ b/source/common/grpc/common.cc @@ -131,11 +131,9 @@ Buffer::InstancePtr Common::serializeToGrpcFrame(const Protobuf::Message& messag Buffer::InstancePtr body(new Buffer::OwnedImpl()); const uint32_t size = message.ByteSize(); const uint32_t alloc_size = size + 5; - Buffer::RawSlice iovec; - body->reserve(alloc_size, &iovec, 1); - ASSERT(iovec.len_ >= alloc_size); - iovec.len_ = alloc_size; - uint8_t* current = reinterpret_cast(iovec.mem_); + Buffer::Reservation reservation = body->reserveSingleSlice(alloc_size); + ASSERT(reservation.slices()[0].len_ >= alloc_size); + uint8_t* current = reinterpret_cast(reservation.slices()[0].mem_); *current++ = 0; // flags const uint32_t nsize = htonl(size); std::memcpy(current, reinterpret_cast(&nsize), sizeof(uint32_t)); @@ -143,22 +141,20 @@ Buffer::InstancePtr Common::serializeToGrpcFrame(const Protobuf::Message& messag Protobuf::io::ArrayOutputStream stream(current, size, -1); Protobuf::io::CodedOutputStream codec_stream(&stream); message.SerializeWithCachedSizes(&codec_stream); - body->commit(&iovec, 1); + reservation.commit(alloc_size); return body; } Buffer::InstancePtr Common::serializeMessage(const Protobuf::Message& message) { auto body = std::make_unique(); const uint32_t size = message.ByteSize(); - Buffer::RawSlice iovec; - body->reserve(size, &iovec, 1); - ASSERT(iovec.len_ >= size); - iovec.len_ = size; - uint8_t* current = reinterpret_cast(iovec.mem_); + Buffer::Reservation reservation = body->reserveSingleSlice(size); + ASSERT(reservation.slices()[0].len_ >= size); + uint8_t* current = reinterpret_cast(reservation.slices()[0].mem_); Protobuf::io::ArrayOutputStream stream(current, size, -1); Protobuf::io::CodedOutputStream codec_stream(&stream); message.SerializeWithCachedSizes(&codec_stream); - body->commit(&iovec, 1); + reservation.commit(size); return body; } diff --git a/source/common/http/http2/metadata_encoder.cc b/source/common/http/http2/metadata_encoder.cc index 5e5a6970a872..2d38026216d5 100644 --- a/source/common/http/http2/metadata_encoder.cc +++ b/source/common/http/http2/metadata_encoder.cc @@ -62,18 +62,16 @@ bool MetadataEncoder::createHeaderBlockUsingNghttp2(const MetadataMap& metadata_ ENVOY_LOG(error, "Payload size {} exceeds the max bound.", buflen); return false; } - Buffer::RawSlice iovec; - payload_.reserve(buflen, &iovec, 1); - ASSERT(iovec.len_ >= buflen); + Buffer::Reservation reservation = payload_.reserveSingleSlice(buflen); + ASSERT(reservation.slices()[0].len_ >= buflen); // Creates payload using nghttp2. - uint8_t* buf = reinterpret_cast(iovec.mem_); + uint8_t* buf = reinterpret_cast(reservation.slices()[0].mem_); const ssize_t result = nghttp2_hd_deflate_hd(deflater_.get(), buf, buflen, nva.begin(), nvlen); RELEASE_ASSERT(result > 0, fmt::format("Failed to deflate metadata payload, with result {}.", result)); - iovec.len_ = result; - payload_.commit(&iovec, 1); + reservation.commit(result); return true; } diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index 05a73a0ebf66..210edfd21261 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -101,21 +101,14 @@ Api::IoCallUint64Result receiveMessage(uint64_t max_packet_size, Buffer::Instanc IoHandle::RecvMsgOutput& output, IoHandle& handle, const Address::Instance& local_address) { - Buffer::RawSlice slice; - const uint64_t num_slices = buffer->reserve(max_packet_size, &slice, 1); - ASSERT(num_slices == 1u); + Buffer::Reservation reservation = buffer->reserveSingleSlice(max_packet_size); + Api::IoCallUint64Result result = handle.recvmsg(reservation.slices(), reservation.numSlices(), + local_address.ip()->port(), output); - Api::IoCallUint64Result result = - handle.recvmsg(&slice, num_slices, local_address.ip()->port(), output); - - if (!result.ok()) { - return result; + if (result.ok()) { + reservation.commit(std::min(max_packet_size, static_cast(result.rc_))); } - // Adjust memory length and commit slice to buffer - slice.len_ = std::min(slice.len_, static_cast(result.rc_)); - buffer->commit(&slice, 1); - return result; } @@ -619,16 +612,32 @@ Api::IoCallUint64Result Utility::readFromSocket(IoHandle& handle, } if (handle.supportsMmsg()) { - const uint32_t num_packets_per_mmsg_call = 16u; - const uint32_t num_slices_per_packet = 1u; - absl::FixedArray buffers(num_packets_per_mmsg_call); + const auto max_packet_size = udp_packet_processor.maxPacketSize(); + + // Buffer::Reservation is always passed by value, and can only be constructed + // by Buffer::Instance::reserve(), so this is needed to keep a fixed array + // in which all elements are legally constructed. + // + // TODO(ggreenway) PERF: each reservation currently has inline storage for 18 pointers (144 + // bytes on 64-bit) because the reservation can hold several buffers. For a read of 16 packets, + // that is 2304 bytes. Figure out a way to make the reservation only hold 2 pointers for the + // `reserveSingleSlice()` case. + struct BufferAndReservation { + BufferAndReservation(uint64_t max_packet_size) + : buffer_(std::make_unique()), + reservation_(buffer_->reserveSingleSlice(max_packet_size, true)) {} + + Buffer::InstancePtr buffer_; + Buffer::Reservation reservation_; + }; + constexpr uint32_t num_packets_per_mmsg_call = 16u; + constexpr uint32_t num_slices_per_packet = 1u; + absl::InlinedVector buffers; RawSliceArrays slices(num_packets_per_mmsg_call, absl::FixedArray(num_slices_per_packet)); - for (uint32_t i = 0; i < num_packets_per_mmsg_call; ++i) { - buffers[i] = std::make_unique(); - const uint64_t num_slices = buffers[i]->reserve(udp_packet_processor.maxPacketSize(), - slices[i].data(), num_slices_per_packet); - ASSERT(num_slices == num_slices_per_packet); + for (uint32_t i = 0; i < num_packets_per_mmsg_call; i++) { + buffers.push_back(max_packet_size); + slices[i][0] = buffers[i].reservation_.slices()[0]; } IoHandle::RecvMsgOutput output(num_packets_per_mmsg_call, packets_dropped); @@ -650,11 +659,9 @@ Api::IoCallUint64Result Utility::readFromSocket(IoHandle& handle, ENVOY_LOG_MISC(debug, "Receive a packet with {} bytes from {}", msg_len, output.msg_[i].peer_address_->asString()); - // Adjust used memory length and commit slice to buffer - slice->len_ = std::min(slice->len_, static_cast(msg_len)); - buffers[i]->commit(slice, 1); + buffers[i].reservation_.commit(std::min(max_packet_size, static_cast(msg_len))); - passPayloadToProcessor(msg_len, std::move(buffers[i]), output.msg_[i].peer_address_, + passPayloadToProcessor(msg_len, std::move(buffers[i].buffer_), output.msg_[i].peer_address_, output.msg_[i].local_address_, udp_packet_processor, receive_time); } return result; diff --git a/source/extensions/quic_listeners/quiche/envoy_quic_client_stream.cc b/source/extensions/quic_listeners/quiche/envoy_quic_client_stream.cc index 79e442ea8628..af6e5a36c057 100644 --- a/source/extensions/quic_listeners/quiche/envoy_quic_client_stream.cc +++ b/source/extensions/quic_listeners/quiche/envoy_quic_client_stream.cc @@ -161,12 +161,7 @@ void EnvoyQuicClientStream::OnBodyAvailable() { int num_regions = GetReadableRegions(&iov, 1); ASSERT(num_regions > 0); size_t bytes_read = iov.iov_len; - Buffer::RawSlice slice; - buffer->reserve(bytes_read, &slice, 1); - ASSERT(slice.len_ >= bytes_read); - slice.len_ = bytes_read; - memcpy(slice.mem_, iov.iov_base, iov.iov_len); - buffer->commit(&slice, 1); + buffer->add(iov.iov_base, bytes_read); MarkConsumed(bytes_read); } ASSERT(buffer->length() == 0 || !end_stream_decoded_); diff --git a/source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc b/source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc index c7d25b8d47da..a52d9cbaf4ec 100644 --- a/source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc +++ b/source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc @@ -179,12 +179,7 @@ void EnvoyQuicServerStream::OnBodyAvailable() { int num_regions = GetReadableRegions(&iov, 1); ASSERT(num_regions > 0); size_t bytes_read = iov.iov_len; - Buffer::RawSlice slice; - buffer->reserve(bytes_read, &slice, 1); - ASSERT(slice.len_ >= bytes_read); - slice.len_ = bytes_read; - memcpy(slice.mem_, iov.iov_base, iov.iov_len); - buffer->commit(&slice, 1); + buffer->add(iov.iov_base, bytes_read); MarkConsumed(bytes_read); } diff --git a/source/extensions/quic_listeners/quiche/platform/quic_mem_slice_storage_impl.cc b/source/extensions/quic_listeners/quiche/platform/quic_mem_slice_storage_impl.cc index 1c2c63b39e56..fb436ac3a2f0 100644 --- a/source/extensions/quic_listeners/quiche/platform/quic_mem_slice_storage_impl.cc +++ b/source/extensions/quic_listeners/quiche/platform/quic_mem_slice_storage_impl.cc @@ -30,19 +30,14 @@ QuicMemSliceStorageImpl::QuicMemSliceStorageImpl(const iovec* iov, int iov_count size_t io_offset = 0; while (io_offset < write_len) { size_t slice_len = std::min(write_len - io_offset, max_slice_len); - Envoy::Buffer::RawSlice slice; - // Populate a temporary buffer instance and then move it to |buffer_|. This is necessary because - // consecutive reserve/commit can return addresses in same slice which violates the restriction - // of |max_slice_len| when ToSpan() is called. - Envoy::Buffer::OwnedImpl buffer; - uint16_t num_slice = buffer.reserve(slice_len, &slice, 1); - ASSERT(num_slice == 1); - QuicUtils::CopyToBuffer(iov, iov_count, io_offset, slice_len, static_cast(slice.mem_)); + + // Use a separate slice so that we do not violate the restriction of |max_slice_len| when + // ToSpan() is called. + Envoy::Buffer::Reservation reservation = buffer_.reserveSingleSlice(slice_len, true); + QuicUtils::CopyToBuffer(iov, iov_count, io_offset, slice_len, + static_cast(reservation.slices()[0].mem_)); io_offset += slice_len; - // OwnedImpl may return a slice longer than needed, trim it to requested length. - slice.len_ = slice_len; - buffer.commit(&slice, num_slice); - buffer_.move(buffer); + reservation.commit(slice_len); } } diff --git a/source/extensions/stat_sinks/common/statsd/statsd.cc b/source/extensions/stat_sinks/common/statsd/statsd.cc index ce39592a63bc..c310b9b1e395 100644 --- a/source/extensions/stat_sinks/common/statsd/statsd.cc +++ b/source/extensions/stat_sinks/common/statsd/statsd.cc @@ -188,12 +188,12 @@ TcpStatsdSink::TlsSink::~TlsSink() { void TcpStatsdSink::TlsSink::beginFlush(bool expect_empty_buffer) { ASSERT(!expect_empty_buffer || buffer_.length() == 0); ASSERT(current_slice_mem_ == nullptr); + ASSERT(!current_buffer_reservation_.has_value()); - uint64_t num_iovecs = buffer_.reserve(FLUSH_SLICE_SIZE_BYTES, ¤t_buffer_slice_, 1); - ASSERT(num_iovecs == 1); + current_buffer_reservation_.emplace(buffer_.reserveSingleSlice(FLUSH_SLICE_SIZE_BYTES)); - ASSERT(current_buffer_slice_.len_ >= FLUSH_SLICE_SIZE_BYTES); - current_slice_mem_ = reinterpret_cast(current_buffer_slice_.mem_); + ASSERT(current_buffer_reservation_->slices()[0].len_ >= FLUSH_SLICE_SIZE_BYTES); + current_slice_mem_ = reinterpret_cast(current_buffer_reservation_->slices()[0].mem_); } void TcpStatsdSink::TlsSink::commonFlush(const std::string& name, uint64_t value, char stat_type) { @@ -201,7 +201,7 @@ void TcpStatsdSink::TlsSink::commonFlush(const std::string& name, uint64_t value // 36 > 1 ("." after prefix) + 1 (":" after name) + 4 (postfix chars, e.g., "|ms\n") + 30 for // number (bigger than it will ever be) const uint32_t max_size = name.size() + parent_.getPrefix().size() + 36; - if (current_buffer_slice_.len_ - usedBuffer() < max_size) { + if (current_buffer_reservation_->slices()[0].len_ - usedBuffer() < max_size) { endFlush(false); beginFlush(false); } @@ -234,8 +234,9 @@ void TcpStatsdSink::TlsSink::flushGauge(const std::string& name, uint64_t value) void TcpStatsdSink::TlsSink::endFlush(bool do_write) { ASSERT(current_slice_mem_ != nullptr); - current_buffer_slice_.len_ = usedBuffer(); - buffer_.commit(¤t_buffer_slice_, 1); + ASSERT(current_buffer_reservation_.has_value()); + current_buffer_reservation_->commit(usedBuffer()); + current_buffer_reservation_.reset(); current_slice_mem_ = nullptr; if (do_write) { write(buffer_); @@ -305,7 +306,9 @@ void TcpStatsdSink::TlsSink::write(Buffer::Instance& buffer) { uint64_t TcpStatsdSink::TlsSink::usedBuffer() const { ASSERT(current_slice_mem_ != nullptr); - return current_slice_mem_ - reinterpret_cast(current_buffer_slice_.mem_); + ASSERT(current_buffer_reservation_.has_value()); + return current_slice_mem_ - + reinterpret_cast(current_buffer_reservation_->slices()[0].mem_); } } // namespace Statsd diff --git a/source/extensions/stat_sinks/common/statsd/statsd.h b/source/extensions/stat_sinks/common/statsd/statsd.h index b7eb8bfac627..80ee370f6090 100644 --- a/source/extensions/stat_sinks/common/statsd/statsd.h +++ b/source/extensions/stat_sinks/common/statsd/statsd.h @@ -135,7 +135,7 @@ class TcpStatsdSink : public Stats::Sink { Event::Dispatcher& dispatcher_; Network::ClientConnectionPtr connection_; Buffer::OwnedImpl buffer_; - Buffer::RawSlice current_buffer_slice_; + absl::optional current_buffer_reservation_; char* current_slice_mem_{}; }; diff --git a/source/extensions/tracers/lightstep/lightstep_tracer_impl.cc b/source/extensions/tracers/lightstep/lightstep_tracer_impl.cc index 1cb535deb6c5..a40aa0c923d5 100644 --- a/source/extensions/tracers/lightstep/lightstep_tracer_impl.cc +++ b/source/extensions/tracers/lightstep/lightstep_tracer_impl.cc @@ -24,12 +24,10 @@ namespace Lightstep { static void serializeGrpcMessage(const lightstep::BufferChain& buffer_chain, Buffer::Instance& body) { auto size = buffer_chain.num_bytes(); - Buffer::RawSlice iovec; - body.reserve(size, &iovec, 1); - ASSERT(iovec.len_ >= size); - iovec.len_ = size; - buffer_chain.CopyOut(static_cast(iovec.mem_), size); - body.commit(&iovec, 1); + Buffer::Reservation reservation = body.reserveSingleSlice(size); + ASSERT(reservation.slices()[0].len_ >= size); + buffer_chain.CopyOut(static_cast(reservation.slices()[0].mem_), size); + reservation.commit(size); Grpc::Common::prependGrpcFrameHeader(body); } diff --git a/test/common/buffer/buffer_fuzz.cc b/test/common/buffer/buffer_fuzz.cc index 68e3454344e5..8e46a0516a19 100644 --- a/test/common/buffer/buffer_fuzz.cc +++ b/test/common/buffer/buffer_fuzz.cc @@ -105,10 +105,10 @@ class StringBuffer : public Buffer::Instance { src.size_ = 0; } - void commit(Buffer::RawSlice* iovecs, uint64_t num_iovecs) override { + /*void commit(Buffer::RawSlice* iovecs, uint64_t num_iovecs) override { FUZZ_ASSERT(num_iovecs == 1); size_ += iovecs[0].len_; - } + }*/ void commit(Buffer::Reservation&, uint64_t length) override { size_ += length; @@ -151,20 +151,29 @@ class StringBuffer : public Buffer::Instance { src.size_ -= length; } - uint64_t reserve(uint64_t length, Buffer::RawSlice* iovecs, uint64_t num_iovecs) override { + /*uint64_t reserve(uint64_t length, Buffer::RawSlice* iovecs, uint64_t num_iovecs) override { FUZZ_ASSERT(num_iovecs > 0); FUZZ_ASSERT(start_ + size_ + length <= data_.size()); iovecs[0].mem_ = mutableEnd(); iovecs[0].len_ = length; return 1; - } + }*/ Buffer::Reservation reserve(uint64_t length) override { + return reserveSingleSlice(length, false); + } + + Buffer::Reservation reserveSingleSlice(uint64_t length, bool separate_slice) override { + ASSERT(!separate_slice); + FUZZ_ASSERT(start_ + size_ + length <= data_.size()); + + Buffer::Reservation reservation = Buffer::Reservation::bufferImplUseOnlyConstruct(*this); Buffer::RawSlice slice; - reserve(length, &slice, 1); + slice.mem_ = mutableEnd(); + slice.len_ = length; + reservation.bufferImplUseOnlySlices().push_back(slice); + reservation.bufferImplUseOnlyOwnedSlices().push_back(nullptr); - Buffer::Reservation reservation(*this); - reservation.slices_.push_back(slice); return reservation; } @@ -262,31 +271,13 @@ uint32_t bufferAction(Context& ctxt, char insert_value, uint32_t max_alloc, Buff if (reserve_length == 0) { break; } - constexpr uint32_t reserve_slices = 16; - Buffer::RawSlice slices[reserve_slices]; - const uint32_t allocated_slices = target_buffer.reserve(reserve_length, slices, reserve_slices); - uint32_t allocated_length = 0; - for (uint32_t i = 0; i < allocated_slices; ++i) { - ::memset(slices[i].mem_, insert_value, slices[i].len_); - allocated_length += slices[i].len_; + Buffer::Reservation reservation = target_buffer.reserve(reserve_length); + for (uint32_t i = 0; i < reservation.numSlices(); ++i) { + ::memset(reservation.slices()[i].mem_, insert_value, reservation.slices()[i].len_); } - FUZZ_ASSERT(reserve_length <= allocated_length); const uint32_t target_length = std::min(reserve_length, action.reserve_commit().commit_length()); - uint32_t shrink_length = allocated_length; - int32_t shrink_slice = allocated_slices - 1; - while (shrink_length > target_length) { - FUZZ_ASSERT(shrink_slice >= 0); - const uint32_t available = slices[shrink_slice].len_; - const uint32_t remainder = shrink_length - target_length; - if (available >= remainder) { - slices[shrink_slice].len_ -= remainder; - break; - } - shrink_length -= available; - slices[shrink_slice--].len_ = 0; - } - target_buffer.commit(slices, allocated_slices); + reservation.commit(target_length); break; } case test::common::buffer::Action::kCopyOut: { diff --git a/test/common/buffer/buffer_speed_test.cc b/test/common/buffer/buffer_speed_test.cc index 49240c69f356..8952fa94d23d 100644 --- a/test/common/buffer/buffer_speed_test.cc +++ b/test/common/buffer/buffer_speed_test.cc @@ -207,14 +207,9 @@ BENCHMARK(bufferMovePartial)->Arg(1)->Arg(4096)->Arg(16384)->Arg(65536); static void bufferReserveCommit(benchmark::State& state) { Buffer::OwnedImpl buffer; for (auto _ : state) { - constexpr uint64_t NumSlices = 2; - Buffer::RawSlice slices[NumSlices]; - uint64_t slices_used = buffer.reserve(state.range(0), slices, NumSlices); - uint64_t bytes_to_commit = 0; - for (uint64_t i = 0; i < slices_used; i++) { - bytes_to_commit += static_cast(slices[i].len_); - } - buffer.commit(slices, slices_used); + auto size = state.range(0); + Buffer::Reservation reservation = buffer.reserve(size); + reservation.commit(size); if (buffer.length() >= MaxBufferLength) { buffer.drain(buffer.length()); } @@ -228,14 +223,10 @@ BENCHMARK(bufferReserveCommit)->Arg(1)->Arg(4096)->Arg(16384)->Arg(65536); static void bufferReserveCommitPartial(benchmark::State& state) { Buffer::OwnedImpl buffer; for (auto _ : state) { - constexpr uint64_t NumSlices = 2; - Buffer::RawSlice slices[NumSlices]; - uint64_t slices_used = buffer.reserve(state.range(0), slices, NumSlices); - ASSERT(slices_used > 0); + auto size = state.range(0); + Buffer::Reservation reservation = buffer.reserve(size); // Commit one byte from the first slice and nothing from any subsequent slice. - uint64_t bytes_to_commit = 1; - slices[0].len_ = bytes_to_commit; - buffer.commit(slices, 1); + reservation.commit(1); if (buffer.length() >= MaxBufferLength) { buffer.drain(buffer.length()); } diff --git a/test/common/buffer/owned_impl_test.cc b/test/common/buffer/owned_impl_test.cc index 465cd287aefa..2db110bc5504 100644 --- a/test/common/buffer/owned_impl_test.cc +++ b/test/common/buffer/owned_impl_test.cc @@ -24,17 +24,6 @@ class OwnedImplTest : public testing::Test { bool release_callback_called_ = false; protected: - static void clearReservation(Buffer::RawSlice* iovecs, uint64_t num_iovecs, OwnedImpl& buffer) { - for (uint64_t i = 0; i < num_iovecs; i++) { - iovecs[i].len_ = 0; - } - buffer.commit(iovecs, num_iovecs); - } - - static void commitReservation(Buffer::RawSlice* iovecs, uint64_t num_iovecs, OwnedImpl& buffer) { - buffer.commit(iovecs, num_iovecs); - } - static void expectSlices(std::vector> buffer_list, OwnedImpl& buffer) { const auto& buffer_slices = buffer.describeSlicesForTest(); ASSERT_EQ(buffer_list.size(), buffer_slices.size()); @@ -794,79 +783,79 @@ TEST_F(OwnedImplTest, ReserveCommit) { { Buffer::OwnedImpl buffer; - // A zero-byte reservation should fail. - static constexpr uint64_t NumIovecs = 16; - Buffer::RawSlice iovecs[NumIovecs]; - uint64_t num_reserved = buffer.reserve(0, iovecs, NumIovecs); - EXPECT_EQ(0, num_reserved); - clearReservation(iovecs, num_reserved, buffer); + // A zero-byte reservation should return an empty reservation. + { + auto reservation = buffer.reserve(0); + EXPECT_EQ(0, reservation.numSlices()); + } EXPECT_EQ(0, buffer.length()); // Test and commit a small reservation. This should succeed. - num_reserved = buffer.reserve(1, iovecs, NumIovecs); - EXPECT_EQ(1, num_reserved); - // The implementation might provide a bigger reservation than requested. - EXPECT_LE(1, iovecs[0].len_); - iovecs[0].len_ = 1; - commitReservation(iovecs, num_reserved, buffer); + { + auto reservation = buffer.reserve(1); + EXPECT_EQ(1, reservation.numSlices()); + EXPECT_EQ(16384, reservation.slices()[0].len_); + reservation.commit(1); + } EXPECT_EQ(1, buffer.length()); // Request a reservation that fits in the remaining space at the end of the last slice. - num_reserved = buffer.reserve(1, iovecs, NumIovecs); - EXPECT_EQ(1, num_reserved); - EXPECT_LE(1, iovecs[0].len_); - iovecs[0].len_ = 1; - const void* slice1 = iovecs[0].mem_; - clearReservation(iovecs, num_reserved, buffer); + const void* slice1; + { + auto reservation = buffer.reserve(1); + EXPECT_EQ(1, reservation.numSlices()); + EXPECT_EQ(1, reservation.slices()[0].len_); + slice1 = reservation.slices()[0].mem_; + } // Request a reservation that is too large to fit in the remaining space at the end of // the last slice, and allow the buffer to use only one slice. This should result in the // creation of a new slice within the buffer. - num_reserved = buffer.reserve(4096, iovecs, 1); - EXPECT_EQ(1, num_reserved); - EXPECT_NE(slice1, iovecs[0].mem_); - clearReservation(iovecs, num_reserved, buffer); + { + auto reservation = buffer.reserveSingleSlice(16384); + EXPECT_EQ(1, reservation.numSlices()); + EXPECT_EQ(16384, reservation.slices()[0].len_); + EXPECT_NE(slice1, reservation.slices()[0].mem_); + } // Request the same size reservation, but allow the buffer to use multiple slices. This // should result in the buffer creating a second slice and splitting the reservation between the // last two slices. - num_reserved = buffer.reserve(4096, iovecs, NumIovecs); - EXPECT_EQ(2, num_reserved); - EXPECT_EQ(slice1, iovecs[0].mem_); - clearReservation(iovecs, num_reserved, buffer); + { + auto reservation = buffer.reserve(16384); + EXPECT_EQ(2, reservation.numSlices()); + EXPECT_EQ(slice1, reservation.slices()[0].mem_); + } // Request a reservation that too big to fit in the existing slices. This should result // in the creation of a third slice. - expectSlices({{1, 4095, 4096}}, buffer); - buffer.reserve(4096, iovecs, NumIovecs); - expectSlices({{1, 4095, 4096}, {0, 4096, 4096}}, buffer); - const void* slice2 = iovecs[1].mem_; - num_reserved = buffer.reserve(8192, iovecs, NumIovecs); - expectSlices({{1, 4095, 4096}, {0, 4096, 4096}, {0, 4096, 4096}}, buffer); - EXPECT_EQ(3, num_reserved); - EXPECT_EQ(slice1, iovecs[0].mem_); - EXPECT_EQ(slice2, iovecs[1].mem_); - clearReservation(iovecs, num_reserved, buffer); + { + expectSlices({{1, 16383, 16384}}, buffer); + auto reservation = buffer.reserve(32768); + EXPECT_EQ(3, reservation.numSlices()); + EXPECT_EQ(16383, reservation.slices()[0].len_); + EXPECT_EQ(16384, reservation.slices()[1].len_); + EXPECT_EQ(16384, reservation.slices()[2].len_); + } // Append a fragment to the buffer, and then request a small reservation. The buffer // should make a new slice to satisfy the reservation; it cannot safely use any of // the previously seen slices, because they are no longer at the end of the buffer. - expectSlices({{1, 4095, 4096}}, buffer); - buffer.addBufferFragment(fragment); - EXPECT_EQ(13, buffer.length()); - num_reserved = buffer.reserve(1, iovecs, NumIovecs); - expectSlices({{1, 4095, 4096}, {12, 0, 12}, {0, 4096, 4096}}, buffer); - EXPECT_EQ(1, num_reserved); - EXPECT_NE(slice1, iovecs[0].mem_); - commitReservation(iovecs, num_reserved, buffer); + { + expectSlices({{1, 16383, 16384}}, buffer); + buffer.addBufferFragment(fragment); + EXPECT_EQ(13, buffer.length()); + auto reservation = buffer.reserve(1); + EXPECT_NE(slice1, reservation.slices()[0].mem_); + reservation.commit(1); + expectSlices({{1, 16383, 16384}, {12, 0, 12}, {1, 16383, 16384}}, buffer); + } EXPECT_EQ(14, buffer.length()); } } TEST_F(OwnedImplTest, ReserveCommitReuse) { Buffer::OwnedImpl buffer; - static constexpr uint64_t NumIovecs = 2; - Buffer::RawSlice iovecs[NumIovecs]; // Reserve 8KB and commit all but a few bytes of it, to ensure that // the last slice of the buffer can hold part but not all of the @@ -874,108 +863,79 @@ TEST_F(OwnedImplTest, ReserveCommitReuse) { // allocate more than the requested 8KB. In case the implementation // uses a power-of-two allocator, the subsequent reservations all // request 16KB. - uint64_t num_reserved = buffer.reserve(8200, iovecs, NumIovecs); - EXPECT_EQ(1, num_reserved); - iovecs[0].len_ = 8000; - buffer.commit(iovecs, 1); + { + auto reservation = buffer.reserveSingleSlice(8200); + reservation.commit(8000); + } EXPECT_EQ(8000, buffer.length()); // Reserve 16KB. The resulting reservation should span 2 slices. // Commit part of the first slice and none of the second slice. - num_reserved = buffer.reserve(16384, iovecs, NumIovecs); - EXPECT_EQ(2, num_reserved); - const void* first_slice = iovecs[0].mem_; - iovecs[0].len_ = 1; - expectSlices({{8000, 4288, 12288}, {0, 12288, 12288}}, buffer); - buffer.commit(iovecs, 1); + const void* first_slice; + { + auto reservation = buffer.reserve(16384); + expectSlices({{8000, 4288, 12288}}, buffer); + first_slice = reservation.slices()[0].mem_; + + EXPECT_EQ(2, reservation.numSlices()); + reservation.commit(1); + } EXPECT_EQ(8001, buffer.length()); - EXPECT_EQ(first_slice, iovecs[0].mem_); // The second slice is now released because there's nothing in the second slice. expectSlices({{8001, 4287, 12288}}, buffer); // Reserve 16KB again. - num_reserved = buffer.reserve(16384, iovecs, NumIovecs); - expectSlices({{8001, 4287, 12288}, {0, 12288, 12288}}, buffer); - EXPECT_EQ(2, num_reserved); - EXPECT_EQ(static_cast(first_slice) + 1, - static_cast(iovecs[0].mem_)); + { + auto reservation = buffer.reserve(16384); + EXPECT_EQ(2, reservation.numSlices()); + EXPECT_EQ(static_cast(first_slice) + 1, + static_cast(reservation.slices()[0].mem_)); + } + expectSlices({{8001, 4287, 12288}}, buffer); } TEST_F(OwnedImplTest, ReserveReuse) { Buffer::OwnedImpl buffer; - static constexpr uint64_t NumIovecs = 2; - Buffer::RawSlice iovecs[NumIovecs]; // Reserve some space and leave it uncommitted. - uint64_t num_reserved = buffer.reserve(8200, iovecs, NumIovecs); - EXPECT_EQ(1, num_reserved); - const void* first_slice = iovecs[0].mem_; - - // Reserve more space and verify that it begins with the same slice from the last reservation. - num_reserved = buffer.reserve(16384, iovecs, NumIovecs); - EXPECT_EQ(2, num_reserved); - EXPECT_EQ(first_slice, iovecs[0].mem_); - const void* second_slice = iovecs[1].mem_; - - // Repeat the last reservation and verify that it yields the same slices. - num_reserved = buffer.reserve(16384, iovecs, NumIovecs); - EXPECT_EQ(2, num_reserved); - EXPECT_EQ(first_slice, iovecs[0].mem_); - EXPECT_EQ(second_slice, iovecs[1].mem_); - expectSlices({{0, 12288, 12288}, {0, 4096, 4096}}, buffer); - - // Request a larger reservation, verify that the second entry is replaced with a block with a - // larger size. - num_reserved = buffer.reserve(30000, iovecs, NumIovecs); - const void* third_slice = iovecs[1].mem_; - EXPECT_EQ(2, num_reserved); - EXPECT_EQ(first_slice, iovecs[0].mem_); - EXPECT_EQ(12288, iovecs[0].len_); - EXPECT_NE(second_slice, iovecs[1].mem_); - EXPECT_EQ(30000 - iovecs[0].len_, iovecs[1].len_); - expectSlices({{0, 12288, 12288}, {0, 4096, 4096}, {0, 20480, 20480}}, buffer); - - // Repeating a the reservation request for a smaller block returns the previous entry. - num_reserved = buffer.reserve(16384, iovecs, NumIovecs); - EXPECT_EQ(2, num_reserved); - EXPECT_EQ(first_slice, iovecs[0].mem_); - EXPECT_EQ(second_slice, iovecs[1].mem_); - expectSlices({{0, 12288, 12288}, {0, 4096, 4096}, {0, 20480, 20480}}, buffer); - - // Repeat the larger reservation notice that it doesn't match the prior reservation for 30000 - // bytes. - num_reserved = buffer.reserve(30000, iovecs, NumIovecs); - EXPECT_EQ(2, num_reserved); - EXPECT_EQ(first_slice, iovecs[0].mem_); - EXPECT_EQ(12288, iovecs[0].len_); - EXPECT_NE(second_slice, iovecs[1].mem_); - EXPECT_NE(third_slice, iovecs[1].mem_); - EXPECT_EQ(30000 - iovecs[0].len_, iovecs[1].len_); - expectSlices({{0, 12288, 12288}, {0, 4096, 4096}, {0, 20480, 20480}, {0, 20480, 20480}}, buffer); - - // Commit the most recent reservation and verify the representation. - buffer.commit(iovecs, num_reserved); - expectSlices({{12288, 0, 12288}, {0, 4096, 4096}, {0, 20480, 20480}, {17712, 2768, 20480}}, - buffer); + const void* first_slice; + { + auto reservation = buffer.reserveSingleSlice(8200); + EXPECT_EQ(1, reservation.numSlices()); + first_slice = reservation.slices()[0].mem_; + } + + // Reserve more space and verify that it is not the same slice from the last reservation. + // That one was for a different size and was not committed. + const void* second_slice; + { + auto reservation = buffer.reserveSingleSlice(OwnedSlice::default_slice_size_); + EXPECT_EQ(1, reservation.numSlices()); + EXPECT_NE(first_slice, reservation.slices()[0].mem_); + second_slice = reservation.slices()[0].mem_; + } + + // Repeat the last reservation and verify that it yields the same slices. Both slices + // are for the default size, so the previous one should get re-used. + { + auto reservation = buffer.reserveSingleSlice(OwnedSlice::default_slice_size_); + EXPECT_EQ(1, reservation.numSlices()); + EXPECT_EQ(second_slice, reservation.slices()[0].mem_); + + // Commit the most recent reservation and verify the representation. + reservation.commit(OwnedSlice::default_slice_size_); + expectSlices({{16384, 0, 16384}}, buffer); + } // Do another reservation. - num_reserved = buffer.reserve(16384, iovecs, NumIovecs); - EXPECT_EQ(2, num_reserved); - expectSlices({{12288, 0, 12288}, - {0, 4096, 4096}, - {0, 20480, 20480}, - {17712, 2768, 20480}, - {0, 16384, 16384}}, - buffer); + { + auto reservation = buffer.reserveSingleSlice(OwnedSlice::default_slice_size_); + expectSlices({{16384, 0, 16384}}, buffer); - // And commit. - buffer.commit(iovecs, num_reserved); - expectSlices({{12288, 0, 12288}, - {0, 4096, 4096}, - {0, 20480, 20480}, - {20480, 0, 20480}, - {13616, 2768, 16384}}, - buffer); + // And commit. + reservation.commit(OwnedSlice::default_slice_size_); + expectSlices({{16384, 0, 16384}, {16384, 0, 16384}}, buffer); + } } TEST_F(OwnedImplTest, Search) { @@ -1139,13 +1099,7 @@ TEST_F(OwnedImplTest, ReserveZeroCommit) { buf.addBufferFragment(frag); buf.prepend("bbbbb"); buf.add(""); - constexpr uint32_t reserve_slices = 16; - Buffer::RawSlice slices[reserve_slices]; - const uint32_t allocated_slices = buf.reserve(1280, slices, reserve_slices); - for (uint32_t i = 0; i < allocated_slices; ++i) { - slices[i].len_ = 0; - } - buf.commit(slices, allocated_slices); + { auto reservation = buf.reserve(1280); } os_fd_t pipe_fds[2] = {0, 0}; auto& os_sys_calls = Api::OsSysCallsSingleton::get(); #ifdef WIN32 diff --git a/test/common/buffer/watermark_buffer_test.cc b/test/common/buffer/watermark_buffer_test.cc index 1ce8149f3dac..4e2367f51eca 100644 --- a/test/common/buffer/watermark_buffer_test.cc +++ b/test/common/buffer/watermark_buffer_test.cc @@ -121,11 +121,8 @@ TEST_F(WatermarkBufferTest, PrependBuffer) { TEST_F(WatermarkBufferTest, Commit) { buffer_.add(TEN_BYTES, 10); EXPECT_EQ(0, times_high_watermark_called_); - RawSlice out; - buffer_.reserve(10, &out, 1); - memcpy(out.mem_, &TEN_BYTES[0], 10); - out.len_ = 10; - buffer_.commit(&out, 1); + Buffer::Reservation reservation = buffer_.reserve(10); + reservation.commit(10); EXPECT_EQ(1, times_high_watermark_called_); EXPECT_EQ(20, buffer_.length()); } @@ -474,10 +471,8 @@ TEST_F(WatermarkBufferTest, OverflowWatermarkDisabledOnVeryHighValue) { const uint32_t segment_denominator = 128; const uint32_t big_segment_len = std::numeric_limits::max() / segment_denominator + 1; for (uint32_t i = 0; i < segment_denominator; ++i) { - Buffer::RawSlice iovecs[2]; - uint64_t num_reserved = buffer1.reserve(big_segment_len, iovecs, 2); - EXPECT_GE(num_reserved, 1); - buffer1.commit(iovecs, num_reserved); + Buffer::Reservation reservation = buffer1.reserveSingleSlice(big_segment_len); + reservation.commit(big_segment_len); } EXPECT_GT(buffer1.length(), std::numeric_limits::max()); EXPECT_LT(buffer1.length(), high_watermark_threshold * overflow_multiplier); @@ -486,12 +481,9 @@ TEST_F(WatermarkBufferTest, OverflowWatermarkDisabledOnVeryHighValue) { // Reserve and commit additional space on the buffer beyond the expected // high_watermark_threshold * overflow_multiplier threshold. - // Adding high_watermark_threshold * overflow_multiplier - buffer1.length() + 1 bytes - Buffer::RawSlice iovecs[2]; - uint64_t num_reserved = buffer1.reserve( - high_watermark_threshold * overflow_multiplier - buffer1.length() + 1, iovecs, 2); - EXPECT_GE(num_reserved, 1); - buffer1.commit(iovecs, num_reserved); + const uint64_t size = high_watermark_threshold * overflow_multiplier - buffer1.length() + 1; + Buffer::Reservation reservation = buffer1.reserve(size); + reservation.commit(size); EXPECT_EQ(buffer1.length(), high_watermark_threshold * overflow_multiplier + 1); EXPECT_EQ(1, high_watermark_buffer1); EXPECT_EQ(0, overflow_watermark_buffer1); diff --git a/test/extensions/filters/network/postgres_proxy/postgres_decoder_test.cc b/test/extensions/filters/network/postgres_proxy/postgres_decoder_test.cc index 11b11025bb81..c8bbe271395b 100644 --- a/test/extensions/filters/network/postgres_proxy/postgres_decoder_test.cc +++ b/test/extensions/filters/network/postgres_proxy/postgres_decoder_test.cc @@ -519,7 +519,6 @@ class FakeBuffer : public Buffer::Instance { MOCK_METHOD(void, add, (const Instance&), (override)); MOCK_METHOD(void, prepend, (absl::string_view), (override)); MOCK_METHOD(void, prepend, (Instance&), (override)); - MOCK_METHOD(void, commit, (Buffer::RawSlice*, uint64_t), (override)); MOCK_METHOD(void, copyOut, (size_t, uint64_t, void*), (const, override)); MOCK_METHOD(void, drain, (uint64_t), (override)); MOCK_METHOD(Buffer::RawSliceVector, getRawSlices, (absl::optional), (const, override)); @@ -529,8 +528,8 @@ class FakeBuffer : public Buffer::Instance { MOCK_METHOD(void*, linearize, (uint32_t), (override)); MOCK_METHOD(void, move, (Instance&), (override)); MOCK_METHOD(void, move, (Instance&, uint64_t), (override)); - MOCK_METHOD(uint64_t, reserve, (uint64_t, Buffer::RawSlice*, uint64_t), (override)); - MOCK_METHOD(Buffer::Reservation, reserve, (uint64_t preferred_length), (override)); + MOCK_METHOD(Buffer::Reservation, reserve, (uint64_t), (override)); + MOCK_METHOD(Buffer::Reservation, reserveSingleSlice, (uint64_t, bool), (override)); MOCK_METHOD(void, commit, (Buffer::Reservation & reservation, uint64_t length), (override)); MOCK_METHOD(ssize_t, search, (const void*, uint64_t, size_t, size_t), (const, override)); MOCK_METHOD(bool, startsWith, (absl::string_view), (const, override)); diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index 3d4279edd82e..0e68460cc551 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -4881,13 +4881,9 @@ class SslReadBufferLimitTest : public SslSocketTest { for (uint32_t i = 0; i < num_writes; i++) { Buffer::OwnedImpl data(std::string(write_size, 'a')); - // Incredibly contrived way of making sure that the write buffer has an empty chain in it. if (reserve_write_space) { - Buffer::RawSlice iovecs[2]; - EXPECT_EQ(2UL, data.reserve(16384, iovecs, 2)); - iovecs[0].len_ = 0; - iovecs[1].len_ = 0; - data.commit(iovecs, 2); + data.appendSliceForTest(absl::string_view()); + ASSERT_EQ(0, data.describeSlicesForTest().back().data); } client_connection_->write(data, false); diff --git a/test/extensions/transport_sockets/tls/tls_throughput_benchmark.cc b/test/extensions/transport_sockets/tls/tls_throughput_benchmark.cc index 19ec111bebad..348486c65e5b 100644 --- a/test/extensions/transport_sockets/tls/tls_throughput_benchmark.cc +++ b/test/extensions/transport_sockets/tls/tls_throughput_benchmark.cc @@ -33,18 +33,16 @@ static void handleSslError(SSL* ssl, int err, bool is_server) { } static void appendSlice(Buffer::Instance& buffer, uint32_t size) { - Buffer::RawSlice slice; std::string data(size, 'a'); RELEASE_ASSERT(data.size() <= 16384, "short_slice_size can't be larger than full slice"); // A 16kb request currently has inline metadata, which makes it 16384+8. This gets rounded up // to the next page size. Request enough that there is no extra space, to ensure that this results // in a new slice. - buffer.reserve(16384, &slice, 1); + Buffer::Reservation reservation = buffer.reserveSingleSlice(16384); - memcpy(slice.mem_, data.data(), data.size()); - slice.len_ = data.size(); - buffer.commit(&slice, 1); + memcpy(reservation.slices()[0].mem_, data.data(), data.size()); + reservation.commit(data.size()); } // If move_slices is true, add full-sized slices using move similar to how HTTP codecs move data @@ -56,12 +54,11 @@ static void addFullSlices(Buffer::Instance& output_buffer, int num_slices, bool for (int i = 0; i < num_slices; i++) { auto start_size = buffer->length(); - Buffer::RawSlice slices[2]; - auto num_slices = buffer->reserve(16384, slices, 2); - for (unsigned i = 0; i < num_slices; i++) { - memset(slices[i].mem_, 'a', slices[i].len_); + Buffer::Reservation reservation = buffer->reserve(16384); + for (unsigned i = 0; i < reservation.numSlices(); i++) { + memset(reservation.slices()[i].mem_, 'a', reservation.slices()[i].len_); } - buffer->commit(slices, num_slices); + reservation.commit(16384); RELEASE_ASSERT(buffer->length() - start_size == 16384, "correct reserve/commit"); } diff --git a/test/integration/socket_interface_integration_test.cc b/test/integration/socket_interface_integration_test.cc index 93a5db5aff84..7acef28630d8 100644 --- a/test/integration/socket_interface_integration_test.cc +++ b/test/integration/socket_interface_integration_test.cc @@ -135,13 +135,12 @@ TEST_P(SocketInterfaceIntegrationTest, UdpSendToInternalAddressWithSocketInterfa std::make_unique(Network::Socket::Type::Datagram, local_valid_address); Buffer::OwnedImpl buffer; - Buffer::RawSlice iovec; - buffer.reserve(100, &iovec, 1); + Buffer::Reservation reservation = buffer.reserve(100); - auto result = - socket->ioHandle().sendmsg(&iovec, 1, 0, local_valid_address->ip(), *peer_internal_address); + auto result = socket->ioHandle().sendmsg(reservation.slices(), reservation.numSlices(), 0, + local_valid_address->ip(), *peer_internal_address); ASSERT_FALSE(result.ok()); ASSERT_EQ(result.err_->getErrorCode(), Api::IoError::IoErrorCode::NoSupport); } } // namespace -} // namespace Envoy \ No newline at end of file +} // namespace Envoy From c0661ebf2f275c87505e3c4c03be4cb6f3401abe Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Mon, 7 Dec 2020 16:01:16 -0800 Subject: [PATCH 07/46] don't increase size of preferred length in watermark code trying to limit it Signed-off-by: Greg Greenway --- source/common/buffer/watermark_buffer.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/buffer/watermark_buffer.cc b/source/common/buffer/watermark_buffer.cc index eaabc1735639..5a126d55768f 100644 --- a/source/common/buffer/watermark_buffer.cc +++ b/source/common/buffer/watermark_buffer.cc @@ -76,9 +76,9 @@ Reservation WatermarkBuffer::reserve(uint64_t preferred_length) { adjusted_length = OwnedSlice::default_slice_size_; } else { const uint64_t available_length = high_watermark_ - current_length; - adjusted_length = std::min(available_length, preferred_length); adjusted_length = IntUtil::roundUpToMultiple(adjusted_length, OwnedSlice::default_slice_size_); + adjusted_length = std::min(available_length, preferred_length); } } From 6e9f1aa19373fa0d47856d329283d487d9a2f0a5 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Thu, 10 Dec 2020 15:17:47 -0800 Subject: [PATCH 08/46] api comments; delete old code Signed-off-by: Greg Greenway --- include/envoy/buffer/buffer.h | 48 +++++++++++-- source/common/buffer/buffer_impl.cc | 92 ------------------------ source/common/buffer/buffer_impl.h | 4 +- source/common/buffer/watermark_buffer.cc | 6 -- source/common/buffer/watermark_buffer.h | 2 - 5 files changed, 43 insertions(+), 109 deletions(-) diff --git a/include/envoy/buffer/buffer.h b/include/envoy/buffer/buffer.h index 2f2084f4c99e..8eb1434382b7 100644 --- a/include/envoy/buffer/buffer.h +++ b/include/envoy/buffer/buffer.h @@ -195,18 +195,29 @@ class Instance { /** * Reserve space in the buffer. - * @param length supplies the amount of space to reserve. - * @param iovecs supplies the slices to fill with reserved memory. - * @param num_iovecs supplies the size of the slices array. - * @return the number of iovecs used to reserve the space. + * @param preferred_length the suggested length to reserve. The actual + * reserved size may be differ based on heuristics to maximize performance. + * @return a `Reservation`, on which `commit()` can be called, or which can + * be destructed to discard any resources in the `Reservation`. */ - // virtual uint64_t reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iovecs) PURE; - virtual Reservation reserve(uint64_t preferred_length) PURE; + + /** + * Reserve space in the buffer in a single slice. + * @param length the exact length of the reservation. + * @param separate_slice specifies whether the reserved space must be in a separate slice + * from any other data in this buffer. + * @return a `Reservation` which has exactly one slice in it. + */ virtual Reservation reserveSingleSlice(uint64_t length, bool separate_slice = false) PURE; private: friend Reservation; + + /** + * Called by a `Reservation` to commit `length` bytes of the + * reservation. + */ virtual void commit(Reservation& reservation, uint64_t length) PURE; public: @@ -449,14 +460,31 @@ using WatermarkFactoryPtr = std::unique_ptr; class Reservation final { public: Reservation(Reservation&&) = default; + ~Reservation() { if (!slices_.empty()) { buffer_.commit(*this, 0); } } + + /** + * @return an array of `RawSlice` of length `numSlices()`. + */ RawSlice* slices() { return slices_.data(); } const RawSlice* slices() const { return slices_.data(); } + + /** + * @return the number of slices present. + */ uint64_t numSlices() const { return slices_.size(); } + + /** + * Commits some or all of the data in the reservation. + * @param length supplies the number of bytes to commit. This must be + * less than or equal to the size of the `Reservation`. + * + * @note No other methods should be called on the object after `commit()` is called. + */ void commit(uint64_t length) { buffer_.commit(*this, length); slices_.clear(); @@ -469,8 +497,16 @@ class Reservation final { private: Reservation(Instance& buffer) : buffer_(buffer) {} + + // The buffer that created this `Reservation`. Instance& buffer_; + + // The RawSlices in the reservation, usable by operations such as `::readv()`. absl::InlinedVector slices_; + + // An array of the same length as `slices_`, in which each element is either nullptr + // if no operation needs to be performed to transfer ownership during commit/destructor, + // or a pointer to the object that needs to be moved/deleted. absl::InlinedVector owned_slices_; public: diff --git a/source/common/buffer/buffer_impl.cc b/source/common/buffer/buffer_impl.cc index db210625df12..53460a848be0 100644 --- a/source/common/buffer/buffer_impl.cc +++ b/source/common/buffer/buffer_impl.cc @@ -83,47 +83,6 @@ void OwnedImpl::prepend(Instance& data) { other.postProcess(); } -/*void OwnedImpl::commit(RawSlice* iovecs, uint64_t num_iovecs) { - if (num_iovecs == 0) { - return; - } - if (slices_.empty()) { - return; - } - // Find the slices in the buffer that correspond to the iovecs: - // First, scan backward from the end of the buffer to find the last slice containing - // any content. Reservations are made from the end of the buffer, and out-of-order commits - // aren't supported, so any slices before this point cannot match the iovecs being committed. - ssize_t slice_index = static_cast(slices_.size()) - 1; - while (slice_index >= 0 && slices_[slice_index].dataSize() == 0) { - slice_index--; - } - if (slice_index < 0) { - // There was no slice containing any data, so rewind the iterator at the first slice. - slice_index = 0; - } - - // Next, scan forward and attempt to match the slices against iovecs. - uint64_t num_slices_committed = 0; - while (num_slices_committed < num_iovecs) { - if (slices_[slice_index].commit(iovecs[num_slices_committed])) { - length_ += iovecs[num_slices_committed].len_; - num_slices_committed++; - } - slice_index++; - if (slice_index == static_cast(slices_.size())) { - break; - } - } - - // In case an extra slice was reserved, remove empty slices from the end of the buffer. - while (!slices_.empty() && slices_.back().dataSize() == 0) { - slices_.pop_back(); - } - - ASSERT(num_slices_committed > 0); - }*/ - void OwnedImpl::copyOut(size_t start, uint64_t size, void* data) const { uint64_t bytes_to_skip = start; uint8_t* dest = static_cast(data); @@ -340,57 +299,6 @@ void OwnedImpl::move(Instance& rhs, uint64_t length) { other.postProcess(); } -/*uint64_t OwnedImpl::reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iovecs) { - if (num_iovecs == 0 || length == 0) { - return 0; - } - // Check whether there are any empty slices with reservable space at the end of the buffer. - size_t first_reservable_slice = slices_.size(); - while (first_reservable_slice > 0) { - if (slices_[first_reservable_slice - 1].reservableSize() == 0) { - break; - } - first_reservable_slice--; - if (slices_[first_reservable_slice].dataSize() != 0) { - // There is some content in this slice, so anything in front of it is non-reservable. - break; - } - } - - // Having found the sequence of reservable slices at the back of the buffer, reserve - // as much space as possible from each one. - uint64_t num_slices_used = 0; - uint64_t bytes_remaining = length; - size_t slice_index = first_reservable_slice; - while (slice_index < slices_.size() && bytes_remaining != 0 && num_slices_used < num_iovecs) { - auto& slice = slices_[slice_index]; - const uint64_t reservation_size = std::min(slice.reservableSize(), bytes_remaining); - if (num_slices_used + 1 == num_iovecs && reservation_size < bytes_remaining) { - // There is only one iovec left, and this next slice does not have enough space to - // complete the reservation. Stop iterating, with last one iovec still unpopulated, - // so the code following this loop can allocate a new slice to hold the rest of the - // reservation. - break; - } - iovecs[num_slices_used] = slice.reserve(reservation_size); - bytes_remaining -= iovecs[num_slices_used].len_; - num_slices_used++; - slice_index++; - } - - // If needed, allocate one more slice at the end to provide the remainder of the reservation. - if (bytes_remaining != 0) { - slices_.emplace_back(bytes_remaining); - iovecs[num_slices_used] = slices_.back().reserve(bytes_remaining); - bytes_remaining -= iovecs[num_slices_used].len_; - num_slices_used++; - } - - ASSERT(num_slices_used <= num_iovecs); - ASSERT(bytes_remaining == 0); - return num_slices_used; - }*/ - Reservation OwnedImpl::reserve(uint64_t length) { Reservation reservation = Reservation::bufferImplUseOnlyConstruct(*this); if (length == 0) { diff --git a/source/common/buffer/buffer_impl.h b/source/common/buffer/buffer_impl.h index ac7b68921e6f..c0742779af26 100644 --- a/source/common/buffer/buffer_impl.h +++ b/source/common/buffer/buffer_impl.h @@ -613,8 +613,6 @@ class OwnedImpl : public LibEventInstance { void add(const Instance& data) override; void prepend(absl::string_view data) override; void prepend(Instance& data) override; - // void commit(RawSlice* iovecs, uint64_t num_iovecs) override; - void commit(Reservation& reservation, uint64_t length) override; void copyOut(size_t start, uint64_t size, void* data) const override; void drain(uint64_t size) override; RawSliceVector getRawSlices(absl::optional max_slices = absl::nullopt) const override; @@ -624,9 +622,9 @@ class OwnedImpl : public LibEventInstance { void* linearize(uint32_t size) override; void move(Instance& rhs) override; void move(Instance& rhs, uint64_t length) override; - // uint64_t reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iovecs) override; Reservation reserve(uint64_t preferred_length) override; Reservation reserveSingleSlice(uint64_t length, bool separate_slice = false) override; + void commit(Reservation& reservation, uint64_t length) override; ssize_t search(const void* data, uint64_t size, size_t start, size_t length) const override; bool startsWith(absl::string_view data) const override; std::string toString() const override; diff --git a/source/common/buffer/watermark_buffer.cc b/source/common/buffer/watermark_buffer.cc index d4f056e09e87..d01a1bc0ed65 100644 --- a/source/common/buffer/watermark_buffer.cc +++ b/source/common/buffer/watermark_buffer.cc @@ -84,12 +84,6 @@ Reservation WatermarkBuffer::reserve(uint64_t preferred_length) { return OwnedImpl::reserve(adjusted_length); } -/*uint64_t WatermarkBuffer::reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iovecs) { - uint64_t bytes_reserved = OwnedImpl::reserve(length, iovecs, num_iovecs); - checkHighAndOverflowWatermarks(); - return bytes_reserved; - }*/ - void WatermarkBuffer::appendSliceForTest(const void* data, uint64_t size) { OwnedImpl::appendSliceForTest(data, size); checkHighAndOverflowWatermarks(); diff --git a/source/common/buffer/watermark_buffer.h b/source/common/buffer/watermark_buffer.h index 932a3db3df8d..2a47690af157 100644 --- a/source/common/buffer/watermark_buffer.h +++ b/source/common/buffer/watermark_buffer.h @@ -30,12 +30,10 @@ class WatermarkBuffer : public OwnedImpl { void add(const Instance& data) override; void prepend(absl::string_view data) override; void prepend(Instance& data) override; - // void commit(RawSlice* iovecs, uint64_t num_iovecs) override; void drain(uint64_t size) override; void move(Instance& rhs) override; void move(Instance& rhs, uint64_t length) override; SliceDataPtr extractMutableFrontSlice() override; - // uint64_t reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iovecs) override; Reservation reserve(uint64_t preferred_length) override; void commit(Reservation& reservation, uint64_t length) override; void postProcess() override { checkLowWatermark(); } From eb3fe0e900818cdfd4614ce50066ac14f592e56e Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Thu, 10 Dec 2020 16:02:58 -0800 Subject: [PATCH 09/46] readability cleanup Signed-off-by: Greg Greenway --- source/common/buffer/buffer_impl.cc | 33 +++++++++++++++-------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/source/common/buffer/buffer_impl.cc b/source/common/buffer/buffer_impl.cc index 53460a848be0..f581d5adc42d 100644 --- a/source/common/buffer/buffer_impl.cc +++ b/source/common/buffer/buffer_impl.cc @@ -311,6 +311,8 @@ Reservation OwnedImpl::reserve(uint64_t length) { } uint64_t bytes_remaining = length; + auto& reservation_slices = reservation.bufferImplUseOnlySlices(); + auto& reservation_owned_slices = reservation.bufferImplUseOnlyOwnedSlices(); // Check whether there are any empty slices with reservable space at the end of the buffer. uint64_t reservable_size = slices_.empty() ? 0 : slices_.back().reservableSize(); @@ -318,28 +320,25 @@ Reservation OwnedImpl::reserve(uint64_t length) { auto& last_slice = slices_.back(); const uint64_t reservation_size = std::min(last_slice.reservableSize(), bytes_remaining); auto slice = last_slice.reserve(reservation_size); - reservation.bufferImplUseOnlySlices().push_back(slice); - reservation.bufferImplUseOnlyOwnedSlices().push_back(nullptr); + reservation_slices.push_back(slice); + reservation_owned_slices.push_back(nullptr); bytes_remaining -= slice.len_; } while (bytes_remaining != 0) { uint64_t size = Slice::default_slice_size_; - if (bytes_remaining > size && - (reservation.bufferImplUseOnlySlices().size() + 1) == reservation.NUM_ELEMENTS_) { + if (bytes_remaining > size && (reservation_slices.size() + 1) == reservation.NUM_ELEMENTS_) { size = bytes_remaining; } Slice slice(size); - reservation.bufferImplUseOnlySlices().push_back(slice.reserve(size)); - reservation.bufferImplUseOnlyOwnedSlices().emplace_back( - std::make_unique(std::move(slice))); - bytes_remaining -= std::min(reservation.bufferImplUseOnlySlices().back().len_, bytes_remaining); + reservation_slices.push_back(slice.reserve(size)); + reservation_owned_slices.emplace_back(std::make_unique(std::move(slice))); + bytes_remaining -= std::min(reservation_slices.back().len_, bytes_remaining); } - ASSERT(reservation.bufferImplUseOnlySlices().size() <= reservation.NUM_ELEMENTS_, + ASSERT(reservation_slices.size() <= reservation.NUM_ELEMENTS_, "The absl::InlineVector should be sized so that out-of-line storage isn't needed."); - ASSERT(reservation.bufferImplUseOnlySlices().size() == - reservation.bufferImplUseOnlyOwnedSlices().size()); + ASSERT(reservation_slices.size() == reservation_owned_slices.size()); return reservation; } @@ -354,19 +353,21 @@ Reservation OwnedImpl::reserveSingleSlice(uint64_t length, bool separate_slice) slices_.pop_back(); } + auto& reservation_slices = reservation.bufferImplUseOnlySlices(); + auto& reservation_owned_slices = reservation.bufferImplUseOnlyOwnedSlices(); + // Check whether there are any empty slices with reservable space at the end of the buffer. uint64_t reservable_size = (separate_slice || slices_.empty()) ? 0 : slices_.back().reservableSize(); if (reservable_size >= length) { auto& last_slice = slices_.back(); auto slice = last_slice.reserve(length); - reservation.bufferImplUseOnlySlices().push_back(slice); - reservation.bufferImplUseOnlyOwnedSlices().push_back(nullptr); + reservation_slices.push_back(slice); + reservation_owned_slices.push_back(nullptr); } else { Slice slice(length); - reservation.bufferImplUseOnlySlices().push_back(slice.reserve(length)); - reservation.bufferImplUseOnlyOwnedSlices().emplace_back( - std::make_unique(std::move(slice))); + reservation_slices.push_back(slice.reserve(length)); + reservation_owned_slices.emplace_back(std::make_unique(std::move(slice))); } return reservation; From aff7e5352d2a1325ec5225fc2ec8abf452df116e Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Mon, 14 Dec 2020 13:21:51 -0800 Subject: [PATCH 10/46] change interface to have a different Reservation type for single-slice reservations Signed-off-by: Greg Greenway --- bazel/setup_clang.sh | 8 ++ include/envoy/buffer/buffer.h | 82 +++++++++++++++++-- source/common/buffer/buffer_impl.cc | 27 +++--- source/common/buffer/buffer_impl.h | 7 +- source/common/buffer/watermark_buffer.cc | 14 ++-- source/common/buffer/watermark_buffer.h | 5 +- source/common/grpc/common.cc | 12 +-- source/common/http/http2/metadata_encoder.cc | 6 +- .../common/network/io_socket_handle_impl.cc | 5 +- source/common/network/utility.cc | 17 ++-- .../platform/quic_mem_slice_storage_impl.cc | 4 +- .../stat_sinks/common/statsd/statsd.cc | 9 +- .../stat_sinks/common/statsd/statsd.h | 2 +- .../lightstep/lightstep_tracer_impl.cc | 6 +- .../transport_sockets/tls/ssl_socket.cc | 2 +- test/common/buffer/buffer_fuzz.cc | 44 +++++----- test/common/buffer/buffer_speed_test.cc | 4 +- test/common/buffer/owned_impl_test.cc | 34 ++++---- test/common/buffer/watermark_buffer_test.cc | 6 +- .../postgres_proxy/postgres_decoder_test.cc | 6 +- .../tls/tls_throughput_benchmark.cc | 6 +- 21 files changed, 185 insertions(+), 121 deletions(-) diff --git a/bazel/setup_clang.sh b/bazel/setup_clang.sh index d0e58478dc0a..bf92da6f8ffc 100755 --- a/bazel/setup_clang.sh +++ b/bazel/setup_clang.sh @@ -23,6 +23,14 @@ build:clang --repo_env='LLVM_CONFIG=${LLVM_PREFIX}/bin/llvm-config' build:clang --linkopt='-L$(llvm-config --libdir)' build:clang --linkopt='-Wl,-rpath,$(llvm-config --libdir)' +build:clang-asan --action_env='PATH=${PATH}' +build:clang-asan --action_env=CC=clang +build:clang-asan --action_env=CXX=clang++ +build:clang-asan --action_env='LLVM_CONFIG=${LLVM_PREFIX}/bin/llvm-config' +build:clang-asan --repo_env='LLVM_CONFIG=${LLVM_PREFIX}/bin/llvm-config' +build:clang-asan --linkopt='-L$(llvm-config --libdir)' +build:clang-asan --linkopt='-Wl,-rpath,$(llvm-config --libdir)' + build:clang-asan --action_env=ENVOY_UBSAN_VPTR=1 build:clang-asan --copt=-fsanitize=vptr,function build:clang-asan --linkopt=-fsanitize=vptr,function diff --git a/include/envoy/buffer/buffer.h b/include/envoy/buffer/buffer.h index 8eb1434382b7..b82a72e28202 100644 --- a/include/envoy/buffer/buffer.h +++ b/include/envoy/buffer/buffer.h @@ -73,6 +73,7 @@ class SliceData { using SliceDataPtr = std::unique_ptr; class Reservation; +class ReservationSingleSlice; /** * A basic buffer abstraction. @@ -200,7 +201,7 @@ class Instance { * @return a `Reservation`, on which `commit()` can be called, or which can * be destructed to discard any resources in the `Reservation`. */ - virtual Reservation reserve(uint64_t preferred_length) PURE; + virtual Reservation reserveApproximately(uint64_t preferred_length) PURE; /** * Reserve space in the buffer in a single slice. @@ -209,16 +210,19 @@ class Instance { * from any other data in this buffer. * @return a `Reservation` which has exactly one slice in it. */ - virtual Reservation reserveSingleSlice(uint64_t length, bool separate_slice = false) PURE; + virtual ReservationSingleSlice reserveSingleSlice(uint64_t length, + bool separate_slice = false) PURE; private: friend Reservation; + friend ReservationSingleSlice; /** * Called by a `Reservation` to commit `length` bytes of the * reservation. */ - virtual void commit(Reservation& reservation, uint64_t length) PURE; + virtual void commit(uint64_t length, absl::Span slices, + absl::Span owned_slices) PURE; public: /** @@ -463,7 +467,7 @@ class Reservation final { ~Reservation() { if (!slices_.empty()) { - buffer_.commit(*this, 0); + commit(0); } } @@ -478,6 +482,11 @@ class Reservation final { */ uint64_t numSlices() const { return slices_.size(); } + /** + * @return the total length of the Reservation. + */ + uint64_t length() const { return length_; } + /** * Commits some or all of the data in the reservation. * @param length supplies the number of bytes to commit. This must be @@ -486,7 +495,7 @@ class Reservation final { * @note No other methods should be called on the object after `commit()` is called. */ void commit(uint64_t length) { - buffer_.commit(*this, length); + buffer_.commit(length, absl::MakeSpan(slices_), absl::MakeSpan(owned_slices_)); slices_.clear(); owned_slices_.clear(); } @@ -501,6 +510,9 @@ class Reservation final { // The buffer that created this `Reservation`. Instance& buffer_; + // The combined length of all slices in the Reservation. + uint64_t length_; + // The RawSlices in the reservation, usable by operations such as `::readv()`. absl::InlinedVector slices_; @@ -516,6 +528,66 @@ class Reservation final { static Reservation bufferImplUseOnlyConstruct(Instance& buffer) { return Reservation(buffer); } decltype(slices_)& bufferImplUseOnlySlices() { return slices_; } decltype(owned_slices_)& bufferImplUseOnlyOwnedSlices() { return owned_slices_; } + void bufferImplUseOnlySetLength(uint64_t length) { length_ = length; } +}; + +/** + * Holds an in-progress addition to a buffer, holding only a single slice. + * + * @note For performance reasons, this class is passed by value to + * avoid an extra allocation, so it cannot have any virtual methods. + */ +class ReservationSingleSlice final { +public: + ReservationSingleSlice(ReservationSingleSlice&&) = default; + + ~ReservationSingleSlice() { + if (slice_.mem_ != nullptr) { + commit(0); + } + } + + /** + * @return an array of `RawSlice` of length `numSlices()`. + */ + RawSlice slice() const { return slice_; } + + /** + * Commits some or all of the data in the reservation. + * @param length supplies the number of bytes to commit. This must be + * less than or equal to the size of the `Reservation`. + * + * @note No other methods should be called on the object after `commit()` is called. + */ + void commit(uint64_t length) { + buffer_.commit(length, absl::MakeSpan(&slice_, 1), absl::MakeSpan(&owned_slice_, 1)); + slice_ = {nullptr, 0}; + owned_slice_.reset(); + } + +private: + ReservationSingleSlice(Instance& buffer) : buffer_(buffer) {} + + // The buffer that created this `Reservation`. + Instance& buffer_; + + // The RawSlice in the reservation, usable by anything needing the raw pointer + // and length to read into. + RawSlice slice_{}; + + // Contains either nullptr if no operation needs to be performed to transfer ownership during + // commit/destructor, or a pointer to the object that needs to be moved/deleted. + SliceDataPtr owned_slice_{}; + +public: + // The following are for use only by implementations of Buffer. Because c++ + // doesn't allow inheritance of friendship, these are just trying to make + // misuse easy to spot in a code review. + static ReservationSingleSlice bufferImplUseOnlyConstruct(Instance& buffer) { + return ReservationSingleSlice(buffer); + } + RawSlice& bufferImplUseOnlySlice() { return slice_; } + SliceDataPtr& bufferImplUseOnlyOwnedSlice() { return owned_slice_; } }; } // namespace Buffer diff --git a/source/common/buffer/buffer_impl.cc b/source/common/buffer/buffer_impl.cc index f581d5adc42d..f829dabb264b 100644 --- a/source/common/buffer/buffer_impl.cc +++ b/source/common/buffer/buffer_impl.cc @@ -299,7 +299,7 @@ void OwnedImpl::move(Instance& rhs, uint64_t length) { other.postProcess(); } -Reservation OwnedImpl::reserve(uint64_t length) { +Reservation OwnedImpl::reserveApproximately(uint64_t length) { Reservation reservation = Reservation::bufferImplUseOnlyConstruct(*this); if (length == 0) { return reservation; @@ -336,14 +336,16 @@ Reservation OwnedImpl::reserve(uint64_t length) { bytes_remaining -= std::min(reservation_slices.back().len_, bytes_remaining); } + reservation.bufferImplUseOnlySetLength(length); + ASSERT(reservation_slices.size() <= reservation.NUM_ELEMENTS_, "The absl::InlineVector should be sized so that out-of-line storage isn't needed."); ASSERT(reservation_slices.size() == reservation_owned_slices.size()); return reservation; } -Reservation OwnedImpl::reserveSingleSlice(uint64_t length, bool separate_slice) { - Reservation reservation = Reservation::bufferImplUseOnlyConstruct(*this); +ReservationSingleSlice OwnedImpl::reserveSingleSlice(uint64_t length, bool separate_slice) { + ReservationSingleSlice reservation = ReservationSingleSlice::bufferImplUseOnlyConstruct(*this); if (length == 0) { return reservation; } @@ -353,29 +355,26 @@ Reservation OwnedImpl::reserveSingleSlice(uint64_t length, bool separate_slice) slices_.pop_back(); } - auto& reservation_slices = reservation.bufferImplUseOnlySlices(); - auto& reservation_owned_slices = reservation.bufferImplUseOnlyOwnedSlices(); + auto& reservation_slice = reservation.bufferImplUseOnlySlice(); + auto& reservation_owned_slice = reservation.bufferImplUseOnlyOwnedSlice(); // Check whether there are any empty slices with reservable space at the end of the buffer. uint64_t reservable_size = (separate_slice || slices_.empty()) ? 0 : slices_.back().reservableSize(); if (reservable_size >= length) { - auto& last_slice = slices_.back(); - auto slice = last_slice.reserve(length); - reservation_slices.push_back(slice); - reservation_owned_slices.push_back(nullptr); + reservation_slice = slices_.back().reserve(length); + reservation_owned_slice = nullptr; } else { Slice slice(length); - reservation_slices.push_back(slice.reserve(length)); - reservation_owned_slices.emplace_back(std::make_unique(std::move(slice))); + reservation_slice = slice.reserve(length); + reservation_owned_slice = std::make_unique(std::move(slice)); } return reservation; } -void OwnedImpl::commit(Reservation& reservation, uint64_t length) { - auto& slices = reservation.bufferImplUseOnlySlices(); - auto& owned_slices = reservation.bufferImplUseOnlyOwnedSlices(); +void OwnedImpl::commit(uint64_t length, absl::Span slices, + absl::Span owned_slices) { ASSERT(slices.size() == owned_slices.size()); uint64_t bytes_remaining = length; for (uint32_t i = 0; i < slices.size(); i++) { diff --git a/source/common/buffer/buffer_impl.h b/source/common/buffer/buffer_impl.h index c0742779af26..f1d358c5624d 100644 --- a/source/common/buffer/buffer_impl.h +++ b/source/common/buffer/buffer_impl.h @@ -622,9 +622,10 @@ class OwnedImpl : public LibEventInstance { void* linearize(uint32_t size) override; void move(Instance& rhs) override; void move(Instance& rhs, uint64_t length) override; - Reservation reserve(uint64_t preferred_length) override; - Reservation reserveSingleSlice(uint64_t length, bool separate_slice = false) override; - void commit(Reservation& reservation, uint64_t length) override; + Reservation reserveApproximately(uint64_t preferred_length) override; + ReservationSingleSlice reserveSingleSlice(uint64_t length, bool separate_slice = false) override; + void commit(uint64_t length, absl::Span slices, + absl::Span owned_slices) override; ssize_t search(const void* data, uint64_t size, size_t start, size_t length) const override; bool startsWith(absl::string_view data) const override; std::string toString() const override; diff --git a/source/common/buffer/watermark_buffer.cc b/source/common/buffer/watermark_buffer.cc index d01a1bc0ed65..aed8ddc93063 100644 --- a/source/common/buffer/watermark_buffer.cc +++ b/source/common/buffer/watermark_buffer.cc @@ -31,13 +31,9 @@ void WatermarkBuffer::prepend(Instance& data) { checkHighAndOverflowWatermarks(); } -/*void WatermarkBuffer::commit(RawSlice* iovecs, uint64_t num_iovecs) { - OwnedImpl::commit(iovecs, num_iovecs); - checkHighAndOverflowWatermarks(); - }*/ - -void WatermarkBuffer::commit(Reservation& reservation, uint64_t length) { - OwnedImpl::commit(reservation, length); +void WatermarkBuffer::commit(uint64_t length, absl::Span slices, + absl::Span owned_slices) { + OwnedImpl::commit(length, slices, owned_slices); checkHighAndOverflowWatermarks(); } @@ -65,7 +61,7 @@ SliceDataPtr WatermarkBuffer::extractMutableFrontSlice() { // Adjust the reservation size based on space available before hitting // the high watermark to avoid overshooting by a lot and thus violating the limits // the watermark is imposing. -Reservation WatermarkBuffer::reserve(uint64_t preferred_length) { +Reservation WatermarkBuffer::reserveApproximately(uint64_t preferred_length) { uint64_t adjusted_length = preferred_length; if (high_watermark_ > 0 && preferred_length > 0) { @@ -81,7 +77,7 @@ Reservation WatermarkBuffer::reserve(uint64_t preferred_length) { } } - return OwnedImpl::reserve(adjusted_length); + return OwnedImpl::reserveApproximately(adjusted_length); } void WatermarkBuffer::appendSliceForTest(const void* data, uint64_t size) { diff --git a/source/common/buffer/watermark_buffer.h b/source/common/buffer/watermark_buffer.h index 2a47690af157..e8aeb4acec0a 100644 --- a/source/common/buffer/watermark_buffer.h +++ b/source/common/buffer/watermark_buffer.h @@ -34,8 +34,9 @@ class WatermarkBuffer : public OwnedImpl { void move(Instance& rhs) override; void move(Instance& rhs, uint64_t length) override; SliceDataPtr extractMutableFrontSlice() override; - Reservation reserve(uint64_t preferred_length) override; - void commit(Reservation& reservation, uint64_t length) override; + Reservation reserveApproximately(uint64_t preferred_length) override; + void commit(uint64_t length, absl::Span slices, + absl::Span owned_slices) override; void postProcess() override { checkLowWatermark(); } void appendSliceForTest(const void* data, uint64_t size) override; void appendSliceForTest(absl::string_view data) override; diff --git a/source/common/grpc/common.cc b/source/common/grpc/common.cc index 237647144632..549be3dbbc72 100644 --- a/source/common/grpc/common.cc +++ b/source/common/grpc/common.cc @@ -131,9 +131,9 @@ Buffer::InstancePtr Common::serializeToGrpcFrame(const Protobuf::Message& messag Buffer::InstancePtr body(new Buffer::OwnedImpl()); const uint32_t size = message.ByteSize(); const uint32_t alloc_size = size + 5; - Buffer::Reservation reservation = body->reserveSingleSlice(alloc_size); - ASSERT(reservation.slices()[0].len_ >= alloc_size); - uint8_t* current = reinterpret_cast(reservation.slices()[0].mem_); + auto reservation = body->reserveSingleSlice(alloc_size); + ASSERT(reservation.slice().len_ >= alloc_size); + uint8_t* current = reinterpret_cast(reservation.slice().mem_); *current++ = 0; // flags const uint32_t nsize = htonl(size); std::memcpy(current, reinterpret_cast(&nsize), sizeof(uint32_t)); @@ -148,9 +148,9 @@ Buffer::InstancePtr Common::serializeToGrpcFrame(const Protobuf::Message& messag Buffer::InstancePtr Common::serializeMessage(const Protobuf::Message& message) { auto body = std::make_unique(); const uint32_t size = message.ByteSize(); - Buffer::Reservation reservation = body->reserveSingleSlice(size); - ASSERT(reservation.slices()[0].len_ >= size); - uint8_t* current = reinterpret_cast(reservation.slices()[0].mem_); + auto reservation = body->reserveSingleSlice(size); + ASSERT(reservation.slice().len_ >= size); + uint8_t* current = reinterpret_cast(reservation.slice().mem_); Protobuf::io::ArrayOutputStream stream(current, size, -1); Protobuf::io::CodedOutputStream codec_stream(&stream); message.SerializeWithCachedSizes(&codec_stream); diff --git a/source/common/http/http2/metadata_encoder.cc b/source/common/http/http2/metadata_encoder.cc index 2d38026216d5..ac1c06af4f36 100644 --- a/source/common/http/http2/metadata_encoder.cc +++ b/source/common/http/http2/metadata_encoder.cc @@ -62,11 +62,11 @@ bool MetadataEncoder::createHeaderBlockUsingNghttp2(const MetadataMap& metadata_ ENVOY_LOG(error, "Payload size {} exceeds the max bound.", buflen); return false; } - Buffer::Reservation reservation = payload_.reserveSingleSlice(buflen); - ASSERT(reservation.slices()[0].len_ >= buflen); + auto reservation = payload_.reserveSingleSlice(buflen); + ASSERT(reservation.slice().len_ >= buflen); // Creates payload using nghttp2. - uint8_t* buf = reinterpret_cast(reservation.slices()[0].mem_); + uint8_t* buf = reinterpret_cast(reservation.slice().mem_); const ssize_t result = nghttp2_hd_deflate_hd(deflater_.get(), buf, buflen, nva.begin(), nvlen); RELEASE_ASSERT(result > 0, fmt::format("Failed to deflate metadata payload, with result {}.", result)); diff --git a/source/common/network/io_socket_handle_impl.cc b/source/common/network/io_socket_handle_impl.cc index 48701c5f6983..ddbc5fd3515b 100644 --- a/source/common/network/io_socket_handle_impl.cc +++ b/source/common/network/io_socket_handle_impl.cc @@ -112,8 +112,9 @@ Api::IoCallUint64Result IoSocketHandleImpl::read(Buffer::Instance& buffer, uint6 if (max_length == 0) { return Api::ioCallUint64ResultNoError(); } - Buffer::Reservation reservation = buffer.reserve(max_length); - Api::IoCallUint64Result result = readv(max_length, reservation.slices(), reservation.numSlices()); + Buffer::Reservation reservation = buffer.reserveApproximately(max_length); + Api::IoCallUint64Result result = + readv(reservation.length(), reservation.slices(), reservation.numSlices()); uint64_t bytes_to_commit = result.ok() ? result.rc_ : 0; ASSERT(bytes_to_commit <= max_length); reservation.commit(bytes_to_commit); diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index 210edfd21261..1c04cc75114d 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -101,9 +101,9 @@ Api::IoCallUint64Result receiveMessage(uint64_t max_packet_size, Buffer::Instanc IoHandle::RecvMsgOutput& output, IoHandle& handle, const Address::Instance& local_address) { - Buffer::Reservation reservation = buffer->reserveSingleSlice(max_packet_size); - Api::IoCallUint64Result result = handle.recvmsg(reservation.slices(), reservation.numSlices(), - local_address.ip()->port(), output); + auto reservation = buffer->reserveSingleSlice(max_packet_size); + Buffer::RawSlice slice = reservation.slice(); + Api::IoCallUint64Result result = handle.recvmsg(&slice, 1, local_address.ip()->port(), output); if (result.ok()) { reservation.commit(std::min(max_packet_size, static_cast(result.rc_))); @@ -614,21 +614,16 @@ Api::IoCallUint64Result Utility::readFromSocket(IoHandle& handle, if (handle.supportsMmsg()) { const auto max_packet_size = udp_packet_processor.maxPacketSize(); - // Buffer::Reservation is always passed by value, and can only be constructed + // Buffer::ReservationSingleSlice is always passed by value, and can only be constructed // by Buffer::Instance::reserve(), so this is needed to keep a fixed array // in which all elements are legally constructed. - // - // TODO(ggreenway) PERF: each reservation currently has inline storage for 18 pointers (144 - // bytes on 64-bit) because the reservation can hold several buffers. For a read of 16 packets, - // that is 2304 bytes. Figure out a way to make the reservation only hold 2 pointers for the - // `reserveSingleSlice()` case. struct BufferAndReservation { BufferAndReservation(uint64_t max_packet_size) : buffer_(std::make_unique()), reservation_(buffer_->reserveSingleSlice(max_packet_size, true)) {} Buffer::InstancePtr buffer_; - Buffer::Reservation reservation_; + Buffer::ReservationSingleSlice reservation_; }; constexpr uint32_t num_packets_per_mmsg_call = 16u; constexpr uint32_t num_slices_per_packet = 1u; @@ -637,7 +632,7 @@ Api::IoCallUint64Result Utility::readFromSocket(IoHandle& handle, absl::FixedArray(num_slices_per_packet)); for (uint32_t i = 0; i < num_packets_per_mmsg_call; i++) { buffers.push_back(max_packet_size); - slices[i][0] = buffers[i].reservation_.slices()[0]; + slices[i][0] = buffers[i].reservation_.slice(); } IoHandle::RecvMsgOutput output(num_packets_per_mmsg_call, packets_dropped); diff --git a/source/extensions/quic_listeners/quiche/platform/quic_mem_slice_storage_impl.cc b/source/extensions/quic_listeners/quiche/platform/quic_mem_slice_storage_impl.cc index fb436ac3a2f0..5105ec6815ab 100644 --- a/source/extensions/quic_listeners/quiche/platform/quic_mem_slice_storage_impl.cc +++ b/source/extensions/quic_listeners/quiche/platform/quic_mem_slice_storage_impl.cc @@ -33,9 +33,9 @@ QuicMemSliceStorageImpl::QuicMemSliceStorageImpl(const iovec* iov, int iov_count // Use a separate slice so that we do not violate the restriction of |max_slice_len| when // ToSpan() is called. - Envoy::Buffer::Reservation reservation = buffer_.reserveSingleSlice(slice_len, true); + auto reservation = buffer_.reserveSingleSlice(slice_len, true); QuicUtils::CopyToBuffer(iov, iov_count, io_offset, slice_len, - static_cast(reservation.slices()[0].mem_)); + static_cast(reservation.slice().mem_)); io_offset += slice_len; reservation.commit(slice_len); } diff --git a/source/extensions/stat_sinks/common/statsd/statsd.cc b/source/extensions/stat_sinks/common/statsd/statsd.cc index c310b9b1e395..268420698979 100644 --- a/source/extensions/stat_sinks/common/statsd/statsd.cc +++ b/source/extensions/stat_sinks/common/statsd/statsd.cc @@ -192,8 +192,8 @@ void TcpStatsdSink::TlsSink::beginFlush(bool expect_empty_buffer) { current_buffer_reservation_.emplace(buffer_.reserveSingleSlice(FLUSH_SLICE_SIZE_BYTES)); - ASSERT(current_buffer_reservation_->slices()[0].len_ >= FLUSH_SLICE_SIZE_BYTES); - current_slice_mem_ = reinterpret_cast(current_buffer_reservation_->slices()[0].mem_); + ASSERT(current_buffer_reservation_->slice().len_ >= FLUSH_SLICE_SIZE_BYTES); + current_slice_mem_ = reinterpret_cast(current_buffer_reservation_->slice().mem_); } void TcpStatsdSink::TlsSink::commonFlush(const std::string& name, uint64_t value, char stat_type) { @@ -201,7 +201,7 @@ void TcpStatsdSink::TlsSink::commonFlush(const std::string& name, uint64_t value // 36 > 1 ("." after prefix) + 1 (":" after name) + 4 (postfix chars, e.g., "|ms\n") + 30 for // number (bigger than it will ever be) const uint32_t max_size = name.size() + parent_.getPrefix().size() + 36; - if (current_buffer_reservation_->slices()[0].len_ - usedBuffer() < max_size) { + if (current_buffer_reservation_->slice().len_ - usedBuffer() < max_size) { endFlush(false); beginFlush(false); } @@ -307,8 +307,7 @@ void TcpStatsdSink::TlsSink::write(Buffer::Instance& buffer) { uint64_t TcpStatsdSink::TlsSink::usedBuffer() const { ASSERT(current_slice_mem_ != nullptr); ASSERT(current_buffer_reservation_.has_value()); - return current_slice_mem_ - - reinterpret_cast(current_buffer_reservation_->slices()[0].mem_); + return current_slice_mem_ - reinterpret_cast(current_buffer_reservation_->slice().mem_); } } // namespace Statsd diff --git a/source/extensions/stat_sinks/common/statsd/statsd.h b/source/extensions/stat_sinks/common/statsd/statsd.h index 80ee370f6090..6abc53bc0661 100644 --- a/source/extensions/stat_sinks/common/statsd/statsd.h +++ b/source/extensions/stat_sinks/common/statsd/statsd.h @@ -135,7 +135,7 @@ class TcpStatsdSink : public Stats::Sink { Event::Dispatcher& dispatcher_; Network::ClientConnectionPtr connection_; Buffer::OwnedImpl buffer_; - absl::optional current_buffer_reservation_; + absl::optional current_buffer_reservation_; char* current_slice_mem_{}; }; diff --git a/source/extensions/tracers/lightstep/lightstep_tracer_impl.cc b/source/extensions/tracers/lightstep/lightstep_tracer_impl.cc index a40aa0c923d5..31e9360d7025 100644 --- a/source/extensions/tracers/lightstep/lightstep_tracer_impl.cc +++ b/source/extensions/tracers/lightstep/lightstep_tracer_impl.cc @@ -24,9 +24,9 @@ namespace Lightstep { static void serializeGrpcMessage(const lightstep::BufferChain& buffer_chain, Buffer::Instance& body) { auto size = buffer_chain.num_bytes(); - Buffer::Reservation reservation = body.reserveSingleSlice(size); - ASSERT(reservation.slices()[0].len_ >= size); - buffer_chain.CopyOut(static_cast(reservation.slices()[0].mem_), size); + auto reservation = body.reserveSingleSlice(size); + ASSERT(reservation.slice().len_ >= size); + buffer_chain.CopyOut(static_cast(reservation.slice().mem_), size); reservation.commit(size); Grpc::Common::prependGrpcFrameHeader(body); } diff --git a/source/extensions/transport_sockets/tls/ssl_socket.cc b/source/extensions/transport_sockets/tls/ssl_socket.cc index cc763d171781..dbbb25f97f2d 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.cc +++ b/source/extensions/transport_sockets/tls/ssl_socket.cc @@ -120,7 +120,7 @@ Network::IoResult SslSocket::doRead(Buffer::Instance& read_buffer) { uint64_t bytes_read = 0; while (keep_reading) { uint64_t bytes_read_this_iteration = 0; - Buffer::Reservation reservation = read_buffer.reserve(16384); + Buffer::Reservation reservation = read_buffer.reserveApproximately(16384); for (uint64_t i = 0; i < reservation.numSlices(); i++) { auto result = sslReadIntoSlice(reservation.slices()[i]); bytes_read_this_iteration += result.bytes_read_; diff --git a/test/common/buffer/buffer_fuzz.cc b/test/common/buffer/buffer_fuzz.cc index c51efea94534..3c2594f1964a 100644 --- a/test/common/buffer/buffer_fuzz.cc +++ b/test/common/buffer/buffer_fuzz.cc @@ -105,16 +105,6 @@ class StringBuffer : public Buffer::Instance { src.size_ = 0; } - /*void commit(Buffer::RawSlice* iovecs, uint64_t num_iovecs) override { - FUZZ_ASSERT(num_iovecs == 1); - size_ += iovecs[0].len_; - }*/ - - void commit(Buffer::Reservation&, uint64_t length) override { - size_ += length; - FUZZ_ASSERT(start_ + size_ + length <= data_.size()); - } - void copyOut(size_t start, uint64_t size, void* data) const override { ::memcpy(data, this->start() + start, size); } @@ -151,32 +141,38 @@ class StringBuffer : public Buffer::Instance { src.size_ -= length; } - /*uint64_t reserve(uint64_t length, Buffer::RawSlice* iovecs, uint64_t num_iovecs) override { - FUZZ_ASSERT(num_iovecs > 0); + Buffer::Reservation reserveApproximately(uint64_t length) override { FUZZ_ASSERT(start_ + size_ + length <= data_.size()); - iovecs[0].mem_ = mutableEnd(); - iovecs[0].len_ = length; - return 1; - }*/ - Buffer::Reservation reserve(uint64_t length) override { - return reserveSingleSlice(length, false); + auto reservation = Buffer::Reservation::bufferImplUseOnlyConstruct(*this); + Buffer::RawSlice slice; + slice.mem_ = mutableEnd(); + slice.len_ = length; + reservation.bufferImplUseOnlySlices().push_back(slice); + reservation.bufferImplUseOnlyOwnedSlices().push_back(nullptr); + + return reservation; } - Buffer::Reservation reserveSingleSlice(uint64_t length, bool separate_slice) override { + Buffer::ReservationSingleSlice reserveSingleSlice(uint64_t length, bool separate_slice) override { ASSERT(!separate_slice); FUZZ_ASSERT(start_ + size_ + length <= data_.size()); - Buffer::Reservation reservation = Buffer::Reservation::bufferImplUseOnlyConstruct(*this); + auto reservation = Buffer::ReservationSingleSlice::bufferImplUseOnlyConstruct(*this); Buffer::RawSlice slice; slice.mem_ = mutableEnd(); slice.len_ = length; - reservation.bufferImplUseOnlySlices().push_back(slice); - reservation.bufferImplUseOnlyOwnedSlices().push_back(nullptr); + reservation.bufferImplUseOnlySlice() = slice; return reservation; } + void commit(uint64_t length, absl::Span, + absl::Span) override { + size_ += length; + FUZZ_ASSERT(start_ + size_ + length <= data_.size()); + } + ssize_t search(const void* data, uint64_t size, size_t start, size_t length) const override { UNREFERENCED_PARAMETER(length); return asStringView().find({static_cast(data), size}, start); @@ -280,12 +276,12 @@ uint32_t bufferAction(Context& ctxt, char insert_value, uint32_t max_alloc, Buff if (reserve_length == 0) { break; } - Buffer::Reservation reservation = target_buffer.reserve(reserve_length); + Buffer::Reservation reservation = target_buffer.reserveApproximately(reserve_length); for (uint32_t i = 0; i < reservation.numSlices(); ++i) { ::memset(reservation.slices()[i].mem_, insert_value, reservation.slices()[i].len_); } const uint32_t target_length = - std::min(reserve_length, action.reserve_commit().commit_length()); + std::min(reservation.length(), action.reserve_commit().commit_length()); reservation.commit(target_length); break; } diff --git a/test/common/buffer/buffer_speed_test.cc b/test/common/buffer/buffer_speed_test.cc index 8952fa94d23d..ae8537d9443c 100644 --- a/test/common/buffer/buffer_speed_test.cc +++ b/test/common/buffer/buffer_speed_test.cc @@ -208,7 +208,7 @@ static void bufferReserveCommit(benchmark::State& state) { Buffer::OwnedImpl buffer; for (auto _ : state) { auto size = state.range(0); - Buffer::Reservation reservation = buffer.reserve(size); + Buffer::Reservation reservation = buffer.reserveApproximately(size); reservation.commit(size); if (buffer.length() >= MaxBufferLength) { buffer.drain(buffer.length()); @@ -224,7 +224,7 @@ static void bufferReserveCommitPartial(benchmark::State& state) { Buffer::OwnedImpl buffer; for (auto _ : state) { auto size = state.range(0); - Buffer::Reservation reservation = buffer.reserve(size); + Buffer::Reservation reservation = buffer.reserveApproximately(size); // Commit one byte from the first slice and nothing from any subsequent slice. reservation.commit(1); if (buffer.length() >= MaxBufferLength) { diff --git a/test/common/buffer/owned_impl_test.cc b/test/common/buffer/owned_impl_test.cc index b5a3f32ca7f9..05835482636e 100644 --- a/test/common/buffer/owned_impl_test.cc +++ b/test/common/buffer/owned_impl_test.cc @@ -785,14 +785,14 @@ TEST_F(OwnedImplTest, ReserveCommit) { Buffer::OwnedImpl buffer; // A zero-byte reservation should return an empty reservation. { - auto reservation = buffer.reserve(0); + auto reservation = buffer.reserveApproximately(0); EXPECT_EQ(0, reservation.numSlices()); } EXPECT_EQ(0, buffer.length()); // Test and commit a small reservation. This should succeed. { - auto reservation = buffer.reserve(1); + auto reservation = buffer.reserveApproximately(1); EXPECT_EQ(1, reservation.numSlices()); EXPECT_EQ(16384, reservation.slices()[0].len_); reservation.commit(1); @@ -802,7 +802,7 @@ TEST_F(OwnedImplTest, ReserveCommit) { // Request a reservation that fits in the remaining space at the end of the last slice. const void* slice1; { - auto reservation = buffer.reserve(1); + auto reservation = buffer.reserveApproximately(1); EXPECT_EQ(1, reservation.numSlices()); EXPECT_EQ(1, reservation.slices()[0].len_); slice1 = reservation.slices()[0].mem_; @@ -813,16 +813,15 @@ TEST_F(OwnedImplTest, ReserveCommit) { // creation of a new slice within the buffer. { auto reservation = buffer.reserveSingleSlice(16384); - EXPECT_EQ(1, reservation.numSlices()); - EXPECT_EQ(16384, reservation.slices()[0].len_); - EXPECT_NE(slice1, reservation.slices()[0].mem_); + EXPECT_EQ(16384, reservation.slice().len_); + EXPECT_NE(slice1, reservation.slice().mem_); } // Request the same size reservation, but allow the buffer to use multiple slices. This // should result in the buffer creating a second slice and splitting the reservation between the // last two slices. { - auto reservation = buffer.reserve(16384); + auto reservation = buffer.reserveApproximately(16384); EXPECT_EQ(2, reservation.numSlices()); EXPECT_EQ(slice1, reservation.slices()[0].mem_); } @@ -831,7 +830,7 @@ TEST_F(OwnedImplTest, ReserveCommit) { // in the creation of a third slice. { expectSlices({{1, 16383, 16384}}, buffer); - auto reservation = buffer.reserve(32768); + auto reservation = buffer.reserveApproximately(32768); EXPECT_EQ(3, reservation.numSlices()); EXPECT_EQ(16383, reservation.slices()[0].len_); EXPECT_EQ(16384, reservation.slices()[1].len_); @@ -845,7 +844,7 @@ TEST_F(OwnedImplTest, ReserveCommit) { expectSlices({{1, 16383, 16384}}, buffer); buffer.addBufferFragment(fragment); EXPECT_EQ(13, buffer.length()); - auto reservation = buffer.reserve(1); + auto reservation = buffer.reserveApproximately(1); EXPECT_NE(slice1, reservation.slices()[0].mem_); reservation.commit(1); expectSlices({{1, 16383, 16384}, {12, 0, 12}, {1, 16383, 16384}}, buffer); @@ -873,7 +872,7 @@ TEST_F(OwnedImplTest, ReserveCommitReuse) { // Commit part of the first slice and none of the second slice. const void* first_slice; { - auto reservation = buffer.reserve(16384); + auto reservation = buffer.reserveApproximately(16384); expectSlices({{8000, 4288, 12288}}, buffer); first_slice = reservation.slices()[0].mem_; @@ -886,7 +885,7 @@ TEST_F(OwnedImplTest, ReserveCommitReuse) { // Reserve 16KB again. { - auto reservation = buffer.reserve(16384); + auto reservation = buffer.reserveApproximately(16384); EXPECT_EQ(2, reservation.numSlices()); EXPECT_EQ(static_cast(first_slice) + 1, static_cast(reservation.slices()[0].mem_)); @@ -901,8 +900,7 @@ TEST_F(OwnedImplTest, ReserveReuse) { const void* first_slice; { auto reservation = buffer.reserveSingleSlice(8200); - EXPECT_EQ(1, reservation.numSlices()); - first_slice = reservation.slices()[0].mem_; + first_slice = reservation.slice().mem_; } // Reserve more space and verify that it is not the same slice from the last reservation. @@ -910,17 +908,15 @@ TEST_F(OwnedImplTest, ReserveReuse) { const void* second_slice; { auto reservation = buffer.reserveSingleSlice(Slice::default_slice_size_); - EXPECT_EQ(1, reservation.numSlices()); - EXPECT_NE(first_slice, reservation.slices()[0].mem_); - second_slice = reservation.slices()[0].mem_; + EXPECT_NE(first_slice, reservation.slice().mem_); + second_slice = reservation.slice().mem_; } // Repeat the last reservation and verify that it yields the same slices. Both slices // are for the default size, so the previous one should get re-used. { auto reservation = buffer.reserveSingleSlice(Slice::default_slice_size_); - EXPECT_EQ(1, reservation.numSlices()); - EXPECT_EQ(second_slice, reservation.slices()[0].mem_); + EXPECT_EQ(second_slice, reservation.slice().mem_); // Commit the most recent reservation and verify the representation. reservation.commit(Slice::default_slice_size_); @@ -1099,7 +1095,7 @@ TEST_F(OwnedImplTest, ReserveZeroCommit) { buf.addBufferFragment(frag); buf.prepend("bbbbb"); buf.add(""); - { auto reservation = buf.reserve(1280); } + { auto reservation = buf.reserveApproximately(1280); } os_fd_t pipe_fds[2] = {0, 0}; auto& os_sys_calls = Api::OsSysCallsSingleton::get(); #ifdef WIN32 diff --git a/test/common/buffer/watermark_buffer_test.cc b/test/common/buffer/watermark_buffer_test.cc index 4e2367f51eca..57f09121c708 100644 --- a/test/common/buffer/watermark_buffer_test.cc +++ b/test/common/buffer/watermark_buffer_test.cc @@ -121,7 +121,7 @@ TEST_F(WatermarkBufferTest, PrependBuffer) { TEST_F(WatermarkBufferTest, Commit) { buffer_.add(TEN_BYTES, 10); EXPECT_EQ(0, times_high_watermark_called_); - Buffer::Reservation reservation = buffer_.reserve(10); + Buffer::Reservation reservation = buffer_.reserveApproximately(10); reservation.commit(10); EXPECT_EQ(1, times_high_watermark_called_); EXPECT_EQ(20, buffer_.length()); @@ -471,7 +471,7 @@ TEST_F(WatermarkBufferTest, OverflowWatermarkDisabledOnVeryHighValue) { const uint32_t segment_denominator = 128; const uint32_t big_segment_len = std::numeric_limits::max() / segment_denominator + 1; for (uint32_t i = 0; i < segment_denominator; ++i) { - Buffer::Reservation reservation = buffer1.reserveSingleSlice(big_segment_len); + auto reservation = buffer1.reserveSingleSlice(big_segment_len); reservation.commit(big_segment_len); } EXPECT_GT(buffer1.length(), std::numeric_limits::max()); @@ -482,7 +482,7 @@ TEST_F(WatermarkBufferTest, OverflowWatermarkDisabledOnVeryHighValue) { // Reserve and commit additional space on the buffer beyond the expected // high_watermark_threshold * overflow_multiplier threshold. const uint64_t size = high_watermark_threshold * overflow_multiplier - buffer1.length() + 1; - Buffer::Reservation reservation = buffer1.reserve(size); + auto reservation = buffer1.reserveApproximately(size); reservation.commit(size); EXPECT_EQ(buffer1.length(), high_watermark_threshold * overflow_multiplier + 1); EXPECT_EQ(1, high_watermark_buffer1); diff --git a/test/extensions/filters/network/postgres_proxy/postgres_decoder_test.cc b/test/extensions/filters/network/postgres_proxy/postgres_decoder_test.cc index 5211c64ad466..04d90d5416fa 100644 --- a/test/extensions/filters/network/postgres_proxy/postgres_decoder_test.cc +++ b/test/extensions/filters/network/postgres_proxy/postgres_decoder_test.cc @@ -528,9 +528,9 @@ class FakeBuffer : public Buffer::Instance { MOCK_METHOD(void*, linearize, (uint32_t), (override)); MOCK_METHOD(void, move, (Instance&), (override)); MOCK_METHOD(void, move, (Instance&, uint64_t), (override)); - MOCK_METHOD(Buffer::Reservation, reserve, (uint64_t), (override)); - MOCK_METHOD(Buffer::Reservation, reserveSingleSlice, (uint64_t, bool), (override)); - MOCK_METHOD(void, commit, (Buffer::Reservation & reservation, uint64_t length), (override)); + MOCK_METHOD(Buffer::Reservation, reserveApproximately, (uint64_t), (override)); + MOCK_METHOD(Buffer::ReservationSingleSlice, reserveSingleSlice, (uint64_t, bool), (override)); + MOCK_METHOD(void, commit, (uint64_t, absl::Span, absl::Span), (override)); MOCK_METHOD(ssize_t, search, (const void*, uint64_t, size_t, size_t), (const, override)); MOCK_METHOD(bool, startsWith, (absl::string_view), (const, override)); MOCK_METHOD(std::string, toString, (), (const, override)); diff --git a/test/extensions/transport_sockets/tls/tls_throughput_benchmark.cc b/test/extensions/transport_sockets/tls/tls_throughput_benchmark.cc index 348486c65e5b..eb9d990698b8 100644 --- a/test/extensions/transport_sockets/tls/tls_throughput_benchmark.cc +++ b/test/extensions/transport_sockets/tls/tls_throughput_benchmark.cc @@ -39,9 +39,9 @@ static void appendSlice(Buffer::Instance& buffer, uint32_t size) { // A 16kb request currently has inline metadata, which makes it 16384+8. This gets rounded up // to the next page size. Request enough that there is no extra space, to ensure that this results // in a new slice. - Buffer::Reservation reservation = buffer.reserveSingleSlice(16384); + auto reservation = buffer.reserveSingleSlice(16384); - memcpy(reservation.slices()[0].mem_, data.data(), data.size()); + memcpy(reservation.slice().mem_, data.data(), data.size()); reservation.commit(data.size()); } @@ -54,7 +54,7 @@ static void addFullSlices(Buffer::Instance& output_buffer, int num_slices, bool for (int i = 0; i < num_slices; i++) { auto start_size = buffer->length(); - Buffer::Reservation reservation = buffer->reserve(16384); + Buffer::Reservation reservation = buffer->reserveApproximately(16384); for (unsigned i = 0; i < reservation.numSlices(); i++) { memset(reservation.slices()[i].mem_, 'a', reservation.slices()[i].len_); } From abcbe9e085fab15bf2baec5e2601502a65165d7a Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Wed, 16 Dec 2020 14:43:45 -0800 Subject: [PATCH 11/46] build fixes Signed-off-by: Greg Greenway --- .../filters/network/postgres_proxy/postgres_decoder_test.cc | 4 +++- test/integration/socket_interface_integration_test.cc | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/test/extensions/filters/network/postgres_proxy/postgres_decoder_test.cc b/test/extensions/filters/network/postgres_proxy/postgres_decoder_test.cc index fee49227a7d1..b906ba66c846 100644 --- a/test/extensions/filters/network/postgres_proxy/postgres_decoder_test.cc +++ b/test/extensions/filters/network/postgres_proxy/postgres_decoder_test.cc @@ -530,7 +530,9 @@ class FakeBuffer : public Buffer::Instance { MOCK_METHOD(void, move, (Instance&, uint64_t), (override)); MOCK_METHOD(Buffer::Reservation, reserveApproximately, (uint64_t), (override)); MOCK_METHOD(Buffer::ReservationSingleSlice, reserveSingleSlice, (uint64_t, bool), (override)); - MOCK_METHOD(void, commit, (uint64_t, absl::Span, absl::Span), (override)); + MOCK_METHOD(void, commit, + (uint64_t, absl::Span, absl::Span), + (override)); MOCK_METHOD(ssize_t, search, (const void*, uint64_t, size_t, size_t), (const, override)); MOCK_METHOD(bool, startsWith, (absl::string_view), (const, override)); MOCK_METHOD(std::string, toString, (), (const, override)); diff --git a/test/integration/socket_interface_integration_test.cc b/test/integration/socket_interface_integration_test.cc index 7acef28630d8..69883091e6eb 100644 --- a/test/integration/socket_interface_integration_test.cc +++ b/test/integration/socket_interface_integration_test.cc @@ -135,7 +135,7 @@ TEST_P(SocketInterfaceIntegrationTest, UdpSendToInternalAddressWithSocketInterfa std::make_unique(Network::Socket::Type::Datagram, local_valid_address); Buffer::OwnedImpl buffer; - Buffer::Reservation reservation = buffer.reserve(100); + auto reservation = buffer.reserveApproximately(100); auto result = socket->ioHandle().sendmsg(reservation.slices(), reservation.numSlices(), 0, local_valid_address->ip(), *peer_internal_address); From be6a9a330b0fee9a2e9fca5ac7201d3f66b2e8ab Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Wed, 16 Dec 2020 14:43:58 -0800 Subject: [PATCH 12/46] fix watermark buffer reservation logic Signed-off-by: Greg Greenway --- source/common/buffer/watermark_buffer.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/common/buffer/watermark_buffer.cc b/source/common/buffer/watermark_buffer.cc index aed8ddc93063..524ccfb99be6 100644 --- a/source/common/buffer/watermark_buffer.cc +++ b/source/common/buffer/watermark_buffer.cc @@ -72,8 +72,9 @@ Reservation WatermarkBuffer::reserveApproximately(uint64_t preferred_length) { adjusted_length = Slice::default_slice_size_; } else { const uint64_t available_length = high_watermark_ - current_length; - adjusted_length = IntUtil::roundUpToMultiple(adjusted_length, Slice::default_slice_size_); adjusted_length = std::min(available_length, preferred_length); + adjusted_length = IntUtil::roundUpToMultiple(adjusted_length, Slice::default_slice_size_); + adjusted_length = std::min(adjusted_length, preferred_length); } } From 532edbb667af04d44cf61d5c51223a1ad89b5713 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Mon, 21 Dec 2020 12:34:28 -0800 Subject: [PATCH 13/46] remove accidental addition to setup_clang.sh Signed-off-by: Greg Greenway --- bazel/setup_clang.sh | 8 -------- 1 file changed, 8 deletions(-) diff --git a/bazel/setup_clang.sh b/bazel/setup_clang.sh index bf92da6f8ffc..d0e58478dc0a 100755 --- a/bazel/setup_clang.sh +++ b/bazel/setup_clang.sh @@ -23,14 +23,6 @@ build:clang --repo_env='LLVM_CONFIG=${LLVM_PREFIX}/bin/llvm-config' build:clang --linkopt='-L$(llvm-config --libdir)' build:clang --linkopt='-Wl,-rpath,$(llvm-config --libdir)' -build:clang-asan --action_env='PATH=${PATH}' -build:clang-asan --action_env=CC=clang -build:clang-asan --action_env=CXX=clang++ -build:clang-asan --action_env='LLVM_CONFIG=${LLVM_PREFIX}/bin/llvm-config' -build:clang-asan --repo_env='LLVM_CONFIG=${LLVM_PREFIX}/bin/llvm-config' -build:clang-asan --linkopt='-L$(llvm-config --libdir)' -build:clang-asan --linkopt='-Wl,-rpath,$(llvm-config --libdir)' - build:clang-asan --action_env=ENVOY_UBSAN_VPTR=1 build:clang-asan --copt=-fsanitize=vptr,function build:clang-asan --linkopt=-fsanitize=vptr,function From 53fbc3390b6a9efa2da499a8a532736b58017513 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Mon, 21 Dec 2020 15:30:26 -0800 Subject: [PATCH 14/46] fixes Signed-off-by: Greg Greenway --- include/envoy/buffer/BUILD | 1 + include/envoy/buffer/buffer.h | 56 +++++++++---------- source/common/buffer/buffer_impl.cc | 22 ++++++-- source/common/buffer/watermark_buffer.cc | 3 +- .../common/network/io_socket_handle_impl.cc | 4 +- source/common/network/raw_buffer_socket.cc | 4 +- test/common/buffer/owned_impl_test.cc | 4 +- 7 files changed, 50 insertions(+), 44 deletions(-) diff --git a/include/envoy/buffer/BUILD b/include/envoy/buffer/BUILD index 499ec8605a2e..9367bedb85ec 100644 --- a/include/envoy/buffer/BUILD +++ b/include/envoy/buffer/BUILD @@ -16,6 +16,7 @@ envoy_cc_library( ], deps = [ "//include/envoy/api:os_sys_calls_interface", + "//source/common/common:assert_lib", "//source/common/common:byte_order_lib", "//source/common/common:utility_lib", ], diff --git a/include/envoy/buffer/buffer.h b/include/envoy/buffer/buffer.h index b82a72e28202..9bc97666e774 100644 --- a/include/envoy/buffer/buffer.h +++ b/include/envoy/buffer/buffer.h @@ -10,6 +10,7 @@ #include "envoy/common/platform.h" #include "envoy/common/pure.h" +#include "common/common/assert.h" #include "common/common/byte_order.h" #include "common/common/utility.h" @@ -203,28 +204,22 @@ class Instance { */ virtual Reservation reserveApproximately(uint64_t preferred_length) PURE; + /** + * The maximum size that should be passed to `reserveApproximately` to result + * in maximum performance. + */ + static constexpr uint64_t MAX_RESERVATION_SIZE = 128 * 1024; + /** * Reserve space in the buffer in a single slice. * @param length the exact length of the reservation. * @param separate_slice specifies whether the reserved space must be in a separate slice * from any other data in this buffer. - * @return a `Reservation` which has exactly one slice in it. + * @return a `ReservationSingleSlice` which has exactly one slice in it. */ virtual ReservationSingleSlice reserveSingleSlice(uint64_t length, bool separate_slice = false) PURE; -private: - friend Reservation; - friend ReservationSingleSlice; - - /** - * Called by a `Reservation` to commit `length` bytes of the - * reservation. - */ - virtual void commit(uint64_t length, absl::Span slices, - absl::Span owned_slices) PURE; - -public: /** * Search for an occurrence of data within the buffer. * @param data supplies the data to search for. @@ -429,6 +424,17 @@ class Instance { * the low watermark. */ virtual bool highWatermarkTriggered() const PURE; + +private: + friend Reservation; + friend ReservationSingleSlice; + + /** + * Called by a `Reservation` to commit `length` bytes of the + * reservation. + */ + virtual void commit(uint64_t length, absl::Span slices, + absl::Span owned_slices) PURE; }; using InstancePtr = std::unique_ptr; @@ -464,12 +470,7 @@ using WatermarkFactoryPtr = std::unique_ptr; class Reservation final { public: Reservation(Reservation&&) = default; - - ~Reservation() { - if (!slices_.empty()) { - commit(0); - } - } + ~Reservation() = default; /** * @return an array of `RawSlice` of length `numSlices()`. @@ -495,14 +496,14 @@ class Reservation final { * @note No other methods should be called on the object after `commit()` is called. */ void commit(uint64_t length) { + ASSERT(length <= length_); buffer_.commit(length, absl::MakeSpan(slices_), absl::MakeSpan(owned_slices_)); slices_.clear(); owned_slices_.clear(); } - // Tuned to allow reads of 128k, using 16k slices, and including one partially-used - // initial slice from a previous allocation. - static constexpr uint32_t NUM_ELEMENTS_ = 9; + // Tuned to allow reads of 128k, using 16k slices. + static constexpr uint32_t MAX_SLICES_ = 8; private: Reservation(Instance& buffer) : buffer_(buffer) {} @@ -514,12 +515,12 @@ class Reservation final { uint64_t length_; // The RawSlices in the reservation, usable by operations such as `::readv()`. - absl::InlinedVector slices_; + absl::InlinedVector slices_; // An array of the same length as `slices_`, in which each element is either nullptr // if no operation needs to be performed to transfer ownership during commit/destructor, // or a pointer to the object that needs to be moved/deleted. - absl::InlinedVector owned_slices_; + absl::InlinedVector owned_slices_; public: // The following are for use only by implementations of Buffer. Because c++ @@ -540,12 +541,7 @@ class Reservation final { class ReservationSingleSlice final { public: ReservationSingleSlice(ReservationSingleSlice&&) = default; - - ~ReservationSingleSlice() { - if (slice_.mem_ != nullptr) { - commit(0); - } - } + ~ReservationSingleSlice() = default; /** * @return an array of `RawSlice` of length `numSlices()`. diff --git a/source/common/buffer/buffer_impl.cc b/source/common/buffer/buffer_impl.cc index f829dabb264b..1dd12b068946 100644 --- a/source/common/buffer/buffer_impl.cc +++ b/source/common/buffer/buffer_impl.cc @@ -311,6 +311,7 @@ Reservation OwnedImpl::reserveApproximately(uint64_t length) { } uint64_t bytes_remaining = length; + uint64_t reserved = 0; auto& reservation_slices = reservation.bufferImplUseOnlySlices(); auto& reservation_owned_slices = reservation.bufferImplUseOnlyOwnedSlices(); @@ -323,23 +324,32 @@ Reservation OwnedImpl::reserveApproximately(uint64_t length) { reservation_slices.push_back(slice); reservation_owned_slices.push_back(nullptr); bytes_remaining -= slice.len_; + reserved += slice.len_; } while (bytes_remaining != 0) { - uint64_t size = Slice::default_slice_size_; - if (bytes_remaining > size && (reservation_slices.size() + 1) == reservation.NUM_ELEMENTS_) { - size = bytes_remaining; + const uint64_t size = Slice::default_slice_size_; + + // If the next slice would go over the desired size, and the amount already reserved is already + // at least one full slice in size, stop allocating slices. This prevents returning a + // reservation larger than requested, which could go above the watermark limits for a watermark + // buffer, unless the size would be very small (less than 1 full slice). + if (size > bytes_remaining && reserved >= size) { + break; } + Slice slice(size); reservation_slices.push_back(slice.reserve(size)); reservation_owned_slices.emplace_back(std::make_unique(std::move(slice))); bytes_remaining -= std::min(reservation_slices.back().len_, bytes_remaining); + reserved += reservation_slices.back().len_; + + ASSERT(reservation_slices.size() <= reservation.MAX_SLICES_, + "The absl::InlineVector should be sized so that out-of-line storage isn't needed."); } - reservation.bufferImplUseOnlySetLength(length); + reservation.bufferImplUseOnlySetLength(reserved); - ASSERT(reservation_slices.size() <= reservation.NUM_ELEMENTS_, - "The absl::InlineVector should be sized so that out-of-line storage isn't needed."); ASSERT(reservation_slices.size() == reservation_owned_slices.size()); return reservation; } diff --git a/source/common/buffer/watermark_buffer.cc b/source/common/buffer/watermark_buffer.cc index 524ccfb99be6..effb2afbd988 100644 --- a/source/common/buffer/watermark_buffer.cc +++ b/source/common/buffer/watermark_buffer.cc @@ -72,8 +72,7 @@ Reservation WatermarkBuffer::reserveApproximately(uint64_t preferred_length) { adjusted_length = Slice::default_slice_size_; } else { const uint64_t available_length = high_watermark_ - current_length; - adjusted_length = std::min(available_length, preferred_length); - adjusted_length = IntUtil::roundUpToMultiple(adjusted_length, Slice::default_slice_size_); + adjusted_length = IntUtil::roundUpToMultiple(available_length, Slice::default_slice_size_); adjusted_length = std::min(adjusted_length, preferred_length); } } diff --git a/source/common/network/io_socket_handle_impl.cc b/source/common/network/io_socket_handle_impl.cc index ddbc5fd3515b..b291b09de079 100644 --- a/source/common/network/io_socket_handle_impl.cc +++ b/source/common/network/io_socket_handle_impl.cc @@ -113,8 +113,8 @@ Api::IoCallUint64Result IoSocketHandleImpl::read(Buffer::Instance& buffer, uint6 return Api::ioCallUint64ResultNoError(); } Buffer::Reservation reservation = buffer.reserveApproximately(max_length); - Api::IoCallUint64Result result = - readv(reservation.length(), reservation.slices(), reservation.numSlices()); + Api::IoCallUint64Result result = readv(std::min(reservation.length(), max_length), + reservation.slices(), reservation.numSlices()); uint64_t bytes_to_commit = result.ok() ? result.rc_ : 0; ASSERT(bytes_to_commit <= max_length); reservation.commit(bytes_to_commit); diff --git a/source/common/network/raw_buffer_socket.cc b/source/common/network/raw_buffer_socket.cc index a4ded3888b41..64e03163eef4 100644 --- a/source/common/network/raw_buffer_socket.cc +++ b/source/common/network/raw_buffer_socket.cc @@ -18,8 +18,8 @@ IoResult RawBufferSocket::doRead(Buffer::Instance& buffer) { uint64_t bytes_read = 0; bool end_stream = false; do { - // 16K read is arbitrary. TODO(mattklein123) PERF: Tune the read size. - Api::IoCallUint64Result result = callbacks_->ioHandle().read(buffer, 131072); + Api::IoCallUint64Result result = + callbacks_->ioHandle().read(buffer, Buffer::Instance::MAX_RESERVATION_SIZE); if (result.ok()) { ENVOY_CONN_LOG(trace, "read returns: {}", callbacks_->connection(), result.rc_); diff --git a/test/common/buffer/owned_impl_test.cc b/test/common/buffer/owned_impl_test.cc index 05835482636e..45c8655743e5 100644 --- a/test/common/buffer/owned_impl_test.cc +++ b/test/common/buffer/owned_impl_test.cc @@ -831,10 +831,9 @@ TEST_F(OwnedImplTest, ReserveCommit) { { expectSlices({{1, 16383, 16384}}, buffer); auto reservation = buffer.reserveApproximately(32768); - EXPECT_EQ(3, reservation.numSlices()); + EXPECT_EQ(2, reservation.numSlices()); EXPECT_EQ(16383, reservation.slices()[0].len_); EXPECT_EQ(16384, reservation.slices()[1].len_); - EXPECT_EQ(16384, reservation.slices()[2].len_); } // Append a fragment to the buffer, and then request a small reservation. The buffer @@ -845,6 +844,7 @@ TEST_F(OwnedImplTest, ReserveCommit) { buffer.addBufferFragment(fragment); EXPECT_EQ(13, buffer.length()); auto reservation = buffer.reserveApproximately(1); + EXPECT_EQ(1, reservation.numSlices()); EXPECT_NE(slice1, reservation.slices()[0].mem_); reservation.commit(1); expectSlices({{1, 16383, 16384}, {12, 0, 12}, {1, 16383, 16384}}, buffer); From efd8057346d1258b4538a56a1e9849102639a845 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Thu, 14 Jan 2021 09:43:20 -0800 Subject: [PATCH 15/46] release note Signed-off-by: Greg Greenway --- docs/root/version_history/current.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 2f07e2da2e44..c9fe92633d8b 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -9,6 +9,7 @@ Minor Behavior Changes ---------------------- *Changes that may cause incompatibilities for some users, but should not for most* +* perf: allow reading more bytes per operation from raw sockets to improve performance. * upstream: host weight changes now cause a full load balancer rebuild as opposed to happening atomically inline. This change has been made to support load balancer pre-computation of data structures based on host weight, but may have performance implications if host weight changes From 75278b62d922f98aeca9cfe2aff207751b211cab Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Thu, 14 Jan 2021 10:35:23 -0800 Subject: [PATCH 16/46] fix merge conflict Signed-off-by: Greg Greenway --- test/common/http/http2/hpack_fuzz_test.cc | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/test/common/http/http2/hpack_fuzz_test.cc b/test/common/http/http2/hpack_fuzz_test.cc index 49fa83db3dbc..cd090faaaabe 100644 --- a/test/common/http/http2/hpack_fuzz_test.cc +++ b/test/common/http/http2/hpack_fuzz_test.cc @@ -41,20 +41,17 @@ Buffer::OwnedImpl encodeHeaders(nghttp2_hd_deflater* deflater, // Estimate the upper bound const size_t buflen = nghttp2_hd_deflate_bound(deflater, input_nv.data(), input_nv.size()); - Buffer::RawSlice iovec; Buffer::OwnedImpl payload; - payload.reserve(buflen, &iovec, 1); - ASSERT(iovec.len_ >= buflen); + auto reservation = payload.reserveSingleSlice(buflen); // Encode using nghttp2 - uint8_t* buf = reinterpret_cast(iovec.mem_); + uint8_t* buf = reinterpret_cast(reservation.slice().mem_); ASSERT(input_nv.data() != nullptr); const ssize_t result = nghttp2_hd_deflate_hd(deflater, buf, buflen, input_nv.data(), input_nv.size()); ASSERT(result >= 0, absl::StrCat("Failed to decode with result ", result)); - iovec.len_ = result; - payload.commit(&iovec, 1); + reservation.commit(result); return payload; } From e56585cd389b1046a30e9a3c793a64cca9a146dc Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Thu, 14 Jan 2021 15:21:54 -0800 Subject: [PATCH 17/46] clang_tidy Signed-off-by: Greg Greenway --- test/common/buffer/buffer_speed_test.cc | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/test/common/buffer/buffer_speed_test.cc b/test/common/buffer/buffer_speed_test.cc index ae8537d9443c..78d6199ebb16 100644 --- a/test/common/buffer/buffer_speed_test.cc +++ b/test/common/buffer/buffer_speed_test.cc @@ -16,6 +16,7 @@ void deleteFragment(const void*, size_t, const Buffer::BufferFragmentImpl* self) static void bufferCreateEmpty(benchmark::State& state) { uint64_t length = 0; for (auto _ : state) { + UNREFERENCED_PARAMETER(_); Buffer::OwnedImpl buffer; length += buffer.length(); } @@ -29,6 +30,7 @@ static void bufferCreate(benchmark::State& state) { const absl::string_view input(data); uint64_t length = 0; for (auto _ : state) { + UNREFERENCED_PARAMETER(_); Buffer::OwnedImpl buffer(input); length += buffer.length(); } @@ -42,6 +44,7 @@ static void bufferAddSmallIncrement(benchmark::State& state) { const absl::string_view input(data); Buffer::OwnedImpl buffer; for (auto _ : state) { + UNREFERENCED_PARAMETER(_); buffer.add(input); if (buffer.length() >= MaxBufferLength) { // Keep the test's memory usage from growing too large. @@ -62,6 +65,7 @@ static void bufferAddString(benchmark::State& state) { const absl::string_view input(data); Buffer::OwnedImpl buffer(input); for (auto _ : state) { + UNREFERENCED_PARAMETER(_); buffer.add(data); if (buffer.length() >= MaxBufferLength) { buffer.drain(buffer.length()); @@ -79,6 +83,7 @@ static void bufferAddBuffer(benchmark::State& state) { const Buffer::OwnedImpl to_add(data); Buffer::OwnedImpl buffer(input); for (auto _ : state) { + UNREFERENCED_PARAMETER(_); buffer.add(to_add); if (buffer.length() >= MaxBufferLength) { buffer.drain(buffer.length()); @@ -94,6 +99,7 @@ static void bufferPrependString(benchmark::State& state) { const absl::string_view input(data); Buffer::OwnedImpl buffer(input); for (auto _ : state) { + UNREFERENCED_PARAMETER(_); buffer.prepend(data); if (buffer.length() >= MaxBufferLength) { buffer.drain(buffer.length()); @@ -109,6 +115,7 @@ static void bufferPrependBuffer(benchmark::State& state) { const absl::string_view input(data); Buffer::OwnedImpl buffer(input); for (auto _ : state) { + UNREFERENCED_PARAMETER(_); // The prepend method removes the content from its source buffer. To populate a new source // buffer every time without the overhead of a copy, we use an BufferFragment that references // (and never deletes) an external string. @@ -144,6 +151,7 @@ static void bufferDrain(benchmark::State& state) { size_t drain_cycle = 0; for (auto _ : state) { + UNREFERENCED_PARAMETER(_); buffer.add(to_add); buffer.drain(drain_size[drain_cycle]); drain_cycle++; @@ -159,6 +167,7 @@ static void bufferDrainSmallIncrement(benchmark::State& state) { const absl::string_view input(data); Buffer::OwnedImpl buffer(input); for (auto _ : state) { + UNREFERENCED_PARAMETER(_); buffer.drain(state.range(0)); if (buffer.length() == 0) { buffer.add(input); @@ -175,6 +184,7 @@ static void bufferMove(benchmark::State& state) { Buffer::OwnedImpl buffer1(input); Buffer::OwnedImpl buffer2(input); for (auto _ : state) { + UNREFERENCED_PARAMETER(_); buffer1.move(buffer2); // now buffer1 has 2 copies of the input, and buffer2 is empty. buffer2.move(buffer1, input.size()); // now buffer1 and buffer2 are the same size. } @@ -192,6 +202,7 @@ static void bufferMovePartial(benchmark::State& state) { Buffer::OwnedImpl buffer1(input); Buffer::OwnedImpl buffer2(input); for (auto _ : state) { + UNREFERENCED_PARAMETER(_); while (buffer2.length() != 0) { buffer1.move(buffer2, 1); } @@ -207,6 +218,7 @@ BENCHMARK(bufferMovePartial)->Arg(1)->Arg(4096)->Arg(16384)->Arg(65536); static void bufferReserveCommit(benchmark::State& state) { Buffer::OwnedImpl buffer; for (auto _ : state) { + UNREFERENCED_PARAMETER(_); auto size = state.range(0); Buffer::Reservation reservation = buffer.reserveApproximately(size); reservation.commit(size); @@ -223,6 +235,7 @@ BENCHMARK(bufferReserveCommit)->Arg(1)->Arg(4096)->Arg(16384)->Arg(65536); static void bufferReserveCommitPartial(benchmark::State& state) { Buffer::OwnedImpl buffer; for (auto _ : state) { + UNREFERENCED_PARAMETER(_); auto size = state.range(0); Buffer::Reservation reservation = buffer.reserveApproximately(size); // Commit one byte from the first slice and nothing from any subsequent slice. @@ -241,6 +254,7 @@ static void bufferLinearizeSimple(benchmark::State& state) { const absl::string_view input(data); Buffer::OwnedImpl buffer; for (auto _ : state) { + UNREFERENCED_PARAMETER(_); buffer.drain(buffer.length()); auto fragment = std::make_unique(input.data(), input.size(), deleteFragment); @@ -258,6 +272,7 @@ static void bufferLinearizeGeneral(benchmark::State& state) { const absl::string_view input(data); Buffer::OwnedImpl buffer; for (auto _ : state) { + UNREFERENCED_PARAMETER(_); buffer.drain(buffer.length()); do { auto fragment = @@ -282,6 +297,7 @@ static void bufferSearch(benchmark::State& state) { Buffer::OwnedImpl buffer(input); ssize_t result = 0; for (auto _ : state) { + UNREFERENCED_PARAMETER(_); result += buffer.search(Pattern.c_str(), Pattern.length(), 0, 0); } benchmark::DoNotOptimize(result); @@ -305,6 +321,7 @@ static void bufferSearchPartialMatch(benchmark::State& state) { Buffer::OwnedImpl buffer(input); ssize_t result = 0; for (auto _ : state) { + UNREFERENCED_PARAMETER(_); result += buffer.search(Pattern.c_str(), Pattern.length(), 0, 0); } benchmark::DoNotOptimize(result); @@ -324,6 +341,7 @@ static void bufferStartsWith(benchmark::State& state) { Buffer::OwnedImpl buffer(input); ssize_t result = 0; for (auto _ : state) { + UNREFERENCED_PARAMETER(_); if (!buffer.startsWith({Pattern.c_str(), Pattern.length()})) { result++; } @@ -347,6 +365,7 @@ static void bufferStartsWithMatch(benchmark::State& state) { Buffer::OwnedImpl buffer(input); ssize_t result = 0; for (auto _ : state) { + UNREFERENCED_PARAMETER(_); if (buffer.startsWith({Prefix.c_str(), Prefix.length()})) { result++; } From fc7b5920206f835f5c6f9b610787e523f3d2ff4e Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Thu, 14 Jan 2021 15:25:42 -0800 Subject: [PATCH 18/46] fix mac build Signed-off-by: Greg Greenway --- source/common/buffer/buffer_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/buffer/buffer_impl.cc b/source/common/buffer/buffer_impl.cc index 645b63e83766..c6d7052d05a7 100644 --- a/source/common/buffer/buffer_impl.cc +++ b/source/common/buffer/buffer_impl.cc @@ -342,7 +342,7 @@ Reservation OwnedImpl::reserveApproximately(uint64_t length) { Slice slice(size); reservation_slices.push_back(slice.reserve(size)); reservation_owned_slices.emplace_back(std::make_unique(std::move(slice))); - bytes_remaining -= std::min(reservation_slices.back().len_, bytes_remaining); + bytes_remaining -= std::min(reservation_slices.back().len_, bytes_remaining); reserved += reservation_slices.back().len_; ASSERT(reservation_slices.size() <= reservation.MAX_SLICES_, From 5885132a87f52e142e6c590b7c99f73d334f7767 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Tue, 19 Jan 2021 10:27:17 -0800 Subject: [PATCH 19/46] one more ASSERT Signed-off-by: Greg Greenway --- include/envoy/buffer/buffer.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/envoy/buffer/buffer.h b/include/envoy/buffer/buffer.h index 61d7d5b55345..fe8c417c8852 100644 --- a/include/envoy/buffer/buffer.h +++ b/include/envoy/buffer/buffer.h @@ -557,6 +557,7 @@ class ReservationSingleSlice final { * @note No other methods should be called on the object after `commit()` is called. */ void commit(uint64_t length) { + ASSERT(length <= slice_.len_); buffer_.commit(length, absl::MakeSpan(&slice_, 1), absl::MakeSpan(&owned_slice_, 1)); slice_ = {nullptr, 0}; owned_slice_.reset(); From a1e6c3e605e22e5b6515082cbdb1b10631c3cdf5 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Wed, 20 Jan 2021 16:25:06 -0800 Subject: [PATCH 20/46] remove cast to fix compilation; the types matched exactly without the cast Signed-off-by: Greg Greenway --- source/common/network/utility.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index 07d71c2b9fb9..14de3cdc92be 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -106,7 +106,7 @@ Api::IoCallUint64Result receiveMessage(uint64_t max_packet_size, Buffer::Instanc Api::IoCallUint64Result result = handle.recvmsg(&slice, 1, local_address.ip()->port(), output); if (result.ok()) { - reservation.commit(std::min(max_packet_size, static_cast(result.rc_))); + reservation.commit(std::min(max_packet_size, result.rc_)); } return result; @@ -654,7 +654,7 @@ Api::IoCallUint64Result Utility::readFromSocket(IoHandle& handle, ENVOY_LOG_MISC(debug, "Receive a packet with {} bytes from {}", msg_len, output.msg_[i].peer_address_->asString()); - buffers[i].reservation_.commit(std::min(max_packet_size, static_cast(msg_len))); + buffers[i].reservation_.commit(std::min(max_packet_size, msg_len)); passPayloadToProcessor(msg_len, std::move(buffers[i].buffer_), output.msg_[i].peer_address_, output.msg_[i].local_address_, udp_packet_processor, receive_time); From 0e80aca61ffd89d3ba99259258ad558e09e9a4c0 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Wed, 20 Jan 2021 16:31:00 -0800 Subject: [PATCH 21/46] fix tls throughput benchmark correctness Signed-off-by: Greg Greenway --- .../transport_sockets/tls/tls_throughput_benchmark.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/extensions/transport_sockets/tls/tls_throughput_benchmark.cc b/test/extensions/transport_sockets/tls/tls_throughput_benchmark.cc index eb9d990698b8..75a7f46455cd 100644 --- a/test/extensions/transport_sockets/tls/tls_throughput_benchmark.cc +++ b/test/extensions/transport_sockets/tls/tls_throughput_benchmark.cc @@ -58,7 +58,7 @@ static void addFullSlices(Buffer::Instance& output_buffer, int num_slices, bool for (unsigned i = 0; i < reservation.numSlices(); i++) { memset(reservation.slices()[i].mem_, 'a', reservation.slices()[i].len_); } - reservation.commit(16384); + reservation.commit(reservation.length()); RELEASE_ASSERT(buffer->length() - start_size == 16384, "correct reserve/commit"); } From a5deffa6d6edaf911e595d001c783f58414a3c2c Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Wed, 20 Jan 2021 16:37:08 -0800 Subject: [PATCH 22/46] undo change to connection impl test buffer size Signed-off-by: Greg Greenway --- test/common/network/connection_impl_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index 8cb27208002a..263119fdd8cc 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -2886,7 +2886,7 @@ TEST_P(ReadBufferLimitTest, SomeLimit) { // Envoy has soft limits, so as long as the first read is <= read_buffer_limit - 1 it will do a // second read. The effective chunk size is then read_buffer_limit - 1 + MaxReadSize, // which is currently 16384. - readBufferLimitTest(read_buffer_limit, read_buffer_limit - 1 + (8 * 16384)); + readBufferLimitTest(read_buffer_limit, read_buffer_limit - 1 + 16384); } class TcpClientConnectionImplTest : public testing::TestWithParam { From dc4d6188f7c22547fe497af9423f1f71686cb5f0 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Wed, 20 Jan 2021 16:41:35 -0800 Subject: [PATCH 23/46] ensure reservation is the desired size Signed-off-by: Greg Greenway --- test/common/buffer/watermark_buffer_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/buffer/watermark_buffer_test.cc b/test/common/buffer/watermark_buffer_test.cc index 57f09121c708..d339e9fef5f5 100644 --- a/test/common/buffer/watermark_buffer_test.cc +++ b/test/common/buffer/watermark_buffer_test.cc @@ -482,7 +482,7 @@ TEST_F(WatermarkBufferTest, OverflowWatermarkDisabledOnVeryHighValue) { // Reserve and commit additional space on the buffer beyond the expected // high_watermark_threshold * overflow_multiplier threshold. const uint64_t size = high_watermark_threshold * overflow_multiplier - buffer1.length() + 1; - auto reservation = buffer1.reserveApproximately(size); + auto reservation = buffer1.reserveSingleSlice(size); reservation.commit(size); EXPECT_EQ(buffer1.length(), high_watermark_threshold * overflow_multiplier + 1); EXPECT_EQ(1, high_watermark_buffer1); From 7bd544abae9f27649d4035ac14a2fcef417dd60c Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Wed, 20 Jan 2021 16:46:53 -0800 Subject: [PATCH 24/46] comment in test regarding when slices are added, and additional expectation to make it clearer Signed-off-by: Greg Greenway --- test/common/buffer/owned_impl_test.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/common/buffer/owned_impl_test.cc b/test/common/buffer/owned_impl_test.cc index 45c8655743e5..e68c003a4dfc 100644 --- a/test/common/buffer/owned_impl_test.cc +++ b/test/common/buffer/owned_impl_test.cc @@ -872,7 +872,11 @@ TEST_F(OwnedImplTest, ReserveCommitReuse) { // Commit part of the first slice and none of the second slice. const void* first_slice; { + expectSlices({{8000, 4288, 12288}}, buffer); auto reservation = buffer.reserveApproximately(16384); + + // No additional slices are added to the buffer until `commit()` is called + // on the reservation. expectSlices({{8000, 4288, 12288}}, buffer); first_slice = reservation.slices()[0].mem_; From 4e0331db544415b6dcafddb85fcf3f4afcf9b3d3 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Wed, 20 Jan 2021 16:49:08 -0800 Subject: [PATCH 25/46] add expectation on reservation length Signed-off-by: Greg Greenway --- test/common/buffer/owned_impl_test.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/test/common/buffer/owned_impl_test.cc b/test/common/buffer/owned_impl_test.cc index e68c003a4dfc..948e2b89e798 100644 --- a/test/common/buffer/owned_impl_test.cc +++ b/test/common/buffer/owned_impl_test.cc @@ -832,6 +832,7 @@ TEST_F(OwnedImplTest, ReserveCommit) { expectSlices({{1, 16383, 16384}}, buffer); auto reservation = buffer.reserveApproximately(32768); EXPECT_EQ(2, reservation.numSlices()); + EXPECT_EQ(32767, reservation.length()); EXPECT_EQ(16383, reservation.slices()[0].len_); EXPECT_EQ(16384, reservation.slices()[1].len_); } From b4c8e4623035fb9954bdf976edf47e9c7422f210 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Thu, 21 Jan 2021 12:10:00 -0800 Subject: [PATCH 26/46] rework API to not take a param for reservation size for reads Signed-off-by: Greg Greenway --- include/envoy/buffer/buffer.h | 24 +++++----- source/common/buffer/buffer_impl.cc | 6 ++- source/common/buffer/buffer_impl.h | 21 ++++++-- source/common/buffer/watermark_buffer.cc | 5 +- source/common/buffer/watermark_buffer.h | 7 +-- .../common/network/io_socket_handle_impl.cc | 2 +- source/common/network/raw_buffer_socket.cc | 6 ++- .../transport_sockets/tls/ssl_socket.cc | 2 +- test/common/buffer/buffer_fuzz.cc | 26 ++++++---- test/common/buffer/buffer_speed_test.cc | 4 +- test/common/buffer/owned_impl_test.cc | 48 ++++++++----------- test/common/buffer/watermark_buffer_test.cc | 14 +++++- .../postgres_proxy/postgres_decoder_test.cc | 2 +- .../tls/tls_throughput_benchmark.cc | 15 +++--- .../socket_interface_integration_test.cc | 7 +-- 15 files changed, 107 insertions(+), 82 deletions(-) diff --git a/include/envoy/buffer/buffer.h b/include/envoy/buffer/buffer.h index fe8c417c8852..c1aabc16a89c 100644 --- a/include/envoy/buffer/buffer.h +++ b/include/envoy/buffer/buffer.h @@ -196,19 +196,12 @@ class Instance { virtual void move(Instance& rhs, uint64_t length) PURE; /** - * Reserve space in the buffer. - * @param preferred_length the suggested length to reserve. The actual - * reserved size may be differ based on heuristics to maximize performance. + * Reserve space in the buffer for reading into. The amount of space reserved is determined + * based on buffer settings and performance considerations. * @return a `Reservation`, on which `commit()` can be called, or which can * be destructed to discard any resources in the `Reservation`. */ - virtual Reservation reserveApproximately(uint64_t preferred_length) PURE; - - /** - * The maximum size that should be passed to `reserveApproximately` to result - * in maximum performance. - */ - static constexpr uint64_t MAX_RESERVATION_SIZE = 128 * 1024; + virtual Reservation reserveForRead() PURE; /** * Reserve space in the buffer in a single slice. @@ -497,7 +490,7 @@ class Reservation final { * @note No other methods should be called on the object after `commit()` is called. */ void commit(uint64_t length) { - ASSERT(length <= length_); + ENVOY_BUG(length <= length_, "commit() length must be <= size of the Reservation"); buffer_.commit(length, absl::MakeSpan(slices_), absl::MakeSpan(owned_slices_)); slices_.clear(); owned_slices_.clear(); @@ -545,10 +538,15 @@ class ReservationSingleSlice final { ~ReservationSingleSlice() = default; /** - * @return an array of `RawSlice` of length `numSlices()`. + * @return the slice in the Reservation. */ RawSlice slice() const { return slice_; } + /** + * @return the total length of the Reservation. + */ + uint64_t length() const { return slice_.len_; } + /** * Commits some or all of the data in the reservation. * @param length supplies the number of bytes to commit. This must be @@ -557,7 +555,7 @@ class ReservationSingleSlice final { * @note No other methods should be called on the object after `commit()` is called. */ void commit(uint64_t length) { - ASSERT(length <= slice_.len_); + ENVOY_BUG(length <= slice_.len_, "commit() length must be <= size of the Reservation"); buffer_.commit(length, absl::MakeSpan(&slice_, 1), absl::MakeSpan(&owned_slice_, 1)); slice_ = {nullptr, 0}; owned_slice_.reset(); diff --git a/source/common/buffer/buffer_impl.cc b/source/common/buffer/buffer_impl.cc index c6d7052d05a7..43c12256498d 100644 --- a/source/common/buffer/buffer_impl.cc +++ b/source/common/buffer/buffer_impl.cc @@ -300,7 +300,11 @@ void OwnedImpl::move(Instance& rhs, uint64_t length) { other.postProcess(); } -Reservation OwnedImpl::reserveApproximately(uint64_t length) { +Reservation OwnedImpl::reserveForRead() { + return reserveWithLength(default_read_reservation_size_); +} + +Reservation OwnedImpl::reserveWithLength(uint64_t length) { Reservation reservation = Reservation::bufferImplUseOnlyConstruct(*this); if (length == 0) { return reservation; diff --git a/source/common/buffer/buffer_impl.h b/source/common/buffer/buffer_impl.h index 1f77fa9c9e4a..df8ccef9f241 100644 --- a/source/common/buffer/buffer_impl.h +++ b/source/common/buffer/buffer_impl.h @@ -622,10 +622,8 @@ class OwnedImpl : public LibEventInstance { void* linearize(uint32_t size) override; void move(Instance& rhs) override; void move(Instance& rhs, uint64_t length) override; - Reservation reserveApproximately(uint64_t preferred_length) override; + Reservation reserveForRead() override; ReservationSingleSlice reserveSingleSlice(uint64_t length, bool separate_slice = false) override; - void commit(uint64_t length, absl::Span slices, - absl::Span owned_slices) override; ssize_t search(const void* data, uint64_t size, size_t start, size_t length) const override; bool startsWith(absl::string_view data) const override; std::string toString() const override; @@ -660,6 +658,23 @@ class OwnedImpl : public LibEventInstance { */ std::vector describeSlicesForTest() const; + /** + * Create a reservation for reading with a non-default length. Used in benchmark tests. + */ + Reservation reserveForReadWithLengthForTest(uint64_t length) { return reserveWithLength(length); } + +protected: + static constexpr uint64_t default_read_reservation_size_ = + Reservation::MAX_SLICES_ * Slice::default_slice_size_; + + /** + * Create a reservation with a specific length. + */ + Reservation reserveWithLength(uint64_t length); + + void commit(uint64_t length, absl::Span slices, + absl::Span owned_slices) override; + private: /** * @param rhs another buffer diff --git a/source/common/buffer/watermark_buffer.cc b/source/common/buffer/watermark_buffer.cc index effb2afbd988..dd68966eff4d 100644 --- a/source/common/buffer/watermark_buffer.cc +++ b/source/common/buffer/watermark_buffer.cc @@ -61,7 +61,8 @@ SliceDataPtr WatermarkBuffer::extractMutableFrontSlice() { // Adjust the reservation size based on space available before hitting // the high watermark to avoid overshooting by a lot and thus violating the limits // the watermark is imposing. -Reservation WatermarkBuffer::reserveApproximately(uint64_t preferred_length) { +Reservation WatermarkBuffer::reserveForRead() { + constexpr auto preferred_length = default_read_reservation_size_; uint64_t adjusted_length = preferred_length; if (high_watermark_ > 0 && preferred_length > 0) { @@ -77,7 +78,7 @@ Reservation WatermarkBuffer::reserveApproximately(uint64_t preferred_length) { } } - return OwnedImpl::reserveApproximately(adjusted_length); + return OwnedImpl::reserveWithLength(adjusted_length); } void WatermarkBuffer::appendSliceForTest(const void* data, uint64_t size) { diff --git a/source/common/buffer/watermark_buffer.h b/source/common/buffer/watermark_buffer.h index a9560f925024..b6feda4dc615 100644 --- a/source/common/buffer/watermark_buffer.h +++ b/source/common/buffer/watermark_buffer.h @@ -34,9 +34,7 @@ class WatermarkBuffer : public OwnedImpl { void move(Instance& rhs) override; void move(Instance& rhs, uint64_t length) override; SliceDataPtr extractMutableFrontSlice() override; - Reservation reserveApproximately(uint64_t preferred_length) override; - void commit(uint64_t length, absl::Span slices, - absl::Span owned_slices) override; + Reservation reserveForRead() override; void postProcess() override { checkLowWatermark(); } void appendSliceForTest(const void* data, uint64_t size) override; void appendSliceForTest(absl::string_view data) override; @@ -53,6 +51,9 @@ class WatermarkBuffer : public OwnedImpl { void checkLowWatermark(); private: + void commit(uint64_t length, absl::Span slices, + absl::Span owned_slices) override; + std::function below_low_watermark_; std::function above_high_watermark_; std::function above_overflow_watermark_; diff --git a/source/common/network/io_socket_handle_impl.cc b/source/common/network/io_socket_handle_impl.cc index b291b09de079..e6d0451a8595 100644 --- a/source/common/network/io_socket_handle_impl.cc +++ b/source/common/network/io_socket_handle_impl.cc @@ -112,7 +112,7 @@ Api::IoCallUint64Result IoSocketHandleImpl::read(Buffer::Instance& buffer, uint6 if (max_length == 0) { return Api::ioCallUint64ResultNoError(); } - Buffer::Reservation reservation = buffer.reserveApproximately(max_length); + Buffer::Reservation reservation = buffer.reserveForRead(); Api::IoCallUint64Result result = readv(std::min(reservation.length(), max_length), reservation.slices(), reservation.numSlices()); uint64_t bytes_to_commit = result.ok() ? result.rc_ : 0; diff --git a/source/common/network/raw_buffer_socket.cc b/source/common/network/raw_buffer_socket.cc index 64e03163eef4..99e37c44bcdd 100644 --- a/source/common/network/raw_buffer_socket.cc +++ b/source/common/network/raw_buffer_socket.cc @@ -18,8 +18,10 @@ IoResult RawBufferSocket::doRead(Buffer::Instance& buffer) { uint64_t bytes_read = 0; bool end_stream = false; do { - Api::IoCallUint64Result result = - callbacks_->ioHandle().read(buffer, Buffer::Instance::MAX_RESERVATION_SIZE); + // The maximum read size will be limited by lower layers, based on configured buffer sizes. + // Specifying UINT64_MAX indicates that this layer is not imposing any limit beyond what + // the lower layers impose. + Api::IoCallUint64Result result = callbacks_->ioHandle().read(buffer, UINT64_MAX); if (result.ok()) { ENVOY_CONN_LOG(trace, "read returns: {}", callbacks_->connection(), result.rc_); diff --git a/source/extensions/transport_sockets/tls/ssl_socket.cc b/source/extensions/transport_sockets/tls/ssl_socket.cc index c45b74a47426..dbcb87543c7c 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.cc +++ b/source/extensions/transport_sockets/tls/ssl_socket.cc @@ -121,7 +121,7 @@ Network::IoResult SslSocket::doRead(Buffer::Instance& read_buffer) { uint64_t bytes_read = 0; while (keep_reading) { uint64_t bytes_read_this_iteration = 0; - Buffer::Reservation reservation = read_buffer.reserveApproximately(16384); + Buffer::Reservation reservation = read_buffer.reserveForRead(); for (uint64_t i = 0; i < reservation.numSlices(); i++) { auto result = sslReadIntoSlice(reservation.slices()[i]); bytes_read_this_iteration += result.bytes_read_; diff --git a/test/common/buffer/buffer_fuzz.cc b/test/common/buffer/buffer_fuzz.cc index 3c2594f1964a..7835fc9d496c 100644 --- a/test/common/buffer/buffer_fuzz.cc +++ b/test/common/buffer/buffer_fuzz.cc @@ -141,13 +141,11 @@ class StringBuffer : public Buffer::Instance { src.size_ -= length; } - Buffer::Reservation reserveApproximately(uint64_t length) override { - FUZZ_ASSERT(start_ + size_ + length <= data_.size()); - + Buffer::Reservation reserveForRead() override { auto reservation = Buffer::Reservation::bufferImplUseOnlyConstruct(*this); Buffer::RawSlice slice; slice.mem_ = mutableEnd(); - slice.len_ = length; + slice.len_ = data_.size() - (start_ + size_); reservation.bufferImplUseOnlySlices().push_back(slice); reservation.bufferImplUseOnlyOwnedSlices().push_back(nullptr); @@ -276,13 +274,21 @@ uint32_t bufferAction(Context& ctxt, char insert_value, uint32_t max_alloc, Buff if (reserve_length == 0) { break; } - Buffer::Reservation reservation = target_buffer.reserveApproximately(reserve_length); - for (uint32_t i = 0; i < reservation.numSlices(); ++i) { - ::memset(reservation.slices()[i].mem_, insert_value, reservation.slices()[i].len_); + if (reserve_length < 16384) { + auto reservation = target_buffer.reserveSingleSlice(reserve_length); + ::memset(reservation.slice().mem_, insert_value, reservation.slice().len_); + RELEASE_ASSERT(action.reserve_commit().commit_length() <= reserve_length, ""); + reservation.commit( + std::min(action.reserve_commit().commit_length(), reservation.length())); + } else { + Buffer::Reservation reservation = target_buffer.reserveForRead(); + for (uint32_t i = 0; i < reservation.numSlices(); ++i) { + ::memset(reservation.slices()[i].mem_, insert_value, reservation.slices()[i].len_); + } + const uint32_t target_length = + std::min(reservation.length(), action.reserve_commit().commit_length()); + reservation.commit(target_length); } - const uint32_t target_length = - std::min(reservation.length(), action.reserve_commit().commit_length()); - reservation.commit(target_length); break; } case test::common::buffer::Action::kCopyOut: { diff --git a/test/common/buffer/buffer_speed_test.cc b/test/common/buffer/buffer_speed_test.cc index 78d6199ebb16..b2a00e3418f8 100644 --- a/test/common/buffer/buffer_speed_test.cc +++ b/test/common/buffer/buffer_speed_test.cc @@ -220,7 +220,7 @@ static void bufferReserveCommit(benchmark::State& state) { for (auto _ : state) { UNREFERENCED_PARAMETER(_); auto size = state.range(0); - Buffer::Reservation reservation = buffer.reserveApproximately(size); + Buffer::Reservation reservation = buffer.reserveForReadWithLengthForTest(size); reservation.commit(size); if (buffer.length() >= MaxBufferLength) { buffer.drain(buffer.length()); @@ -237,7 +237,7 @@ static void bufferReserveCommitPartial(benchmark::State& state) { for (auto _ : state) { UNREFERENCED_PARAMETER(_); auto size = state.range(0); - Buffer::Reservation reservation = buffer.reserveApproximately(size); + Buffer::Reservation reservation = buffer.reserveForReadWithLengthForTest(size); // Commit one byte from the first slice and nothing from any subsequent slice. reservation.commit(1); if (buffer.length() >= MaxBufferLength) { diff --git a/test/common/buffer/owned_impl_test.cc b/test/common/buffer/owned_impl_test.cc index 948e2b89e798..fc83e04c7939 100644 --- a/test/common/buffer/owned_impl_test.cc +++ b/test/common/buffer/owned_impl_test.cc @@ -785,16 +785,15 @@ TEST_F(OwnedImplTest, ReserveCommit) { Buffer::OwnedImpl buffer; // A zero-byte reservation should return an empty reservation. { - auto reservation = buffer.reserveApproximately(0); - EXPECT_EQ(0, reservation.numSlices()); + auto reservation = buffer.reserveSingleSlice(0); + EXPECT_EQ(0, reservation.slice().len_); + EXPECT_EQ(0, reservation.length()); } EXPECT_EQ(0, buffer.length()); // Test and commit a small reservation. This should succeed. { - auto reservation = buffer.reserveApproximately(1); - EXPECT_EQ(1, reservation.numSlices()); - EXPECT_EQ(16384, reservation.slices()[0].len_); + auto reservation = buffer.reserveForRead(); reservation.commit(1); } EXPECT_EQ(1, buffer.length()); @@ -802,10 +801,9 @@ TEST_F(OwnedImplTest, ReserveCommit) { // Request a reservation that fits in the remaining space at the end of the last slice. const void* slice1; { - auto reservation = buffer.reserveApproximately(1); - EXPECT_EQ(1, reservation.numSlices()); - EXPECT_EQ(1, reservation.slices()[0].len_); - slice1 = reservation.slices()[0].mem_; + auto reservation = buffer.reserveSingleSlice(1); + EXPECT_EQ(1, reservation.slice().len_); + slice1 = reservation.slice().mem_; } // Request a reservation that is too large to fit in the remaining space at the end of @@ -820,19 +818,12 @@ TEST_F(OwnedImplTest, ReserveCommit) { // Request the same size reservation, but allow the buffer to use multiple slices. This // should result in the buffer creating a second slice and splitting the reservation between the // last two slices. - { - auto reservation = buffer.reserveApproximately(16384); - EXPECT_EQ(2, reservation.numSlices()); - EXPECT_EQ(slice1, reservation.slices()[0].mem_); - } - - // Request a reservation that too big to fit in the existing slices. This should result - // in the creation of a third slice. { expectSlices({{1, 16383, 16384}}, buffer); - auto reservation = buffer.reserveApproximately(32768); - EXPECT_EQ(2, reservation.numSlices()); - EXPECT_EQ(32767, reservation.length()); + auto reservation = buffer.reserveForRead(); + EXPECT_GE(reservation.numSlices(), 2); + EXPECT_GE(reservation.length(), 32767); + EXPECT_EQ(slice1, reservation.slices()[0].mem_); EXPECT_EQ(16383, reservation.slices()[0].len_); EXPECT_EQ(16384, reservation.slices()[1].len_); } @@ -844,8 +835,7 @@ TEST_F(OwnedImplTest, ReserveCommit) { expectSlices({{1, 16383, 16384}}, buffer); buffer.addBufferFragment(fragment); EXPECT_EQ(13, buffer.length()); - auto reservation = buffer.reserveApproximately(1); - EXPECT_EQ(1, reservation.numSlices()); + auto reservation = buffer.reserveForRead(); EXPECT_NE(slice1, reservation.slices()[0].mem_); reservation.commit(1); expectSlices({{1, 16383, 16384}, {12, 0, 12}, {1, 16383, 16384}}, buffer); @@ -869,29 +859,29 @@ TEST_F(OwnedImplTest, ReserveCommitReuse) { } EXPECT_EQ(8000, buffer.length()); - // Reserve 16KB. The resulting reservation should span 2 slices. + // Reserve some space. The resulting reservation should span 2 slices. // Commit part of the first slice and none of the second slice. const void* first_slice; { expectSlices({{8000, 4288, 12288}}, buffer); - auto reservation = buffer.reserveApproximately(16384); + auto reservation = buffer.reserveForRead(); // No additional slices are added to the buffer until `commit()` is called // on the reservation. expectSlices({{8000, 4288, 12288}}, buffer); first_slice = reservation.slices()[0].mem_; - EXPECT_EQ(2, reservation.numSlices()); + EXPECT_GE(reservation.numSlices(), 2); reservation.commit(1); } EXPECT_EQ(8001, buffer.length()); // The second slice is now released because there's nothing in the second slice. expectSlices({{8001, 4287, 12288}}, buffer); - // Reserve 16KB again. + // Reserve again. { - auto reservation = buffer.reserveApproximately(16384); - EXPECT_EQ(2, reservation.numSlices()); + auto reservation = buffer.reserveForRead(); + EXPECT_GE(reservation.numSlices(), 2); EXPECT_EQ(static_cast(first_slice) + 1, static_cast(reservation.slices()[0].mem_)); } @@ -1100,7 +1090,7 @@ TEST_F(OwnedImplTest, ReserveZeroCommit) { buf.addBufferFragment(frag); buf.prepend("bbbbb"); buf.add(""); - { auto reservation = buf.reserveApproximately(1280); } + { auto reservation = buf.reserveSingleSlice(1280); } os_fd_t pipe_fds[2] = {0, 0}; auto& os_sys_calls = Api::OsSysCallsSingleton::get(); #ifdef WIN32 diff --git a/test/common/buffer/watermark_buffer_test.cc b/test/common/buffer/watermark_buffer_test.cc index d339e9fef5f5..b007625b2cd8 100644 --- a/test/common/buffer/watermark_buffer_test.cc +++ b/test/common/buffer/watermark_buffer_test.cc @@ -121,10 +121,20 @@ TEST_F(WatermarkBufferTest, PrependBuffer) { TEST_F(WatermarkBufferTest, Commit) { buffer_.add(TEN_BYTES, 10); EXPECT_EQ(0, times_high_watermark_called_); - Buffer::Reservation reservation = buffer_.reserveApproximately(10); - reservation.commit(10); + { + auto reservation = buffer_.reserveForRead(); + reservation.commit(10); + } EXPECT_EQ(1, times_high_watermark_called_); EXPECT_EQ(20, buffer_.length()); + + { + auto reservation = buffer_.reserveSingleSlice(10); + reservation.commit(10); + } + // Buffer is already above high watermark, so it won't be called a second time. + EXPECT_EQ(1, times_high_watermark_called_); + EXPECT_EQ(30, buffer_.length()); } TEST_F(WatermarkBufferTest, Drain) { diff --git a/test/extensions/filters/network/postgres_proxy/postgres_decoder_test.cc b/test/extensions/filters/network/postgres_proxy/postgres_decoder_test.cc index b906ba66c846..aa6ce4333588 100644 --- a/test/extensions/filters/network/postgres_proxy/postgres_decoder_test.cc +++ b/test/extensions/filters/network/postgres_proxy/postgres_decoder_test.cc @@ -528,7 +528,7 @@ class FakeBuffer : public Buffer::Instance { MOCK_METHOD(void*, linearize, (uint32_t), (override)); MOCK_METHOD(void, move, (Instance&), (override)); MOCK_METHOD(void, move, (Instance&, uint64_t), (override)); - MOCK_METHOD(Buffer::Reservation, reserveApproximately, (uint64_t), (override)); + MOCK_METHOD(Buffer::Reservation, reserveForRead, (), (override)); MOCK_METHOD(Buffer::ReservationSingleSlice, reserveSingleSlice, (uint64_t, bool), (override)); MOCK_METHOD(void, commit, (uint64_t, absl::Span, absl::Span), diff --git a/test/extensions/transport_sockets/tls/tls_throughput_benchmark.cc b/test/extensions/transport_sockets/tls/tls_throughput_benchmark.cc index 75a7f46455cd..f1e90313b0ca 100644 --- a/test/extensions/transport_sockets/tls/tls_throughput_benchmark.cc +++ b/test/extensions/transport_sockets/tls/tls_throughput_benchmark.cc @@ -48,18 +48,15 @@ static void appendSlice(Buffer::Instance& buffer, uint32_t size) { // If move_slices is true, add full-sized slices using move similar to how HTTP codecs move data // from the filter chain buffer to the output buffer. Else, append full-sized slices directly to the // output buffer like socket read would do. -static void addFullSlices(Buffer::Instance& output_buffer, int num_slices, bool move_slices) { +static void addFullSlices(Buffer::Instance& output_buffer, unsigned num_slices, bool move_slices) { Buffer::OwnedImpl tmp_buf; Buffer::Instance* buffer = move_slices ? &tmp_buf : &output_buffer; - for (int i = 0; i < num_slices; i++) { - auto start_size = buffer->length(); - Buffer::Reservation reservation = buffer->reserveApproximately(16384); - for (unsigned i = 0; i < reservation.numSlices(); i++) { - memset(reservation.slices()[i].mem_, 'a', reservation.slices()[i].len_); - } - reservation.commit(reservation.length()); - RELEASE_ASSERT(buffer->length() - start_size == 16384, "correct reserve/commit"); + const auto initial_slices = buffer->getRawSlices().size(); + while ((buffer->getRawSlices().size() - initial_slices) < num_slices) { + Buffer::Reservation reservation = buffer->reserveForRead(); + memset(reservation.slices()[0].mem_, 'a', reservation.slices()[0].len_); + reservation.commit(reservation.slices()[0].len_); } if (move_slices) { diff --git a/test/integration/socket_interface_integration_test.cc b/test/integration/socket_interface_integration_test.cc index 7535f0bb7928..235867e57739 100644 --- a/test/integration/socket_interface_integration_test.cc +++ b/test/integration/socket_interface_integration_test.cc @@ -136,10 +136,11 @@ TEST_P(SocketInterfaceIntegrationTest, UdpSendToInternalAddressWithSocketInterfa local_valid_address, nullptr); Buffer::OwnedImpl buffer; - auto reservation = buffer.reserveApproximately(100); + auto reservation = buffer.reserveSingleSlice(100); - auto result = socket->ioHandle().sendmsg(reservation.slices(), reservation.numSlices(), 0, - local_valid_address->ip(), *peer_internal_address); + auto slice = reservation.slice(); + auto result = + socket->ioHandle().sendmsg(&slice, 1, 0, local_valid_address->ip(), *peer_internal_address); ASSERT_FALSE(result.ok()); ASSERT_EQ(result.err_->getErrorCode(), Api::IoError::IoErrorCode::NoSupport); } From 112e3bd3472e5b2d8ac16b6f2783901b56f18097 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Thu, 21 Jan 2021 12:13:17 -0800 Subject: [PATCH 27/46] reset length_ on commit() Signed-off-by: Greg Greenway --- include/envoy/buffer/buffer.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/envoy/buffer/buffer.h b/include/envoy/buffer/buffer.h index c1aabc16a89c..7ebed41cd4b6 100644 --- a/include/envoy/buffer/buffer.h +++ b/include/envoy/buffer/buffer.h @@ -492,6 +492,7 @@ class Reservation final { void commit(uint64_t length) { ENVOY_BUG(length <= length_, "commit() length must be <= size of the Reservation"); buffer_.commit(length, absl::MakeSpan(slices_), absl::MakeSpan(owned_slices_)); + length_ = 0; slices_.clear(); owned_slices_.clear(); } From 6195334e2ce6c5e734d347e8c5ae90bbc230f8b3 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Thu, 21 Jan 2021 12:16:51 -0800 Subject: [PATCH 28/46] check for double-commit() Signed-off-by: Greg Greenway --- include/envoy/buffer/buffer.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/include/envoy/buffer/buffer.h b/include/envoy/buffer/buffer.h index 7ebed41cd4b6..5c28c9726c83 100644 --- a/include/envoy/buffer/buffer.h +++ b/include/envoy/buffer/buffer.h @@ -491,6 +491,8 @@ class Reservation final { */ void commit(uint64_t length) { ENVOY_BUG(length <= length_, "commit() length must be <= size of the Reservation"); + ASSERT(length == 0 || !slices_.empty(), + "Reservation.commit() called on empty Reservation; possible double-commit()."); buffer_.commit(length, absl::MakeSpan(slices_), absl::MakeSpan(owned_slices_)); length_ = 0; slices_.clear(); @@ -557,6 +559,8 @@ class ReservationSingleSlice final { */ void commit(uint64_t length) { ENVOY_BUG(length <= slice_.len_, "commit() length must be <= size of the Reservation"); + ASSERT(length == 0 || slice_.mem_ != nullptr, + "Reservation.commit() called on empty Reservation; possible double-commit()."); buffer_.commit(length, absl::MakeSpan(&slice_, 1), absl::MakeSpan(&owned_slice_, 1)); slice_ = {nullptr, 0}; owned_slice_.reset(); From 3475f1d3f928fbd837f70f9e4e1985ccccdf63c3 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Thu, 21 Jan 2021 13:37:17 -0800 Subject: [PATCH 29/46] remove assert that isn't true in fuzz corpus Signed-off-by: Greg Greenway --- test/common/buffer/buffer_fuzz.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/test/common/buffer/buffer_fuzz.cc b/test/common/buffer/buffer_fuzz.cc index 7835fc9d496c..1e23da3e2645 100644 --- a/test/common/buffer/buffer_fuzz.cc +++ b/test/common/buffer/buffer_fuzz.cc @@ -277,7 +277,6 @@ uint32_t bufferAction(Context& ctxt, char insert_value, uint32_t max_alloc, Buff if (reserve_length < 16384) { auto reservation = target_buffer.reserveSingleSlice(reserve_length); ::memset(reservation.slice().mem_, insert_value, reservation.slice().len_); - RELEASE_ASSERT(action.reserve_commit().commit_length() <= reserve_length, ""); reservation.commit( std::min(action.reserve_commit().commit_length(), reservation.length())); } else { From f8d2a72ca4579505a24598b0a858efcf0172cebd Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Thu, 21 Jan 2021 13:37:40 -0800 Subject: [PATCH 30/46] ensure we don't overflow the inline-vector in Reservation Signed-off-by: Greg Greenway --- source/common/buffer/buffer_impl.cc | 5 +---- test/common/buffer/owned_impl_test.cc | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/source/common/buffer/buffer_impl.cc b/source/common/buffer/buffer_impl.cc index 43c12256498d..7f3b66161f4d 100644 --- a/source/common/buffer/buffer_impl.cc +++ b/source/common/buffer/buffer_impl.cc @@ -332,7 +332,7 @@ Reservation OwnedImpl::reserveWithLength(uint64_t length) { reserved += slice.len_; } - while (bytes_remaining != 0) { + while (bytes_remaining != 0 && reservation_slices.size() < reservation.MAX_SLICES_) { const uint64_t size = Slice::default_slice_size_; // If the next slice would go over the desired size, and the amount already reserved is already @@ -348,9 +348,6 @@ Reservation OwnedImpl::reserveWithLength(uint64_t length) { reservation_owned_slices.emplace_back(std::make_unique(std::move(slice))); bytes_remaining -= std::min(reservation_slices.back().len_, bytes_remaining); reserved += reservation_slices.back().len_; - - ASSERT(reservation_slices.size() <= reservation.MAX_SLICES_, - "The absl::InlineVector should be sized so that out-of-line storage isn't needed."); } reservation.bufferImplUseOnlySetLength(reserved); diff --git a/test/common/buffer/owned_impl_test.cc b/test/common/buffer/owned_impl_test.cc index fc83e04c7939..e2386650b6ba 100644 --- a/test/common/buffer/owned_impl_test.cc +++ b/test/common/buffer/owned_impl_test.cc @@ -842,6 +842,25 @@ TEST_F(OwnedImplTest, ReserveCommit) { } EXPECT_EQ(14, buffer.length()); } + + { + Buffer::OwnedImpl buffer; + uint64_t default_reservation_length; + uint64_t default_slice_length; + { + auto reservation = buffer.reserveForRead(); + default_reservation_length = reservation.length(); + default_slice_length = reservation.slices()[0].len_; + reservation.commit(default_slice_length / 2); + } + { + // Test that the Reservation size is capped at the available space in the Reservation + // inline storage, including using the end of a previous slice, no matter how big the request + // is. + auto reservation = buffer.reserveForReadWithLengthForTest(UINT64_MAX); + EXPECT_EQ(reservation.length(), default_reservation_length - (default_slice_length / 2)); + } + } } TEST_F(OwnedImplTest, ReserveCommitReuse) { From 1e20eeb68031031809ef4946adeae30993cd9f7b Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Fri, 22 Jan 2021 11:11:52 -0800 Subject: [PATCH 31/46] fix buffer_fuzz Signed-off-by: Greg Greenway --- test/common/buffer/buffer_fuzz.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/test/common/buffer/buffer_fuzz.cc b/test/common/buffer/buffer_fuzz.cc index 1e23da3e2645..0da9ec1bc37e 100644 --- a/test/common/buffer/buffer_fuzz.cc +++ b/test/common/buffer/buffer_fuzz.cc @@ -148,6 +148,7 @@ class StringBuffer : public Buffer::Instance { slice.len_ = data_.size() - (start_ + size_); reservation.bufferImplUseOnlySlices().push_back(slice); reservation.bufferImplUseOnlyOwnedSlices().push_back(nullptr); + reservation.bufferImplUseOnlySetLength(slice.len_); return reservation; } From d3559f833fc50ae465f27277193744af1ef45310 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Fri, 22 Jan 2021 11:20:14 -0800 Subject: [PATCH 32/46] reserveWithMaxLength Signed-off-by: Greg Greenway --- source/common/buffer/buffer_impl.cc | 10 +++++----- source/common/buffer/buffer_impl.h | 8 +++++--- source/common/buffer/watermark_buffer.cc | 2 +- 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/source/common/buffer/buffer_impl.cc b/source/common/buffer/buffer_impl.cc index 7f3b66161f4d..61dd86a8bb2b 100644 --- a/source/common/buffer/buffer_impl.cc +++ b/source/common/buffer/buffer_impl.cc @@ -301,12 +301,12 @@ void OwnedImpl::move(Instance& rhs, uint64_t length) { } Reservation OwnedImpl::reserveForRead() { - return reserveWithLength(default_read_reservation_size_); + return reserveWithMaxLength(default_read_reservation_size_); } -Reservation OwnedImpl::reserveWithLength(uint64_t length) { +Reservation OwnedImpl::reserveWithMaxLength(uint64_t max_length) { Reservation reservation = Reservation::bufferImplUseOnlyConstruct(*this); - if (length == 0) { + if (max_length == 0) { return reservation; } @@ -315,14 +315,14 @@ Reservation OwnedImpl::reserveWithLength(uint64_t length) { slices_.pop_back(); } - uint64_t bytes_remaining = length; + uint64_t bytes_remaining = max_length; uint64_t reserved = 0; auto& reservation_slices = reservation.bufferImplUseOnlySlices(); auto& reservation_owned_slices = reservation.bufferImplUseOnlyOwnedSlices(); // Check whether there are any empty slices with reservable space at the end of the buffer. uint64_t reservable_size = slices_.empty() ? 0 : slices_.back().reservableSize(); - if (reservable_size >= length || reservable_size >= (Slice::default_slice_size_ / 8)) { + if (reservable_size >= max_length || reservable_size >= (Slice::default_slice_size_ / 8)) { auto& last_slice = slices_.back(); const uint64_t reservation_size = std::min(last_slice.reservableSize(), bytes_remaining); auto slice = last_slice.reserve(reservation_size); diff --git a/source/common/buffer/buffer_impl.h b/source/common/buffer/buffer_impl.h index df8ccef9f241..bb3c42955d4d 100644 --- a/source/common/buffer/buffer_impl.h +++ b/source/common/buffer/buffer_impl.h @@ -661,16 +661,18 @@ class OwnedImpl : public LibEventInstance { /** * Create a reservation for reading with a non-default length. Used in benchmark tests. */ - Reservation reserveForReadWithLengthForTest(uint64_t length) { return reserveWithLength(length); } + Reservation reserveForReadWithLengthForTest(uint64_t length) { + return reserveWithMaxLength(length); + } protected: static constexpr uint64_t default_read_reservation_size_ = Reservation::MAX_SLICES_ * Slice::default_slice_size_; /** - * Create a reservation with a specific length. + * Create a reservation with a maximum length. */ - Reservation reserveWithLength(uint64_t length); + Reservation reserveWithMaxLength(uint64_t max_length); void commit(uint64_t length, absl::Span slices, absl::Span owned_slices) override; diff --git a/source/common/buffer/watermark_buffer.cc b/source/common/buffer/watermark_buffer.cc index dd68966eff4d..9a4df0af6d79 100644 --- a/source/common/buffer/watermark_buffer.cc +++ b/source/common/buffer/watermark_buffer.cc @@ -78,7 +78,7 @@ Reservation WatermarkBuffer::reserveForRead() { } } - return OwnedImpl::reserveWithLength(adjusted_length); + return OwnedImpl::reserveWithMaxLength(adjusted_length); } void WatermarkBuffer::appendSliceForTest(const void* data, uint64_t size) { From 54d01624521813070f173eaeeb704eecb256322d Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Fri, 22 Jan 2021 11:43:38 -0800 Subject: [PATCH 33/46] add test for ENVOY_BUG Signed-off-by: Greg Greenway --- test/common/buffer/owned_impl_test.cc | 36 +++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/test/common/buffer/owned_impl_test.cc b/test/common/buffer/owned_impl_test.cc index e2386650b6ba..3b180660148c 100644 --- a/test/common/buffer/owned_impl_test.cc +++ b/test/common/buffer/owned_impl_test.cc @@ -948,6 +948,42 @@ TEST_F(OwnedImplTest, ReserveReuse) { } } +// Test behavior when the size to commit() is larger than the reservation. +TEST_F(OwnedImplTest, ReserveOverCommit) { + Buffer::OwnedImpl buffer; + auto reservation = buffer.reserveForRead(); + const auto reservation_length = reservation.length(); + const auto excess_length = reservation_length + 1; +#ifdef NDEBUG + reservation.commit(excess_length); + + // The length should be the Reservation length, not the value passed to commit. + EXPECT_EQ(reservation_length, buffer.length()); +#else + EXPECT_DEATH( + reservation.commit(excess_length), + "length <= length_. Details: commit\\(\\) length must be <= size of the Reservation"); +#endif +} + +// Test behavior when the size to commit() is larger than the reservation. +TEST_F(OwnedImplTest, ReserveSingleOverCommit) { + Buffer::OwnedImpl buffer; + auto reservation = buffer.reserveSingleSlice(10); + const auto reservation_length = reservation.length(); + const auto excess_length = reservation_length + 1; +#ifdef NDEBUG + reservation.commit(excess_length); + + // The length should be the Reservation length, not the value passed to commit. + EXPECT_EQ(reservation_length, buffer.length()); +#else + EXPECT_DEATH( + reservation.commit(excess_length), + "length <= length_. Details: commit\\(\\) length must be <= size of the Reservation"); +#endif +} + TEST_F(OwnedImplTest, Search) { // Populate a buffer with a string split across many small slices, to // exercise edge cases in the search implementation. From c8377042ab3ac82c96b18c53a0f936f981f37657 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Fri, 22 Jan 2021 12:15:12 -0800 Subject: [PATCH 34/46] make IoHandle::read() max_length optional Signed-off-by: Greg Greenway --- include/envoy/network/io_handle.h | 6 ++++-- source/common/network/io_socket_handle_impl.cc | 4 +++- source/common/network/io_socket_handle_impl.h | 3 ++- source/common/network/raw_buffer_socket.cc | 5 +---- .../quic_listeners/quiche/quic_io_handle_wrapper.h | 3 ++- test/mocks/network/io_handle.h | 3 ++- 6 files changed, 14 insertions(+), 10 deletions(-) diff --git a/include/envoy/network/io_handle.h b/include/envoy/network/io_handle.h index de530474592a..ab5289507fdc 100644 --- a/include/envoy/network/io_handle.h +++ b/include/envoy/network/io_handle.h @@ -70,11 +70,13 @@ class IoHandle { /** * Read from a io handle directly into buffer. * @param buffer supplies the buffer to read into. - * @param max_length supplies the maximum length to read. + * @param max_length supplies the maximum length to read. A value of absl::nullopt means to read + * as much data as possible, within the constraints of available buffer size. * @return a IoCallUint64Result with err_ = nullptr and rc_ = the number of bytes * read if successful, or err_ = some IoError for failure. If call failed, rc_ shouldn't be used. */ - virtual Api::IoCallUint64Result read(Buffer::Instance& buffer, uint64_t max_length) PURE; + virtual Api::IoCallUint64Result read(Buffer::Instance& buffer, + absl::optional max_length) PURE; /** * Write the data in slices out. diff --git a/source/common/network/io_socket_handle_impl.cc b/source/common/network/io_socket_handle_impl.cc index e6d0451a8595..b71a2273c96f 100644 --- a/source/common/network/io_socket_handle_impl.cc +++ b/source/common/network/io_socket_handle_impl.cc @@ -108,7 +108,9 @@ Api::IoCallUint64Result IoSocketHandleImpl::readv(uint64_t max_length, Buffer::R return result; } -Api::IoCallUint64Result IoSocketHandleImpl::read(Buffer::Instance& buffer, uint64_t max_length) { +Api::IoCallUint64Result IoSocketHandleImpl::read(Buffer::Instance& buffer, + absl::optional max_length_opt) { + const uint64_t max_length = max_length_opt.value_or(UINT64_MAX); if (max_length == 0) { return Api::ioCallUint64ResultNoError(); } diff --git a/source/common/network/io_socket_handle_impl.h b/source/common/network/io_socket_handle_impl.h index 41603976a736..7fbb2c39d274 100644 --- a/source/common/network/io_socket_handle_impl.h +++ b/source/common/network/io_socket_handle_impl.h @@ -33,7 +33,8 @@ class IoSocketHandleImpl : public IoHandle, protected Logger::Loggable max_length) override; Api::IoCallUint64Result writev(const Buffer::RawSlice* slices, uint64_t num_slice) override; diff --git a/source/common/network/raw_buffer_socket.cc b/source/common/network/raw_buffer_socket.cc index 99e37c44bcdd..96f75a1c419b 100644 --- a/source/common/network/raw_buffer_socket.cc +++ b/source/common/network/raw_buffer_socket.cc @@ -18,10 +18,7 @@ IoResult RawBufferSocket::doRead(Buffer::Instance& buffer) { uint64_t bytes_read = 0; bool end_stream = false; do { - // The maximum read size will be limited by lower layers, based on configured buffer sizes. - // Specifying UINT64_MAX indicates that this layer is not imposing any limit beyond what - // the lower layers impose. - Api::IoCallUint64Result result = callbacks_->ioHandle().read(buffer, UINT64_MAX); + Api::IoCallUint64Result result = callbacks_->ioHandle().read(buffer, absl::nullopt); if (result.ok()) { ENVOY_CONN_LOG(trace, "read returns: {}", callbacks_->connection(), result.rc_); diff --git a/source/extensions/quic_listeners/quiche/quic_io_handle_wrapper.h b/source/extensions/quic_listeners/quiche/quic_io_handle_wrapper.h index fc273df6038a..a2f66d52e7a4 100644 --- a/source/extensions/quic_listeners/quiche/quic_io_handle_wrapper.h +++ b/source/extensions/quic_listeners/quiche/quic_io_handle_wrapper.h @@ -29,7 +29,8 @@ class QuicIoHandleWrapper : public Network::IoHandle { } return io_handle_.readv(max_length, slices, num_slice); } - Api::IoCallUint64Result read(Buffer::Instance& buffer, uint64_t max_length) override { + Api::IoCallUint64Result read(Buffer::Instance& buffer, + absl::optional max_length) override { if (closed_) { return Api::IoCallUint64Result(0, Api::IoErrorPtr(new Network::IoSocketError(EBADF), Network::IoSocketError::deleteIoError)); diff --git a/test/mocks/network/io_handle.h b/test/mocks/network/io_handle.h index 132f314048d1..c154269585cc 100644 --- a/test/mocks/network/io_handle.h +++ b/test/mocks/network/io_handle.h @@ -24,7 +24,8 @@ class MockIoHandle : public IoHandle { MOCK_METHOD(bool, isOpen, (), (const)); MOCK_METHOD(Api::IoCallUint64Result, readv, (uint64_t max_length, Buffer::RawSlice* slices, uint64_t num_slice)); - MOCK_METHOD(Api::IoCallUint64Result, read, (Buffer::Instance & buffer, uint64_t max_length)); + MOCK_METHOD(Api::IoCallUint64Result, read, + (Buffer::Instance & buffer, absl::optional max_length)); MOCK_METHOD(Api::IoCallUint64Result, writev, (const Buffer::RawSlice* slices, uint64_t num_slice)); MOCK_METHOD(Api::IoCallUint64Result, write, (Buffer::Instance & buffer)); From 530e045ce5a93d269b71c5374a43295d29d0752c Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Fri, 22 Jan 2021 13:36:40 -0800 Subject: [PATCH 35/46] Add test for freelist; fixes to make it work like a stack Signed-off-by: Greg Greenway --- include/envoy/buffer/buffer.h | 8 ++++- source/common/buffer/buffer_impl.cc | 20 +++++------ source/common/buffer/buffer_impl.h | 7 +++- test/common/buffer/buffer_speed_test.cc | 14 ++++++-- test/common/buffer/owned_impl_test.cc | 48 ++++++++++++++++++++++++- 5 files changed, 80 insertions(+), 17 deletions(-) diff --git a/include/envoy/buffer/buffer.h b/include/envoy/buffer/buffer.h index 5c28c9726c83..a9b13dbbde05 100644 --- a/include/envoy/buffer/buffer.h +++ b/include/envoy/buffer/buffer.h @@ -464,7 +464,13 @@ using WatermarkFactorySharedPtr = std::shared_ptr; class Reservation final { public: Reservation(Reservation&&) = default; - ~Reservation() = default; + ~Reservation() { + // Free in reverse-order so that entries go back onto the free-list in the same order they came + // off, to slightly improve locality in the case that not all slices were pulled into cache. + while (!owned_slices_.empty()) { + owned_slices_.pop_back(); + } + } /** * @return an array of `RawSlice` of length `numSlices()`. diff --git a/source/common/buffer/buffer_impl.cc b/source/common/buffer/buffer_impl.cc index 61dd86a8bb2b..623109fdd38a 100644 --- a/source/common/buffer/buffer_impl.cc +++ b/source/common/buffer/buffer_impl.cc @@ -389,24 +389,20 @@ void OwnedImpl::commit(uint64_t length, absl::Span slices, absl::Span owned_slices) { ASSERT(slices.size() == owned_slices.size()); uint64_t bytes_remaining = length; - for (uint32_t i = 0; i < slices.size(); i++) { + for (uint32_t i = 0; i < slices.size() && bytes_remaining > 0; i++) { ASSERT((owned_slices[i] != nullptr) == (dynamic_cast(owned_slices[i].get()) != nullptr)); std::unique_ptr owned_slice( static_cast(owned_slices[i].release())); - if (bytes_remaining > 0) { - if (owned_slice != nullptr) { - slices_.emplace_back(std::move(owned_slice->slice_)); - } - slices[i].len_ = std::min(slices[i].len_, bytes_remaining); - bool success = slices_.back().commit(slices[i]); - ASSERT(success); - length_ += slices[i].len_; - bytes_remaining -= slices[i].len_; - } else { - owned_slice.reset(); + if (owned_slice != nullptr) { + slices_.emplace_back(std::move(owned_slice->slice_)); } + slices[i].len_ = std::min(slices[i].len_, bytes_remaining); + bool success = slices_.back().commit(slices[i]); + ASSERT(success); + length_ += slices[i].len_; + bytes_remaining -= slices[i].len_; } } diff --git a/source/common/buffer/buffer_impl.h b/source/common/buffer/buffer_impl.h index bb3c42955d4d..f234f7577964 100644 --- a/source/common/buffer/buffer_impl.h +++ b/source/common/buffer/buffer_impl.h @@ -78,6 +78,7 @@ class Slice { if (this != &rhs) { callAndClearDrainTrackers(); + freeStorage(std::move(storage_), capacity_); storage_ = std::move(rhs.storage_); drain_trackers_ = std::move(rhs.drain_trackers_); base_ = rhs.base_; @@ -306,10 +307,14 @@ class Slice { static StoragePtr newStorage(uint64_t capacity) { ASSERT(sliceSize(default_slice_size_) == default_slice_size_, "default_slice_size_ incompatible with sliceSize()"); + ASSERT(sliceSize(capacity) == capacity, + "newStorage should only be called on values returned from sliceSize()"); StoragePtr storage; if (capacity == default_slice_size_ && !free_list_.empty()) { storage = std::move(free_list_.back()); + ASSERT(storage != nullptr); + ASSERT(free_list_.back() == nullptr); free_list_.pop_back(); } else { storage.reset(new uint8_t[capacity]); @@ -331,7 +336,7 @@ class Slice { } } - static constexpr uint32_t free_list_max_ = 8; + static constexpr uint32_t free_list_max_ = Buffer::Reservation::MAX_SLICES_; static thread_local absl::InlinedVector free_list_; /** Length of the byte array that base_ points to. This is also the offset in bytes from the start diff --git a/test/common/buffer/buffer_speed_test.cc b/test/common/buffer/buffer_speed_test.cc index b2a00e3418f8..67d5f5250c12 100644 --- a/test/common/buffer/buffer_speed_test.cc +++ b/test/common/buffer/buffer_speed_test.cc @@ -228,7 +228,12 @@ static void bufferReserveCommit(benchmark::State& state) { } benchmark::DoNotOptimize(buffer.length()); } -BENCHMARK(bufferReserveCommit)->Arg(1)->Arg(4096)->Arg(16384)->Arg(65536); +BENCHMARK(bufferReserveCommit) + ->Arg(1) + ->Arg(4 * 1024) + ->Arg(16 * 1024) + ->Arg(64 * 1024) + ->Arg(128 * 1024); // Test the reserve+commit cycle, for the common case where the reserved space is // only partially used (and therefore the commit size is smaller than the reservation size). @@ -246,7 +251,12 @@ static void bufferReserveCommitPartial(benchmark::State& state) { } benchmark::DoNotOptimize(buffer.length()); } -BENCHMARK(bufferReserveCommitPartial)->Arg(1)->Arg(4096)->Arg(16384)->Arg(65536); +BENCHMARK(bufferReserveCommitPartial) + ->Arg(1) + ->Arg(4 * 1024) + ->Arg(16 * 1024) + ->Arg(64 * 1024) + ->Arg(128 * 1024); // Test the linearization of a buffer in the best case where the data is in one slice. static void bufferLinearizeSimple(benchmark::State& state) { diff --git a/test/common/buffer/owned_impl_test.cc b/test/common/buffer/owned_impl_test.cc index 3b180660148c..cb4c9d8f704a 100644 --- a/test/common/buffer/owned_impl_test.cc +++ b/test/common/buffer/owned_impl_test.cc @@ -980,10 +980,56 @@ TEST_F(OwnedImplTest, ReserveSingleOverCommit) { #else EXPECT_DEATH( reservation.commit(excess_length), - "length <= length_. Details: commit\\(\\) length must be <= size of the Reservation"); + "length <= slice_.len_. Details: commit\\(\\) length must be <= size of the Reservation"); #endif } +// Test functionality of the freelist (a performance optimization) +TEST_F(OwnedImplTest, SliceFreeList) { + Buffer::OwnedImpl b1, b2; + void* slice1; + void* slice2; + { + auto r = b1.reserveForRead(); + slice1 = r.slices()[0].mem_; + slice2 = r.slices()[1].mem_; + r.commit(1); + EXPECT_EQ(slice1, b1.getRawSlices()[0].mem_); + } + + { + auto r = b2.reserveForRead(); + EXPECT_EQ(slice2, r.slices()[0].mem_); + r.commit(1); + EXPECT_EQ(slice2, b2.getRawSlices()[0].mem_); + } + + b1.drain(1); + EXPECT_EQ(0, b1.getRawSlices().size()); + { + auto r = b2.reserveForRead(); + // slices()[0] is the partially used slice that is already part of this buffer. + EXPECT_EQ(slice1, r.slices()[1].mem_); + } + { + auto r = b1.reserveForRead(); + EXPECT_EQ(slice1, r.slices()[0].mem_); + } + { + // This underflows the freelist on creation, and overflows it on deletion. + auto r1 = b1.reserveForRead(); + auto r2 = b2.reserveForRead(); + for (auto& r1_slice : absl::MakeSpan(r1.slices(), r1.numSlices())) { + // r1 reservation does not contain the slice that is a part of b2. + EXPECT_NE(r1_slice.mem_, b2.getRawSlices()[0].mem_); + for (auto& r2_slice : absl::MakeSpan(r2.slices(), r2.numSlices())) { + // The two reservations do not share any slices. + EXPECT_NE(r1_slice.mem_, r2_slice.mem_); + } + } + } +} + TEST_F(OwnedImplTest, Search) { // Populate a buffer with a string split across many small slices, to // exercise edge cases in the search implementation. From 0c9252940bbdeed0232dbec6c2a021819e3fb0af Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Mon, 25 Jan 2021 12:43:13 -0800 Subject: [PATCH 36/46] fix ordering of release notes (auto-merge got wrong order) Signed-off-by: Greg Greenway --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 58aa529e3678..3151acb59ed1 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -9,8 +9,8 @@ Minor Behavior Changes ---------------------- *Changes that may cause incompatibilities for some users, but should not for most* -* perf: allow reading more bytes per operation from raw sockets to improve performance. * oauth filter: added the optional parameter :ref:`auth_scopes ` with default value of 'user' if not provided. Enables this value to be overridden in the Authorization request to the OAuth provider. +* perf: allow reading more bytes per operation from raw sockets to improve performance. * tcp: setting NODELAY in the base connection class. This should have no effect for TCP or HTTP proxying, but may improve throughput in other areas. This behavior can be temporarily reverted by setting `envoy.reloadable_features.always_nodelay` to false. * upstream: host weight changes now cause a full load balancer rebuild as opposed to happening atomically inline. This change has been made to support load balancer pre-computation of data From 2cfd939f5598eaaa73a05ee8f8eaa3fab3fa7729 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Mon, 25 Jan 2021 12:43:36 -0800 Subject: [PATCH 37/46] fix spelling Signed-off-by: Greg Greenway --- test/common/buffer/owned_impl_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/common/buffer/owned_impl_test.cc b/test/common/buffer/owned_impl_test.cc index cb4c9d8f704a..122bf833100a 100644 --- a/test/common/buffer/owned_impl_test.cc +++ b/test/common/buffer/owned_impl_test.cc @@ -984,7 +984,7 @@ TEST_F(OwnedImplTest, ReserveSingleOverCommit) { #endif } -// Test functionality of the freelist (a performance optimization) +// Test functionality of the `freelist` (a performance optimization) TEST_F(OwnedImplTest, SliceFreeList) { Buffer::OwnedImpl b1, b2; void* slice1; @@ -1016,7 +1016,7 @@ TEST_F(OwnedImplTest, SliceFreeList) { EXPECT_EQ(slice1, r.slices()[0].mem_); } { - // This underflows the freelist on creation, and overflows it on deletion. + // This causes an underflow in the `freelist` on creation, and overflows it on deletion. auto r1 = b1.reserveForRead(); auto r2 = b2.reserveForRead(); for (auto& r1_slice : absl::MakeSpan(r1.slices(), r1.numSlices())) { From 0c26ceb68fdd4baa37c8c92079f36593c559dd20 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Mon, 25 Jan 2021 16:32:54 -0800 Subject: [PATCH 38/46] slightly improve speed_test Signed-off-by: Greg Greenway --- test/common/buffer/buffer_speed_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/buffer/buffer_speed_test.cc b/test/common/buffer/buffer_speed_test.cc index 67d5f5250c12..653a5bea770a 100644 --- a/test/common/buffer/buffer_speed_test.cc +++ b/test/common/buffer/buffer_speed_test.cc @@ -221,7 +221,7 @@ static void bufferReserveCommit(benchmark::State& state) { UNREFERENCED_PARAMETER(_); auto size = state.range(0); Buffer::Reservation reservation = buffer.reserveForReadWithLengthForTest(size); - reservation.commit(size); + reservation.commit(reservation.length()); if (buffer.length() >= MaxBufferLength) { buffer.drain(buffer.length()); } From 07a9314fa45e876818b5374d21656df64191ece1 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Mon, 25 Jan 2021 17:02:03 -0800 Subject: [PATCH 39/46] only allocate one block for impl to track ownership in reservation, instead of 1 block per slice Signed-off-by: Greg Greenway --- include/envoy/buffer/buffer.h | 44 ++++++++++--------- source/common/buffer/buffer_impl.cc | 39 +++++++++------- source/common/buffer/buffer_impl.h | 18 +++++++- source/common/buffer/watermark_buffer.cc | 4 +- source/common/buffer/watermark_buffer.h | 2 +- test/common/buffer/buffer_fuzz.cc | 3 +- test/common/buffer/owned_impl_test.cc | 6 +++ .../postgres_proxy/postgres_decoder_test.cc | 3 +- 8 files changed, 75 insertions(+), 44 deletions(-) diff --git a/include/envoy/buffer/buffer.h b/include/envoy/buffer/buffer.h index a9b13dbbde05..a4bdfc9459da 100644 --- a/include/envoy/buffer/buffer.h +++ b/include/envoy/buffer/buffer.h @@ -76,6 +76,15 @@ using SliceDataPtr = std::unique_ptr; class Reservation; class ReservationSingleSlice; +// Base class for an object to manage the ownership for slices in a `Reservation` or +// `ReservationSingleSlice`. +class ReservationSlicesOwner { +public: + virtual ~ReservationSlicesOwner() = default; +}; + +using ReservationSlicesOwnerPtr = std::unique_ptr; + /** * A basic buffer abstraction. */ @@ -427,7 +436,7 @@ class Instance { * reservation. */ virtual void commit(uint64_t length, absl::Span slices, - absl::Span owned_slices) PURE; + ReservationSlicesOwnerPtr slices_owner) PURE; }; using InstancePtr = std::unique_ptr; @@ -464,13 +473,7 @@ using WatermarkFactorySharedPtr = std::shared_ptr; class Reservation final { public: Reservation(Reservation&&) = default; - ~Reservation() { - // Free in reverse-order so that entries go back onto the free-list in the same order they came - // off, to slightly improve locality in the case that not all slices were pulled into cache. - while (!owned_slices_.empty()) { - owned_slices_.pop_back(); - } - } + ~Reservation() = default; /** * @return an array of `RawSlice` of length `numSlices()`. @@ -499,10 +502,10 @@ class Reservation final { ENVOY_BUG(length <= length_, "commit() length must be <= size of the Reservation"); ASSERT(length == 0 || !slices_.empty(), "Reservation.commit() called on empty Reservation; possible double-commit()."); - buffer_.commit(length, absl::MakeSpan(slices_), absl::MakeSpan(owned_slices_)); + buffer_.commit(length, absl::MakeSpan(slices_), std::move(slices_owner_)); length_ = 0; slices_.clear(); - owned_slices_.clear(); + ASSERT(slices_owner_ == nullptr); } // Tuned to allow reads of 128k, using 16k slices. @@ -520,10 +523,9 @@ class Reservation final { // The RawSlices in the reservation, usable by operations such as `::readv()`. absl::InlinedVector slices_; - // An array of the same length as `slices_`, in which each element is either nullptr - // if no operation needs to be performed to transfer ownership during commit/destructor, - // or a pointer to the object that needs to be moved/deleted. - absl::InlinedVector owned_slices_; + // An owner that can be set by the creator of the `Reservation` to free slices upon + // destruction. + ReservationSlicesOwnerPtr slices_owner_; public: // The following are for use only by implementations of Buffer. Because c++ @@ -531,7 +533,7 @@ class Reservation final { // misuse easy to spot in a code review. static Reservation bufferImplUseOnlyConstruct(Instance& buffer) { return Reservation(buffer); } decltype(slices_)& bufferImplUseOnlySlices() { return slices_; } - decltype(owned_slices_)& bufferImplUseOnlyOwnedSlices() { return owned_slices_; } + ReservationSlicesOwnerPtr& bufferImplUseOnlySlicesOwner() { return slices_owner_; } void bufferImplUseOnlySetLength(uint64_t length) { length_ = length; } }; @@ -567,9 +569,9 @@ class ReservationSingleSlice final { ENVOY_BUG(length <= slice_.len_, "commit() length must be <= size of the Reservation"); ASSERT(length == 0 || slice_.mem_ != nullptr, "Reservation.commit() called on empty Reservation; possible double-commit()."); - buffer_.commit(length, absl::MakeSpan(&slice_, 1), absl::MakeSpan(&owned_slice_, 1)); + buffer_.commit(length, absl::MakeSpan(&slice_, 1), std::move(slice_owner_)); slice_ = {nullptr, 0}; - owned_slice_.reset(); + ASSERT(slice_owner_ == nullptr); } private: @@ -582,9 +584,9 @@ class ReservationSingleSlice final { // and length to read into. RawSlice slice_{}; - // Contains either nullptr if no operation needs to be performed to transfer ownership during - // commit/destructor, or a pointer to the object that needs to be moved/deleted. - SliceDataPtr owned_slice_{}; + // An owner that can be set by the creator of the `ReservationSingleSlice` to free the slice upon + // destruction. + ReservationSlicesOwnerPtr slice_owner_; public: // The following are for use only by implementations of Buffer. Because c++ @@ -594,7 +596,7 @@ class ReservationSingleSlice final { return ReservationSingleSlice(buffer); } RawSlice& bufferImplUseOnlySlice() { return slice_; } - SliceDataPtr& bufferImplUseOnlyOwnedSlice() { return owned_slice_; } + ReservationSlicesOwnerPtr& bufferImplUseOnlySliceOwner() { return slice_owner_; } }; } // namespace Buffer diff --git a/source/common/buffer/buffer_impl.cc b/source/common/buffer/buffer_impl.cc index 623109fdd38a..8231535ca31b 100644 --- a/source/common/buffer/buffer_impl.cc +++ b/source/common/buffer/buffer_impl.cc @@ -318,7 +318,7 @@ Reservation OwnedImpl::reserveWithMaxLength(uint64_t max_length) { uint64_t bytes_remaining = max_length; uint64_t reserved = 0; auto& reservation_slices = reservation.bufferImplUseOnlySlices(); - auto& reservation_owned_slices = reservation.bufferImplUseOnlyOwnedSlices(); + auto slices_owner = std::make_unique(); // Check whether there are any empty slices with reservable space at the end of the buffer. uint64_t reservable_size = slices_.empty() ? 0 : slices_.back().reservableSize(); @@ -327,7 +327,7 @@ Reservation OwnedImpl::reserveWithMaxLength(uint64_t max_length) { const uint64_t reservation_size = std::min(last_slice.reservableSize(), bytes_remaining); auto slice = last_slice.reserve(reservation_size); reservation_slices.push_back(slice); - reservation_owned_slices.push_back(nullptr); + slices_owner->owned_slices_.emplace_back(Slice()); bytes_remaining -= slice.len_; reserved += slice.len_; } @@ -345,14 +345,16 @@ Reservation OwnedImpl::reserveWithMaxLength(uint64_t max_length) { Slice slice(size); reservation_slices.push_back(slice.reserve(size)); - reservation_owned_slices.emplace_back(std::make_unique(std::move(slice))); + slices_owner->owned_slices_.emplace_back(std::move(slice)); + ASSERT(slice.dataSize() == 0); bytes_remaining -= std::min(reservation_slices.back().len_, bytes_remaining); reserved += reservation_slices.back().len_; } + ASSERT(reservation_slices.size() == slices_owner->owned_slices_.size()); + reservation.bufferImplUseOnlySlicesOwner() = std::move(slices_owner); reservation.bufferImplUseOnlySetLength(reserved); - ASSERT(reservation_slices.size() == reservation_owned_slices.size()); return reservation; } @@ -368,35 +370,42 @@ ReservationSingleSlice OwnedImpl::reserveSingleSlice(uint64_t length, bool separ } auto& reservation_slice = reservation.bufferImplUseOnlySlice(); - auto& reservation_owned_slice = reservation.bufferImplUseOnlyOwnedSlice(); + auto slice_owner = std::make_unique(); // Check whether there are any empty slices with reservable space at the end of the buffer. uint64_t reservable_size = (separate_slice || slices_.empty()) ? 0 : slices_.back().reservableSize(); if (reservable_size >= length) { reservation_slice = slices_.back().reserve(length); - reservation_owned_slice = nullptr; } else { Slice slice(length); reservation_slice = slice.reserve(length); - reservation_owned_slice = std::make_unique(std::move(slice)); + slice_owner->owned_slice_ = std::move(slice); } + reservation.bufferImplUseOnlySliceOwner() = std::move(slice_owner); + return reservation; } void OwnedImpl::commit(uint64_t length, absl::Span slices, - absl::Span owned_slices) { + ReservationSlicesOwnerPtr slices_owner_base) { + if (length == 0) { + return; + } + + ASSERT(dynamic_cast(slices_owner_base.get()) != nullptr); + std::unique_ptr slices_owner( + static_cast(slices_owner_base.release())); + + absl::Span owned_slices = slices_owner->ownedSlices(); ASSERT(slices.size() == owned_slices.size()); + uint64_t bytes_remaining = length; for (uint32_t i = 0; i < slices.size() && bytes_remaining > 0; i++) { - ASSERT((owned_slices[i] != nullptr) == - (dynamic_cast(owned_slices[i].get()) != nullptr)); - std::unique_ptr owned_slice( - static_cast(owned_slices[i].release())); - - if (owned_slice != nullptr) { - slices_.emplace_back(std::move(owned_slice->slice_)); + Slice& owned_slice = owned_slices[i]; + if (owned_slice.data() != nullptr) { + slices_.emplace_back(std::move(owned_slice)); } slices[i].len_ = std::min(slices[i].len_, bytes_remaining); bool success = slices_.back().commit(slices[i]); diff --git a/source/common/buffer/buffer_impl.h b/source/common/buffer/buffer_impl.h index f234f7577964..fb7859bbaab5 100644 --- a/source/common/buffer/buffer_impl.h +++ b/source/common/buffer/buffer_impl.h @@ -680,7 +680,7 @@ class OwnedImpl : public LibEventInstance { Reservation reserveWithMaxLength(uint64_t max_length); void commit(uint64_t length, absl::Span slices, - absl::Span owned_slices) override; + ReservationSlicesOwnerPtr slices_owner) override; private: /** @@ -705,6 +705,22 @@ class OwnedImpl : public LibEventInstance { /** Sum of the dataSize of all slices. */ OverflowDetectingUInt64 length_; + + struct OwnedImplReservationSlicesOwner : public ReservationSlicesOwner { + virtual absl::Span ownedSlices() PURE; + }; + + struct OwnedImplReservationSlicesOwnerMultiple : public OwnedImplReservationSlicesOwner { + absl::Span ownedSlices() override { return absl::MakeSpan(owned_slices_); } + + absl::InlinedVector owned_slices_; + }; + + struct OwnedImplReservationSlicesOwnerSingle : public OwnedImplReservationSlicesOwner { + absl::Span ownedSlices() override { return absl::MakeSpan(&owned_slice_, 1); } + + Slice owned_slice_; + }; }; using BufferFragmentPtr = std::unique_ptr; diff --git a/source/common/buffer/watermark_buffer.cc b/source/common/buffer/watermark_buffer.cc index 9a4df0af6d79..f7d95e5183f4 100644 --- a/source/common/buffer/watermark_buffer.cc +++ b/source/common/buffer/watermark_buffer.cc @@ -32,8 +32,8 @@ void WatermarkBuffer::prepend(Instance& data) { } void WatermarkBuffer::commit(uint64_t length, absl::Span slices, - absl::Span owned_slices) { - OwnedImpl::commit(length, slices, owned_slices); + ReservationSlicesOwnerPtr slices_owner) { + OwnedImpl::commit(length, slices, std::move(slices_owner)); checkHighAndOverflowWatermarks(); } diff --git a/source/common/buffer/watermark_buffer.h b/source/common/buffer/watermark_buffer.h index b6feda4dc615..9150cdaf54f9 100644 --- a/source/common/buffer/watermark_buffer.h +++ b/source/common/buffer/watermark_buffer.h @@ -52,7 +52,7 @@ class WatermarkBuffer : public OwnedImpl { private: void commit(uint64_t length, absl::Span slices, - absl::Span owned_slices) override; + ReservationSlicesOwnerPtr slices_owner) override; std::function below_low_watermark_; std::function above_high_watermark_; diff --git a/test/common/buffer/buffer_fuzz.cc b/test/common/buffer/buffer_fuzz.cc index 0da9ec1bc37e..f5db5b6e75ab 100644 --- a/test/common/buffer/buffer_fuzz.cc +++ b/test/common/buffer/buffer_fuzz.cc @@ -147,7 +147,6 @@ class StringBuffer : public Buffer::Instance { slice.mem_ = mutableEnd(); slice.len_ = data_.size() - (start_ + size_); reservation.bufferImplUseOnlySlices().push_back(slice); - reservation.bufferImplUseOnlyOwnedSlices().push_back(nullptr); reservation.bufferImplUseOnlySetLength(slice.len_); return reservation; @@ -167,7 +166,7 @@ class StringBuffer : public Buffer::Instance { } void commit(uint64_t length, absl::Span, - absl::Span) override { + Buffer::ReservationSlicesOwnerPtr) override { size_ += length; FUZZ_ASSERT(start_ + size_ + length <= data_.size()); } diff --git a/test/common/buffer/owned_impl_test.cc b/test/common/buffer/owned_impl_test.cc index 122bf833100a..3790cf9e2125 100644 --- a/test/common/buffer/owned_impl_test.cc +++ b/test/common/buffer/owned_impl_test.cc @@ -910,6 +910,12 @@ TEST_F(OwnedImplTest, ReserveCommitReuse) { TEST_F(OwnedImplTest, ReserveReuse) { Buffer::OwnedImpl buffer; + // Test a zero-length reservation and commit. + { + auto reservation = buffer.reserveSingleSlice(0); + reservation.commit(0); + } + // Reserve some space and leave it uncommitted. const void* first_slice; { diff --git a/test/extensions/filters/network/postgres_proxy/postgres_decoder_test.cc b/test/extensions/filters/network/postgres_proxy/postgres_decoder_test.cc index aa6ce4333588..ec1abe605d3d 100644 --- a/test/extensions/filters/network/postgres_proxy/postgres_decoder_test.cc +++ b/test/extensions/filters/network/postgres_proxy/postgres_decoder_test.cc @@ -530,8 +530,7 @@ class FakeBuffer : public Buffer::Instance { MOCK_METHOD(void, move, (Instance&, uint64_t), (override)); MOCK_METHOD(Buffer::Reservation, reserveForRead, (), (override)); MOCK_METHOD(Buffer::ReservationSingleSlice, reserveSingleSlice, (uint64_t, bool), (override)); - MOCK_METHOD(void, commit, - (uint64_t, absl::Span, absl::Span), + MOCK_METHOD(void, commit, (uint64_t, absl::Span, ReservationSlicesOwnerPtr), (override)); MOCK_METHOD(ssize_t, search, (const void*, uint64_t, size_t, size_t), (const, override)); MOCK_METHOD(bool, startsWith, (absl::string_view), (const, override)); From 6bc1fc3915676227cf8006afba85bdc780cf20e2 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Mon, 25 Jan 2021 19:03:04 -0800 Subject: [PATCH 40/46] fix build Signed-off-by: Greg Greenway --- .../filters/network/postgres_proxy/postgres_decoder_test.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/extensions/filters/network/postgres_proxy/postgres_decoder_test.cc b/test/extensions/filters/network/postgres_proxy/postgres_decoder_test.cc index ec1abe605d3d..f9ff8b482a6d 100644 --- a/test/extensions/filters/network/postgres_proxy/postgres_decoder_test.cc +++ b/test/extensions/filters/network/postgres_proxy/postgres_decoder_test.cc @@ -530,7 +530,8 @@ class FakeBuffer : public Buffer::Instance { MOCK_METHOD(void, move, (Instance&, uint64_t), (override)); MOCK_METHOD(Buffer::Reservation, reserveForRead, (), (override)); MOCK_METHOD(Buffer::ReservationSingleSlice, reserveSingleSlice, (uint64_t, bool), (override)); - MOCK_METHOD(void, commit, (uint64_t, absl::Span, ReservationSlicesOwnerPtr), + MOCK_METHOD(void, commit, + (uint64_t, absl::Span, Buffer::ReservationSlicesOwnerPtr), (override)); MOCK_METHOD(ssize_t, search, (const void*, uint64_t, size_t, size_t), (const, override)); MOCK_METHOD(bool, startsWith, (absl::string_view), (const, override)); From 9f3a7179988fe4595f95b236aef0fb17fefecb21 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Tue, 26 Jan 2021 10:30:56 -0800 Subject: [PATCH 41/46] optimize: only lookup thread_local once per reservation Signed-off-by: Greg Greenway --- source/common/buffer/buffer_impl.cc | 2 +- source/common/buffer/buffer_impl.h | 72 +++++++++++++++++++++-------- 2 files changed, 54 insertions(+), 20 deletions(-) diff --git a/source/common/buffer/buffer_impl.cc b/source/common/buffer/buffer_impl.cc index 8231535ca31b..3db10686ea7b 100644 --- a/source/common/buffer/buffer_impl.cc +++ b/source/common/buffer/buffer_impl.cc @@ -343,7 +343,7 @@ Reservation OwnedImpl::reserveWithMaxLength(uint64_t max_length) { break; } - Slice slice(size); + Slice slice(size, slices_owner->free_list_); reservation_slices.push_back(slice.reserve(size)); slices_owner->owned_slices_.emplace_back(std::move(slice)); ASSERT(slice.dataSize() == 0); diff --git a/source/common/buffer/buffer_impl.h b/source/common/buffer/buffer_impl.h index fb7859bbaab5..92d8f540e457 100644 --- a/source/common/buffer/buffer_impl.h +++ b/source/common/buffer/buffer_impl.h @@ -35,6 +35,15 @@ class Slice { using Reservation = RawSlice; using StoragePtr = std::unique_ptr; + static constexpr uint32_t free_list_max_ = Buffer::Reservation::MAX_SLICES_; + using FreeListType = absl::InlinedVector; + class FreeListReference { + private: + FreeListReference(FreeListType& free_list) : free_list_(free_list) {} + FreeListType& free_list_; + friend class Slice; + }; + /** * Create an empty Slice with 0 capacity. */ @@ -45,9 +54,9 @@ class Slice { * @param min_capacity number of bytes of space the slice should have. Actual capacity is rounded * up to the next multiple of 4kb. */ - Slice(uint64_t min_capacity) - : capacity_(sliceSize(min_capacity)), storage_(newStorage(capacity_)), base_(storage_.get()), - data_(0), reservable_(0) {} + Slice(uint64_t min_capacity, absl::optional free_list = absl::nullopt) + : capacity_(sliceSize(min_capacity)), storage_(newStorage(capacity_, free_list)), + base_(storage_.get()), data_(0), reservable_(0) {} /** * Create an immutable Slice that refers to an external buffer fragment. @@ -100,6 +109,11 @@ class Slice { freeStorage(std::move(storage_), capacity_); } + void freeStorage(FreeListReference free_list) { + callAndClearDrainTrackers(); + freeStorage(std::move(storage_), capacity_, free_list); + } + /** * @return true if the data in the slice is mutable */ @@ -292,6 +306,8 @@ class Slice { static constexpr uint32_t default_slice_size_ = 16384; + static FreeListReference freeList() { return FreeListReference(free_list_); } + protected: /** * Compute a slice size big enough to hold a specified amount of data. @@ -304,40 +320,48 @@ class Slice { return num_pages * PageSize; } - static StoragePtr newStorage(uint64_t capacity) { + static StoragePtr newStorage(uint64_t capacity, absl::optional free_list_opt) { ASSERT(sliceSize(default_slice_size_) == default_slice_size_, "default_slice_size_ incompatible with sliceSize()"); ASSERT(sliceSize(capacity) == capacity, "newStorage should only be called on values returned from sliceSize()"); + ASSERT(!free_list_opt.has_value() || &free_list_opt->free_list_ == &free_list_); StoragePtr storage; - if (capacity == default_slice_size_ && !free_list_.empty()) { - storage = std::move(free_list_.back()); - ASSERT(storage != nullptr); - ASSERT(free_list_.back() == nullptr); - free_list_.pop_back(); - } else { - storage.reset(new uint8_t[capacity]); + if (capacity == default_slice_size_ && free_list_opt.has_value()) { + FreeListType& free_list = free_list_opt->free_list_; + if (!free_list.empty()) { + storage = std::move(free_list.back()); + ASSERT(storage != nullptr); + ASSERT(free_list.back() == nullptr); + free_list.pop_back(); + return storage; + } } + storage.reset(new uint8_t[capacity]); return storage; } - static void freeStorage(StoragePtr storage, uint64_t capacity) { + static void freeStorage(StoragePtr storage, uint64_t capacity, + absl::optional free_list_opt = absl::nullopt) { if (storage == nullptr) { return; } - if (capacity == default_slice_size_ && free_list_.size() < free_list_max_) { - free_list_.emplace_back(std::move(storage)); - ASSERT(storage == nullptr); - } else { - storage.reset(); + if (capacity == default_slice_size_ && free_list_opt.has_value()) { + FreeListType& free_list = free_list_opt->free_list_; + if (free_list.size() < free_list_max_) { + free_list.emplace_back(std::move(storage)); + ASSERT(storage == nullptr); + return; + } } + + storage.reset(); } - static constexpr uint32_t free_list_max_ = Buffer::Reservation::MAX_SLICES_; - static thread_local absl::InlinedVector free_list_; + static thread_local FreeListType free_list_; /** Length of the byte array that base_ points to. This is also the offset in bytes from the start * of the slice to the end of the Reservable section. */ @@ -711,8 +735,18 @@ class OwnedImpl : public LibEventInstance { }; struct OwnedImplReservationSlicesOwnerMultiple : public OwnedImplReservationSlicesOwner { + // Optimization: get the thread_local freeList() once per Reservation, outside the loop. + OwnedImplReservationSlicesOwnerMultiple() : free_list_(Slice::freeList()) {} + + ~OwnedImplReservationSlicesOwnerMultiple() { + while (!owned_slices_.empty()) { + owned_slices_.back().freeStorage(free_list_); + owned_slices_.pop_back(); + } + } absl::Span ownedSlices() override { return absl::MakeSpan(owned_slices_); } + Slice::FreeListReference free_list_; absl::InlinedVector owned_slices_; }; From ef66dc4e1a42d01f75da7fadf3c55fe0a24c55b1 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Tue, 26 Jan 2021 11:28:01 -0800 Subject: [PATCH 42/46] fix test Signed-off-by: Greg Greenway --- test/common/buffer/owned_impl_test.cc | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/test/common/buffer/owned_impl_test.cc b/test/common/buffer/owned_impl_test.cc index 3790cf9e2125..3acff2479b1a 100644 --- a/test/common/buffer/owned_impl_test.cc +++ b/test/common/buffer/owned_impl_test.cc @@ -993,21 +993,21 @@ TEST_F(OwnedImplTest, ReserveSingleOverCommit) { // Test functionality of the `freelist` (a performance optimization) TEST_F(OwnedImplTest, SliceFreeList) { Buffer::OwnedImpl b1, b2; - void* slice1; - void* slice2; + std::vector slices; { auto r = b1.reserveForRead(); - slice1 = r.slices()[0].mem_; - slice2 = r.slices()[1].mem_; + for (auto& slice : absl::MakeSpan(r.slices(), r.numSlices())) { + slices.push_back(slice.mem_); + } r.commit(1); - EXPECT_EQ(slice1, b1.getRawSlices()[0].mem_); + EXPECT_EQ(slices[0], b1.getRawSlices()[0].mem_); } { auto r = b2.reserveForRead(); - EXPECT_EQ(slice2, r.slices()[0].mem_); + EXPECT_EQ(slices[1], r.slices()[0].mem_); r.commit(1); - EXPECT_EQ(slice2, b2.getRawSlices()[0].mem_); + EXPECT_EQ(slices[1], b2.getRawSlices()[0].mem_); } b1.drain(1); @@ -1015,11 +1015,11 @@ TEST_F(OwnedImplTest, SliceFreeList) { { auto r = b2.reserveForRead(); // slices()[0] is the partially used slice that is already part of this buffer. - EXPECT_EQ(slice1, r.slices()[1].mem_); + EXPECT_EQ(slices[2], r.slices()[1].mem_); } { auto r = b1.reserveForRead(); - EXPECT_EQ(slice1, r.slices()[0].mem_); + EXPECT_EQ(slices[2], r.slices()[0].mem_); } { // This causes an underflow in the `freelist` on creation, and overflows it on deletion. From 47d7cfda16ebcfae3c5b2262e47216e9f8557637 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Tue, 26 Jan 2021 14:16:08 -0800 Subject: [PATCH 43/46] clang-tidy Signed-off-by: Greg Greenway --- source/common/buffer/buffer_impl.cc | 1 - source/common/buffer/buffer_impl.h | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/source/common/buffer/buffer_impl.cc b/source/common/buffer/buffer_impl.cc index 3db10686ea7b..d4d795421ee9 100644 --- a/source/common/buffer/buffer_impl.cc +++ b/source/common/buffer/buffer_impl.cc @@ -346,7 +346,6 @@ Reservation OwnedImpl::reserveWithMaxLength(uint64_t max_length) { Slice slice(size, slices_owner->free_list_); reservation_slices.push_back(slice.reserve(size)); slices_owner->owned_slices_.emplace_back(std::move(slice)); - ASSERT(slice.dataSize() == 0); bytes_remaining -= std::min(reservation_slices.back().len_, bytes_remaining); reserved += reservation_slices.back().len_; } diff --git a/source/common/buffer/buffer_impl.h b/source/common/buffer/buffer_impl.h index 92d8f540e457..b217d82195ed 100644 --- a/source/common/buffer/buffer_impl.h +++ b/source/common/buffer/buffer_impl.h @@ -738,7 +738,7 @@ class OwnedImpl : public LibEventInstance { // Optimization: get the thread_local freeList() once per Reservation, outside the loop. OwnedImplReservationSlicesOwnerMultiple() : free_list_(Slice::freeList()) {} - ~OwnedImplReservationSlicesOwnerMultiple() { + ~OwnedImplReservationSlicesOwnerMultiple() override { while (!owned_slices_.empty()) { owned_slices_.back().freeStorage(free_list_); owned_slices_.pop_back(); From ea5e9756c17be611709b10598bed5148a55ae88b Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Tue, 26 Jan 2021 15:18:19 -0800 Subject: [PATCH 44/46] minor cleanup of `len_` handling Signed-off-by: Greg Greenway --- source/common/buffer/buffer_impl.cc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/source/common/buffer/buffer_impl.cc b/source/common/buffer/buffer_impl.cc index d4d795421ee9..420dd9cd1d2c 100644 --- a/source/common/buffer/buffer_impl.cc +++ b/source/common/buffer/buffer_impl.cc @@ -344,10 +344,11 @@ Reservation OwnedImpl::reserveWithMaxLength(uint64_t max_length) { } Slice slice(size, slices_owner->free_list_); - reservation_slices.push_back(slice.reserve(size)); + const auto raw_slice = slice.reserve(size); + reservation_slices.push_back(raw_slice); slices_owner->owned_slices_.emplace_back(std::move(slice)); - bytes_remaining -= std::min(reservation_slices.back().len_, bytes_remaining); - reserved += reservation_slices.back().len_; + bytes_remaining -= std::min(raw_slice.len_, bytes_remaining); + reserved += raw_slice.len_; } ASSERT(reservation_slices.size() == slices_owner->owned_slices_.size()); From 4a92b7dc26df2a040225675888ccbbff5989677e Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Wed, 27 Jan 2021 09:12:13 -0800 Subject: [PATCH 45/46] more expectations in ReserveZeroCommit Signed-off-by: Greg Greenway --- test/common/buffer/owned_impl_test.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/common/buffer/owned_impl_test.cc b/test/common/buffer/owned_impl_test.cc index 3acff2479b1a..e131e5543497 100644 --- a/test/common/buffer/owned_impl_test.cc +++ b/test/common/buffer/owned_impl_test.cc @@ -1197,7 +1197,9 @@ TEST_F(OwnedImplTest, ReserveZeroCommit) { buf.addBufferFragment(frag); buf.prepend("bbbbb"); buf.add(""); + expectSlices({{5, 0, 4096}, {0, 0, 0}}, buf); { auto reservation = buf.reserveSingleSlice(1280); } + expectSlices({{5, 0, 4096}}, buf); os_fd_t pipe_fds[2] = {0, 0}; auto& os_sys_calls = Api::OsSysCallsSingleton::get(); #ifdef WIN32 From 7b94c6f5d1ea525ff15b8536b6c20f60582ce5b0 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Wed, 27 Jan 2021 13:31:20 -0800 Subject: [PATCH 46/46] Remove a test that no longer makes sense. This test passed with default TCMalloc, but only because of implementation details of how TCMalloc worked, and it failed on ASAN and Windows. The reserve-single API no longer uses the free-list (unlike previous revisions of this PR), and unused slices are no longer stored in OwnedImpl as empty slices (unlike the code before this PR). Therefore, there are no guarantees about which specific slice memory is used between un-committed reservations; the result is determined by the malloc implementation, and there is no good reason to write a test for what that behavior may be. Signed-off-by: Greg Greenway --- test/common/buffer/owned_impl_test.cc | 47 --------------------------- 1 file changed, 47 deletions(-) diff --git a/test/common/buffer/owned_impl_test.cc b/test/common/buffer/owned_impl_test.cc index e131e5543497..52930e8adabc 100644 --- a/test/common/buffer/owned_impl_test.cc +++ b/test/common/buffer/owned_impl_test.cc @@ -907,53 +907,6 @@ TEST_F(OwnedImplTest, ReserveCommitReuse) { expectSlices({{8001, 4287, 12288}}, buffer); } -TEST_F(OwnedImplTest, ReserveReuse) { - Buffer::OwnedImpl buffer; - - // Test a zero-length reservation and commit. - { - auto reservation = buffer.reserveSingleSlice(0); - reservation.commit(0); - } - - // Reserve some space and leave it uncommitted. - const void* first_slice; - { - auto reservation = buffer.reserveSingleSlice(8200); - first_slice = reservation.slice().mem_; - } - - // Reserve more space and verify that it is not the same slice from the last reservation. - // That one was for a different size and was not committed. - const void* second_slice; - { - auto reservation = buffer.reserveSingleSlice(Slice::default_slice_size_); - EXPECT_NE(first_slice, reservation.slice().mem_); - second_slice = reservation.slice().mem_; - } - - // Repeat the last reservation and verify that it yields the same slices. Both slices - // are for the default size, so the previous one should get re-used. - { - auto reservation = buffer.reserveSingleSlice(Slice::default_slice_size_); - EXPECT_EQ(second_slice, reservation.slice().mem_); - - // Commit the most recent reservation and verify the representation. - reservation.commit(Slice::default_slice_size_); - expectSlices({{16384, 0, 16384}}, buffer); - } - - // Do another reservation. - { - auto reservation = buffer.reserveSingleSlice(Slice::default_slice_size_); - expectSlices({{16384, 0, 16384}}, buffer); - - // And commit. - reservation.commit(Slice::default_slice_size_); - expectSlices({{16384, 0, 16384}, {16384, 0, 16384}}, buffer); - } -} - // Test behavior when the size to commit() is larger than the reservation. TEST_F(OwnedImplTest, ReserveOverCommit) { Buffer::OwnedImpl buffer;