diff --git a/source/common/buffer/buffer_impl.cc b/source/common/buffer/buffer_impl.cc index f43507ac9530e..b57173a1f3b8b 100644 --- a/source/common/buffer/buffer_impl.cc +++ b/source/common/buffer/buffer_impl.cc @@ -23,9 +23,9 @@ void OwnedImpl::addImpl(const void* data, uint64_t size) { bool new_slice_needed = slices_.empty(); while (size != 0) { if (new_slice_needed) { - slices_.emplace_back(OwnedSlice::create(size)); + slices_.emplace_back(size); } - uint64_t copy_size = slices_.back()->append(src, size); + uint64_t copy_size = slices_.back().append(src, size); src += copy_size; size -= copy_size; length_ += copy_size; @@ -35,14 +35,14 @@ void OwnedImpl::addImpl(const void* data, uint64_t size) { void OwnedImpl::addDrainTracker(std::function drain_tracker) { ASSERT(!slices_.empty()); - slices_.back()->addDrainTracker(std::move(drain_tracker)); + slices_.back().addDrainTracker(std::move(drain_tracker)); } void OwnedImpl::add(const void* data, uint64_t size) { addImpl(data, size); } void OwnedImpl::addBufferFragment(BufferFragment& fragment) { length_ += fragment.size(); - slices_.emplace_back(std::make_unique(fragment)); + slices_.emplace_back(fragment); } void OwnedImpl::add(absl::string_view data) { add(data.data(), data.size()); } @@ -59,9 +59,9 @@ void OwnedImpl::prepend(absl::string_view data) { bool new_slice_needed = slices_.empty(); while (size != 0) { if (new_slice_needed) { - slices_.emplace_front(OwnedSlice::create(size)); + slices_.emplace_front(size); } - uint64_t copy_size = slices_.front()->prepend(data.data(), size); + uint64_t copy_size = slices_.front().prepend(data.data(), size); size -= copy_size; length_ += copy_size; new_slice_needed = true; @@ -72,7 +72,7 @@ void OwnedImpl::prepend(Instance& data) { ASSERT(&data != this); OwnedImpl& other = static_cast(data); while (!other.slices_.empty()) { - uint64_t slice_size = other.slices_.back()->dataSize(); + uint64_t slice_size = other.slices_.back().dataSize(); length_ += slice_size; slices_.emplace_front(std::move(other.slices_.back())); other.slices_.pop_back(); @@ -85,26 +85,26 @@ 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) { + 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; - if (!slices_[0]) { - return; - } } // 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])) { + if (slices_[slice_index].commit(iovecs[num_slices_committed])) { length_ += iovecs[num_slices_committed].len_; num_slices_committed++; } @@ -115,7 +115,7 @@ void OwnedImpl::commit(RawSlice* iovecs, uint64_t num_iovecs) { } // In case an extra slice was reserved, remove empty slices from the end of the buffer. - while (!slices_.empty() && slices_.back()->dataSize() == 0) { + while (!slices_.empty() && slices_.back().dataSize() == 0) { slices_.pop_back(); } @@ -129,7 +129,7 @@ void OwnedImpl::copyOut(size_t start, uint64_t size, void* data) const { if (size == 0) { break; } - uint64_t data_size = slice->dataSize(); + uint64_t data_size = slice.dataSize(); if (data_size <= bytes_to_skip) { // The offset where the caller wants to start copying is after the end of this slice, // so just skip over this slice completely. @@ -137,7 +137,7 @@ void OwnedImpl::copyOut(size_t start, uint64_t size, void* data) const { continue; } uint64_t copy_size = std::min(size, data_size - bytes_to_skip); - memcpy(dest, slice->data() + bytes_to_skip, copy_size); + memcpy(dest, slice.data() + bytes_to_skip, copy_size); size -= copy_size; dest += copy_size; // Now that we've started copying, there are no bytes left to skip over. If there @@ -155,20 +155,20 @@ void OwnedImpl::drainImpl(uint64_t size) { if (slices_.empty()) { break; } - uint64_t slice_size = slices_.front()->dataSize(); + uint64_t slice_size = slices_.front().dataSize(); if (slice_size <= size) { slices_.pop_front(); length_ -= slice_size; size -= slice_size; } else { - slices_.front()->drain(size); + slices_.front().drain(size); length_ -= size; size = 0; } } // Make sure to drain any zero byte fragments that might have been added as // sentinels for flushed data. - while (!slices_.empty() && slices_.front()->dataSize() == 0) { + while (!slices_.empty() && slices_.front().dataSize() == 0) { slices_.pop_front(); } } @@ -186,7 +186,7 @@ RawSliceVector OwnedImpl::getRawSlices(absl::optional max_slices) cons break; } - if (slice->dataSize() == 0) { + if (slice.dataSize() == 0) { continue; } @@ -196,7 +196,8 @@ RawSliceVector OwnedImpl::getRawSlices(absl::optional max_slices) cons // there is currently no max size validation. // TODO(antoniovicente) Set realistic limits on the max size of BufferSlice and consider use of // size_t instead of uint64_t in the Slice interface. - raw_slices.emplace_back(RawSlice{slice->data(), static_cast(slice->dataSize())}); + raw_slices.emplace_back( + RawSlice{const_cast(slice.data()), static_cast(slice.dataSize())}); } return raw_slices; } @@ -204,8 +205,8 @@ RawSliceVector OwnedImpl::getRawSlices(absl::optional max_slices) cons RawSlice OwnedImpl::frontSlice() const { // Ignore zero-size slices and return the first slice with data. for (const auto& slice : slices_) { - if (slice->dataSize() > 0) { - return RawSlice{slice->data(), slice->dataSize()}; + if (slice.dataSize() > 0) { + return RawSlice{const_cast(slice.data()), slice.dataSize()}; } } @@ -216,27 +217,26 @@ SliceDataPtr OwnedImpl::extractMutableFrontSlice() { RELEASE_ASSERT(length_ > 0, "Extract called on empty buffer"); // Remove zero byte fragments from the front of the queue to ensure // that the extracted slice has data. - while (!slices_.empty() && slices_.front()->dataSize() == 0) { + while (!slices_.empty() && slices_.front().dataSize() == 0) { slices_.pop_front(); } ASSERT(!slices_.empty()); - ASSERT(slices_.front()); auto slice = std::move(slices_.front()); - auto size = slice->dataSize(); + auto size = slice.dataSize(); length_ -= size; slices_.pop_front(); - if (!slice->isMutable()) { + if (!slice.isMutable()) { // Create a mutable copy of the immutable slice data. - auto mutable_slice = OwnedSlice::create(size); - auto copy_size = mutable_slice->append(slice->data(), size); + Slice mutable_slice{size}; + auto copy_size = mutable_slice.append(slice.data(), size); ASSERT(copy_size == size); // Drain trackers for the immutable slice will be called as part of the slice destructor. - return mutable_slice; + return std::make_unique(std::move(mutable_slice)); } else { // Make sure drain trackers are called before ownership of the slice is transferred from // the buffer to the caller. - slice->callAndClearDrainTrackers(); - return slice; + slice.callAndClearDrainTrackers(); + return std::make_unique(std::move(slice)); } } @@ -246,7 +246,7 @@ uint64_t OwnedImpl::length() const { // of the lengths of the slices. uint64_t length = 0; for (const auto& slice : slices_) { - length += slice->dataSize(); + length += slice.dataSize(); } ASSERT(length == length_); #endif @@ -259,13 +259,13 @@ void* OwnedImpl::linearize(uint32_t size) { if (slices_.empty()) { return nullptr; } - if (slices_[0]->dataSize() < size) { - auto new_slice = OwnedSlice::create(size); - Slice::Reservation reservation = new_slice->reserve(size); + if (slices_[0].dataSize() < size) { + Slice new_slice{size}; + Slice::Reservation reservation = new_slice.reserve(size); ASSERT(reservation.mem_ != nullptr); ASSERT(reservation.len_ == size); copyOut(0, size, reservation.mem_); - new_slice->commit(reservation); + new_slice.commit(reservation); // Replace the first 'size' bytes in the buffer with the new slice. Since new_slice re-adds the // drained bytes, avoid use of the overridable 'drain' method to avoid incorrectly checking if @@ -274,23 +274,23 @@ void* OwnedImpl::linearize(uint32_t size) { slices_.emplace_front(std::move(new_slice)); length_ += size; } - return slices_.front()->data(); + return slices_.front().data(); } -void OwnedImpl::coalesceOrAddSlice(SlicePtr&& other_slice) { - const uint64_t slice_size = other_slice->dataSize(); +void OwnedImpl::coalesceOrAddSlice(Slice&& other_slice) { + const uint64_t slice_size = other_slice.dataSize(); // The `other_slice` content can be coalesced into the existing slice IFF: - // 1. The `other_slice` can be coalesced. Objects of type UnownedSlice can not be coalesced. See - // comment in the UnownedSlice class definition; + // 1. The `other_slice` can be coalesced. Immutable slices can not be safely coalesced because + // their destructors can be arbitrary global side effects. // 2. There are existing slices; // 3. The `other_slice` content length is under the CopyThreshold; // 4. There is enough unused space in the existing slice to accommodate the `other_slice` content. - if (other_slice->canCoalesce() && !slices_.empty() && slice_size < CopyThreshold && - slices_.back()->reservableSize() >= slice_size) { + if (other_slice.canCoalesce() && !slices_.empty() && slice_size < CopyThreshold && + slices_.back().reservableSize() >= slice_size) { // Copy content of the `other_slice`. The `move` methods which call this method effectively // drain the source buffer. - addImpl(other_slice->data(), slice_size); - other_slice->transferDrainTrackersTo(*slices_.back()); + addImpl(other_slice.data(), slice_size); + other_slice.transferDrainTrackersTo(slices_.back()); } else { // Take ownership of the slice. slices_.emplace_back(std::move(other_slice)); @@ -305,7 +305,7 @@ void OwnedImpl::move(Instance& rhs) { // want to maintain an abstraction. OwnedImpl& other = static_cast(rhs); while (!other.slices_.empty()) { - const uint64_t slice_size = other.slices_.front()->dataSize(); + const uint64_t slice_size = other.slices_.front().dataSize(); coalesceOrAddSlice(std::move(other.slices_.front())); other.length_ -= slice_size; other.slices_.pop_front(); @@ -318,15 +318,15 @@ void OwnedImpl::move(Instance& rhs, uint64_t length) { // See move() above for why we do the static cast. OwnedImpl& other = static_cast(rhs); while (length != 0 && !other.slices_.empty()) { - const uint64_t slice_size = other.slices_.front()->dataSize(); + const uint64_t slice_size = other.slices_.front().dataSize(); const uint64_t copy_size = std::min(slice_size, length); if (copy_size == 0) { other.slices_.pop_front(); } else if (copy_size < slice_size) { // TODO(brian-pane) add reference-counting to allow slices to share their storage // and eliminate the copy for this partial-slice case? - add(other.slices_.front()->data(), copy_size); - other.slices_.front()->drain(copy_size); + add(other.slices_.front().data(), copy_size); + other.slices_.front().drain(copy_size); other.length_ -= copy_size; } else { coalesceOrAddSlice(std::move(other.slices_.front())); @@ -345,11 +345,11 @@ uint64_t OwnedImpl::reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iove // 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) { + if (slices_[first_reservable_slice - 1].reservableSize() == 0) { break; } first_reservable_slice--; - if (slices_[first_reservable_slice]->dataSize() != 0) { + if (slices_[first_reservable_slice].dataSize() != 0) { // There is some content in this slice, so anything in front of it is non-reservable. break; } @@ -362,7 +362,7 @@ uint64_t OwnedImpl::reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iove 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); + 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, @@ -370,7 +370,7 @@ uint64_t OwnedImpl::reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iove // reservation. break; } - iovecs[num_slices_used] = slice->reserve(reservation_size); + iovecs[num_slices_used] = slice.reserve(reservation_size); bytes_remaining -= iovecs[num_slices_used].len_; num_slices_used++; slice_index++; @@ -378,8 +378,8 @@ uint64_t OwnedImpl::reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iove // If needed, allocate one more slice at the end to provide the remainder of the reservation. if (bytes_remaining != 0) { - slices_.emplace_back(OwnedSlice::create(bytes_remaining)); - iovecs[num_slices_used] = slices_.back()->reserve(bytes_remaining); + slices_.emplace_back(bytes_remaining); + iovecs[num_slices_used] = slices_.back().reserve(bytes_remaining); bytes_remaining -= iovecs[num_slices_used].len_; num_slices_used++; } @@ -409,13 +409,13 @@ ssize_t OwnedImpl::search(const void* data, uint64_t size, size_t start, size_t for (size_t slice_index = 0; slice_index < slices_.size() && (left_to_search > 0); slice_index++) { const auto& slice = slices_[slice_index]; - uint64_t slice_size = slice->dataSize(); + uint64_t slice_size = slice.dataSize(); if (slice_size <= start) { start -= slice_size; offset += slice_size; continue; } - const uint8_t* slice_start = slice->data(); + const uint8_t* slice_start = slice.data(); const uint8_t* haystack = slice_start; const uint8_t* haystack_end = haystack + slice_size; haystack += start; @@ -450,8 +450,8 @@ ssize_t OwnedImpl::search(const void* data, uint64_t size, size_t start, size_t break; } const auto& match_slice = slices_[match_index]; - match_next = match_slice->data(); - match_end = match_next + match_slice->dataSize(); + match_next = match_slice.data(); + match_end = match_next + match_slice.dataSize(); continue; } left_to_search--; @@ -487,8 +487,8 @@ bool OwnedImpl::startsWith(absl::string_view data) const { const uint8_t* prefix = reinterpret_cast(data.data()); size_t size = data.length(); for (const auto& slice : slices_) { - uint64_t slice_size = slice->dataSize(); - const uint8_t* slice_start = slice->data(); + uint64_t slice_size = slice.dataSize(); + const uint8_t* slice_start = slice.data(); if (slice_size >= size) { // The remaining size bytes of data are in this slice. @@ -530,7 +530,8 @@ std::string OwnedImpl::toString() const { void OwnedImpl::postProcess() {} void OwnedImpl::appendSliceForTest(const void* data, uint64_t size) { - slices_.emplace_back(OwnedSlice::create(data, size)); + slices_.emplace_back(size); + slices_.back().append(data, size); length_ += size; } @@ -538,10 +539,10 @@ void OwnedImpl::appendSliceForTest(absl::string_view data) { appendSliceForTest(data.data(), data.size()); } -std::vector OwnedImpl::describeSlicesForTest() const { - std::vector slices; +std::vector OwnedImpl::describeSlicesForTest() const { + std::vector slices; for (const auto& slice : slices_) { - slices.push_back(slice->describeSliceForTest()); + slices.push_back(slice.describeSliceForTest()); } return slices; } diff --git a/source/common/buffer/buffer_impl.h b/source/common/buffer/buffer_impl.h index be8018b22857c..6a3d852b1dedf 100644 --- a/source/common/buffer/buffer_impl.h +++ b/source/common/buffer/buffer_impl.h @@ -24,28 +24,86 @@ namespace Buffer { * | Unused space | Usable content | New content can be | * | that formerly | | added here with | * | was in the Data | | reserve()/commit() | - * | section | | | + * | section | | or append() | * +-----------------+----------------+----------------------+ - * ^ - * | - * data() + * ^ ^ ^ ^ + * | | | | + * base_ data() base_ + reservable_ base_ + capacity_ */ -class Slice : public SliceData { +class Slice { public: using Reservation = RawSlice; - ~Slice() override { callAndClearDrainTrackers(); } + /** + * Create an empty Slice with 0 capacity. + */ + Slice() = default; - // SliceData - absl::Span getMutableData() override { - RELEASE_ASSERT(isMutable(), "Not allowed to call getMutableData if slice is immutable"); - return {base_ + data_, static_cast::size_type>(reservable_ - data_)}; + /** + * Create an empty mutable Slice that owns its storage. + * @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_(new uint8_t[capacity_]), base_(storage_.get()), + data_(0), reservable_(0) {} + + /** + * Create an immutable Slice that refers to an external buffer fragment. + * @param fragment provides externally owned immutable data. + */ + Slice(BufferFragment& fragment) + : capacity_(fragment.size()), storage_(nullptr), + base_(static_cast(const_cast(fragment.data()))), data_(0), + reservable_(fragment.size()) { + addDrainTracker([&fragment]() { fragment.done(); }); + } + + Slice(Slice&& rhs) noexcept { + storage_ = std::move(rhs.storage_); + drain_trackers_ = std::move(rhs.drain_trackers_); + base_ = rhs.base_; + data_ = rhs.data_; + reservable_ = rhs.reservable_; + capacity_ = rhs.capacity_; + + rhs.base_ = nullptr; + rhs.data_ = 0; + rhs.reservable_ = 0; + rhs.capacity_ = 0; + } + + Slice& operator=(Slice&& rhs) noexcept { + if (this != &rhs) { + callAndClearDrainTrackers(); + + storage_ = std::move(rhs.storage_); + drain_trackers_ = std::move(rhs.drain_trackers_); + base_ = rhs.base_; + data_ = rhs.data_; + reservable_ = rhs.reservable_; + capacity_ = rhs.capacity_; + + rhs.base_ = nullptr; + rhs.data_ = 0; + rhs.reservable_ = 0; + rhs.capacity_ = 0; + } + + return *this; } + ~Slice() { callAndClearDrainTrackers(); } + /** * @return true if the data in the slice is mutable */ - virtual bool isMutable() const { return false; } + bool isMutable() const { return storage_ != nullptr; } + + /** + * @return true if content in this Slice can be coalesced into another Slice. + */ + bool canCoalesce() const { return storage_ != nullptr; } /** * @return a pointer to the start of the usable content. @@ -132,7 +190,7 @@ class Slice : public SliceData { bool commit(const Reservation& reservation) { if (static_cast(reservation.mem_) != base_ + reservable_ || reservable_ + reservation.len_ > capacity_ || reservable_ >= capacity_) { - // The reservation is not from this OwnedSlice. + // The reservation is not from this Slice. return false; } reservable_ += reservation.len_; @@ -187,11 +245,6 @@ class Slice : public SliceData { return copy_size; } - /** - * @return true if content in this Slice can be coalesced into another Slice. - */ - virtual bool canCoalesce() const { return true; } - /** * Describe the in-memory representation of the slice. For use * in tests that want to make assertions about the specific arrangement of @@ -233,74 +286,55 @@ class Slice : public SliceData { } protected: - Slice(uint64_t data, uint64_t reservable, uint64_t capacity) - : data_(data), reservable_(reservable), capacity_(capacity) {} + /** + * Compute a slice size big enough to hold a specified amount of data. + * @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 = (data_size + PageSize - 1) / PageSize; + return num_pages * PageSize; + } + + /** 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. */ + uint64_t capacity_; + + /** Backing storage for mutable slices which own their own storage. This storage should never be + * accessed directly; access base_ instead. */ + std::unique_ptr storage_; - /** Start of the slice - subclasses must set this */ + /** Start of the slice. Points to storage_ iff the slice owns its own storage. */ uint8_t* base_{nullptr}; - /** Offset in bytes from the start of the slice to the start of the Data section */ + /** Offset in bytes from the start of the slice to the start of the Data section. */ uint64_t data_; - /** Offset in bytes from the start of the slice to the start of the Reservable section */ + /** Offset in bytes from the start of the slice to the start of the Reservable section which is + * also the end of the Data section. */ uint64_t reservable_; - /** Total number of bytes in the slice */ - uint64_t capacity_; - + /** Hooks to execute when the slice is destroyed. */ std::list> drain_trackers_; }; -using SlicePtr = std::unique_ptr; - -class OwnedSlice final : public Slice { +class SliceDataImpl : public SliceData { public: - /** - * Create an empty OwnedSlice. - * @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) { - uint64_t slice_capacity = sliceSize(capacity); - return SlicePtr{new OwnedSlice(slice_capacity)}; - } + explicit SliceDataImpl(Slice&& slice) : slice_(std::move(slice)) {} - /** - * Create an OwnedSlice and initialize it with a copy of the supplied copy. - * @param data the content to copy into the slice. - * @param size length of the content. - * @return an OwnedSlice containing a copy of the content, which may (dependent on - * the internal implementation) have a nonzero amount of reservable space at the end. - */ - static SlicePtr create(const void* data, uint64_t size) { - auto slice = create(size); - slice->append(data, size); - return slice; + // SliceData + absl::Span getMutableData() override { + RELEASE_ASSERT(slice_.isMutable(), "Not allowed to call getMutableData if slice is immutable"); + return {slice_.data(), slice_.dataSize()}; } private: - OwnedSlice(uint64_t size) : Slice(0, 0, size), storage_(new uint8_t[size]) { - base_ = storage_.get(); - } - - bool isMutable() const override { return true; } - - /** - * Compute a slice size big enough to hold a specified amount of data. - * @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 = (data_size + PageSize - 1) / PageSize; - return num_pages * PageSize; - } - - std::unique_ptr storage_; + Slice slice_; }; /** - * Queue of SlicePtr that supports efficient read and write access to both + * Queue of Slice that supports efficient read and write access to both * the front and the back of the queue. * @note This class has similar properties to std::deque. The reason for using * a custom deque implementation is that benchmark testing during development @@ -332,14 +366,14 @@ class SliceDeque { return *this; } - void emplace_back(SlicePtr&& slice) { + void emplace_back(Slice&& slice) { // NOLINT(readability-identifier-naming) growRing(); size_t index = internalIndex(size_); ring_[index] = std::move(slice); size_++; } - void emplace_front(SlicePtr&& slice) { + void emplace_front(Slice&& slice) { // NOLINT(readability-identifier-naming) growRing(); start_ = (start_ == 0) ? capacity_ - 1 : start_ - 1; ring_[start_] = std::move(slice); @@ -349,19 +383,25 @@ class SliceDeque { bool empty() const { return size() == 0; } size_t size() const { return size_; } - SlicePtr& front() { return ring_[start_]; } - const SlicePtr& front() const { return ring_[start_]; } - SlicePtr& back() { return ring_[internalIndex(size_ - 1)]; } - const SlicePtr& back() const { return ring_[internalIndex(size_ - 1)]; } + Slice& front() { return ring_[start_]; } + const Slice& front() const { return ring_[start_]; } + Slice& back() { return ring_[internalIndex(size_ - 1)]; } + const Slice& back() const { return ring_[internalIndex(size_ - 1)]; } - SlicePtr& operator[](size_t i) { return ring_[internalIndex(i)]; } - const SlicePtr& operator[](size_t i) const { return ring_[internalIndex(i)]; } + Slice& operator[](size_t i) { + ASSERT(!empty()); + return ring_[internalIndex(i)]; + } + const Slice& operator[](size_t i) const { + ASSERT(!empty()); + return ring_[internalIndex(i)]; + } - void pop_front() { + void pop_front() { // NOLINT(readability-identifier-naming) if (size() == 0) { return; } - front() = SlicePtr(); + front() = Slice(); size_--; start_++; if (start_ == capacity_) { @@ -369,11 +409,11 @@ class SliceDeque { } } - void pop_back() { + void pop_back() { // NOLINT(readability-identifier-naming) if (size() == 0) { return; } - back() = SlicePtr(); + back() = Slice(); size_--; } @@ -384,7 +424,7 @@ class SliceDeque { */ class ConstIterator { public: - const SlicePtr& operator*() { return deque_[index_]; } + const Slice& operator*() { return deque_[index_]; } ConstIterator operator++() { index_++; @@ -424,10 +464,7 @@ class SliceDeque { return; } const size_t new_capacity = capacity_ * 2; - auto new_ring = std::make_unique(new_capacity); - for (size_t i = 0; i < new_capacity; i++) { - ASSERT(new_ring[i] == nullptr); - } + auto new_ring = std::make_unique(new_capacity); size_t src = start_; size_t dst = 0; for (size_t i = 0; i < size_; i++) { @@ -436,43 +473,20 @@ class SliceDeque { src = 0; } } - for (size_t i = 0; i < capacity_; i++) { - ASSERT(ring_[i].get() == nullptr); - } external_ring_.swap(new_ring); ring_ = external_ring_.get(); start_ = 0; capacity_ = new_capacity; } - SlicePtr inline_ring_[InlineRingCapacity]; - std::unique_ptr external_ring_; - SlicePtr* ring_; // points to start of either inline or external ring. + Slice inline_ring_[InlineRingCapacity]; + std::unique_ptr external_ring_; + Slice* ring_; // points to start of either inline or external ring. size_t start_{0}; size_t size_{0}; size_t capacity_; }; -class UnownedSlice : public Slice { -public: - UnownedSlice(BufferFragment& fragment) - : Slice(0, fragment.size(), fragment.size()), fragment_(fragment) { - base_ = static_cast(const_cast(fragment.data())); - } - - ~UnownedSlice() override { fragment_.done(); } - - /** - * BufferFragment objects encapsulated by UnownedSlice are used to track when response content - * is written into transport connection. As a result these slices can not be coalesced when moved - * between buffers. - */ - bool canCoalesce() const override { return false; } - -private: - BufferFragment& fragment_; -}; - /** * An implementation of BufferFragment where a releasor callback is called when the data is * no longer needed. @@ -595,7 +609,7 @@ class OwnedImpl : public LibEventInstance { * in tests that want to make assertions about the specific arrangement of * bytes in the buffer. */ - std::vector describeSlicesForTest() const; + std::vector describeSlicesForTest() const; private: /** @@ -613,7 +627,7 @@ class OwnedImpl : public LibEventInstance { * into an existing slice. * NOTE: the caller is responsible for draining the buffer that contains the `other_slice`. */ - void coalesceOrAddSlice(SlicePtr&& other_slice); + void coalesceOrAddSlice(Slice&& other_slice); /** Ring buffer of slices. */ SliceDeque slices_; diff --git a/test/common/buffer/buffer_test.cc b/test/common/buffer/buffer_test.cc index 845ee78caa28f..28027fcab5b13 100644 --- a/test/common/buffer/buffer_test.cc +++ b/test/common/buffer/buffer_test.cc @@ -14,22 +14,119 @@ namespace Envoy { namespace Buffer { namespace { -class DummySlice : public Slice { +class SliceTest : public testing::TestWithParam { public: - DummySlice(const std::string& data, const std::function& deletion_callback) - : Slice(0, data.size(), data.size()), deletion_callback_(deletion_callback) { - base_ = reinterpret_cast(const_cast(data.c_str())); - } - ~DummySlice() override { - if (deletion_callback_ != nullptr) { - deletion_callback_(); + bool shouldCreateUnownedSlice() const { return GetParam(); } + std::unique_ptr createSlice(absl::string_view data) { + if (shouldCreateUnownedSlice()) { + auto fragment = new BufferFragmentImpl( + data.data(), data.size(), + [](const void*, size_t, const BufferFragmentImpl* fragment) { delete fragment; }); + auto slice = std::make_unique(*fragment); + return slice; + } else { + auto slice = std::make_unique(data.size()); + slice->append(data.data(), data.size()); + return slice; } } - -private: - const std::function deletion_callback_; }; +INSTANTIATE_TEST_SUITE_P(SliceType, SliceTest, testing::Bool(), + [](const testing::TestParamInfo& param) { + return param.param ? "Unowned" : "Owned"; + }); + +TEST_P(SliceTest, MoveConstruction) { + constexpr char input[] = "hello world"; + + auto slice1 = createSlice(input); + bool drain_tracker_called = false; + slice1->addDrainTracker([&drain_tracker_called]() { drain_tracker_called = true; }); + slice1->drain(1); + EXPECT_EQ(10, slice1->dataSize()); + if (shouldCreateUnownedSlice()) { + EXPECT_EQ(0, slice1->reservableSize()); + } else { + EXPECT_EQ(4085, slice1->reservableSize()); + } + EXPECT_EQ(0, memcmp(slice1->data(), input + 1, slice1->dataSize())); + EXPECT_FALSE(drain_tracker_called); + + auto slice2 = std::make_unique(std::move(*slice1)); + // slice1 is cleared as part of the move. + EXPECT_EQ(0, slice1->dataSize()); + EXPECT_EQ(0, slice1->reservableSize()); + EXPECT_EQ(nullptr, slice1->data()); + EXPECT_FALSE(drain_tracker_called); + slice1.reset(); + + EXPECT_EQ(10, slice2->dataSize()); + if (shouldCreateUnownedSlice()) { + EXPECT_EQ(0, slice2->reservableSize()); + } else { + EXPECT_EQ(4085, slice2->reservableSize()); + } + EXPECT_EQ(0, memcmp(slice2->data(), input + 1, slice2->dataSize())); + EXPECT_FALSE(drain_tracker_called); + slice2.reset(nullptr); + EXPECT_TRUE(drain_tracker_called); +} + +TEST_P(SliceTest, MoveAssigment) { + constexpr char input1[] = "hello"; + auto slice1 = createSlice(input1); + bool drain_tracker_called1 = false; + slice1->addDrainTracker([&drain_tracker_called1]() { drain_tracker_called1 = true; }); + slice1->drain(1); + EXPECT_EQ(4, slice1->dataSize()); + if (shouldCreateUnownedSlice()) { + EXPECT_EQ(0, slice1->reservableSize()); + } else { + EXPECT_EQ(4091, slice1->reservableSize()); + } + EXPECT_EQ(0, memcmp(slice1->data(), input1 + 1, slice1->dataSize())); + EXPECT_FALSE(drain_tracker_called1); + + constexpr char input2[] = "how low"; + auto slice2 = createSlice(input2); + bool drain_tracker_called2 = false; + slice2->addDrainTracker([&drain_tracker_called2]() { drain_tracker_called2 = true; }); + slice2->drain(2); + EXPECT_EQ(5, slice2->dataSize()); + if (shouldCreateUnownedSlice()) { + EXPECT_EQ(0, slice2->reservableSize()); + } else { + EXPECT_EQ(4089, slice2->reservableSize()); + } + EXPECT_EQ(0, memcmp(slice2->data(), input2 + 2, slice2->dataSize())); + EXPECT_FALSE(drain_tracker_called2); + + *slice2 = std::move(*slice1); + // The contents of the second slice are overwritten and are no more. Release callback is invoked. + EXPECT_FALSE(drain_tracker_called1); + EXPECT_TRUE(drain_tracker_called2); + + // slice1 is cleared as part of the move. + EXPECT_EQ(0, slice1->dataSize()); + EXPECT_EQ(0, slice1->reservableSize()); + EXPECT_EQ(nullptr, slice1->data()); + EXPECT_FALSE(drain_tracker_called1); + slice1.reset(); + + // The original contents of slice1 are now in slice2. + EXPECT_EQ(4, slice2->dataSize()); + if (shouldCreateUnownedSlice()) { + EXPECT_EQ(0, slice2->reservableSize()); + } else { + EXPECT_EQ(4091, slice2->reservableSize()); + } + EXPECT_EQ(0, memcmp(slice2->data(), input1 + 1, slice2->dataSize())); + EXPECT_FALSE(drain_tracker_called1); + slice2.reset(); + EXPECT_TRUE(drain_tracker_called1); +} + class OwnedSliceTest : public testing::Test { protected: static void expectReservationSuccess(const Slice::Reservation& reservation, const Slice& slice, @@ -54,143 +151,143 @@ class OwnedSliceTest : public testing::Test { } }; -bool sliceMatches(const SlicePtr& slice, const std::string& expected) { - return slice != nullptr && slice->dataSize() == expected.size() && - memcmp(slice->data(), expected.data(), expected.size()) == 0; +bool sliceMatches(const Slice& slice, const std::string& expected) { + return slice.dataSize() == expected.size() && + memcmp(slice.data(), expected.data(), expected.size()) == 0; } TEST_F(OwnedSliceTest, Create) { static constexpr std::pair Sizes[] = { {0, 0}, {1, 4096}, {64, 4096}, {4095, 4096}, {4096, 4096}, {4097, 8192}, {65535, 65536}}; for (const auto& [size, expected_size] : Sizes) { - auto slice = OwnedSlice::create(size); - EXPECT_NE(nullptr, slice->data()); - EXPECT_EQ(0, slice->dataSize()); - EXPECT_LE(size, slice->reservableSize()); - EXPECT_EQ(expected_size, slice->reservableSize()); + Slice slice{size}; + EXPECT_NE(nullptr, slice.data()); + EXPECT_EQ(0, slice.dataSize()); + EXPECT_LE(size, slice.reservableSize()); + EXPECT_EQ(expected_size, slice.reservableSize()); } } TEST_F(OwnedSliceTest, ReserveCommit) { - auto slice = OwnedSlice::create(100); - const uint64_t initial_capacity = slice->reservableSize(); + Slice slice{100}; + const uint64_t initial_capacity = slice.reservableSize(); EXPECT_LE(100, initial_capacity); { // Verify that a zero-byte reservation is rejected. - Slice::Reservation reservation = slice->reserve(0); - expectReservationFailure(reservation, *slice, initial_capacity); + Slice::Reservation reservation = slice.reserve(0); + expectReservationFailure(reservation, slice, initial_capacity); } { // Create a reservation smaller than the reservable size. // It should reserve the exact number of bytes requested. - Slice::Reservation reservation = slice->reserve(10); - expectReservationSuccess(reservation, *slice, 10); + Slice::Reservation reservation = slice.reserve(10); + expectReservationSuccess(reservation, slice, 10); // Request a second reservation while the first reservation remains uncommitted. // This should succeed. - EXPECT_EQ(initial_capacity, slice->reservableSize()); - Slice::Reservation reservation2 = slice->reserve(1); - expectReservationSuccess(reservation2, *slice, 1); + EXPECT_EQ(initial_capacity, slice.reservableSize()); + Slice::Reservation reservation2 = slice.reserve(1); + expectReservationSuccess(reservation2, slice, 1); // Commit the entire reserved size. - bool committed = slice->commit(reservation); - expectCommitSuccess(committed, *slice, 10, initial_capacity - 10); + bool committed = slice.commit(reservation); + expectCommitSuccess(committed, slice, 10, initial_capacity - 10); // Verify that a reservation can only be committed once. - EXPECT_FALSE(slice->commit(reservation)); + EXPECT_FALSE(slice.commit(reservation)); } { // Request another reservation, and commit only part of it. - Slice::Reservation reservation = slice->reserve(10); - expectReservationSuccess(reservation, *slice, 10); + Slice::Reservation reservation = slice.reserve(10); + expectReservationSuccess(reservation, slice, 10); reservation.len_ = 5; - bool committed = slice->commit(reservation); - expectCommitSuccess(committed, *slice, 15, initial_capacity - 15); + bool committed = slice.commit(reservation); + expectCommitSuccess(committed, slice, 15, initial_capacity - 15); } { // Request another reservation, and commit only part of it. - Slice::Reservation reservation = slice->reserve(10); - expectReservationSuccess(reservation, *slice, 10); + Slice::Reservation reservation = slice.reserve(10); + expectReservationSuccess(reservation, slice, 10); reservation.len_ = 5; - bool committed = slice->commit(reservation); - expectCommitSuccess(committed, *slice, 20, initial_capacity - 20); + bool committed = slice.commit(reservation); + expectCommitSuccess(committed, slice, 20, initial_capacity - 20); } { // Request another reservation, and commit zero bytes of it. // This should clear the reservation. - Slice::Reservation reservation = slice->reserve(10); - expectReservationSuccess(reservation, *slice, 10); + Slice::Reservation reservation = slice.reserve(10); + expectReservationSuccess(reservation, slice, 10); reservation.len_ = 0; - bool committed = slice->commit(reservation); - expectCommitSuccess(committed, *slice, 20, initial_capacity - 20); + bool committed = slice.commit(reservation); + expectCommitSuccess(committed, slice, 20, initial_capacity - 20); } { // Try to commit a reservation from the wrong slice, and verify that the slice rejects it. - Slice::Reservation reservation = slice->reserve(10); - expectReservationSuccess(reservation, *slice, 10); - auto other_slice = OwnedSlice::create(100); - Slice::Reservation other_reservation = other_slice->reserve(10); - expectReservationSuccess(other_reservation, *other_slice, 10); - EXPECT_FALSE(slice->commit(other_reservation)); - EXPECT_FALSE(other_slice->commit(reservation)); + Slice::Reservation reservation = slice.reserve(10); + expectReservationSuccess(reservation, slice, 10); + Slice other_slice{100}; + Slice::Reservation other_reservation = other_slice.reserve(10); + expectReservationSuccess(other_reservation, other_slice, 10); + EXPECT_FALSE(slice.commit(other_reservation)); + EXPECT_FALSE(other_slice.commit(reservation)); // Commit the reservations to the proper slices to clear them. reservation.len_ = 0; - bool committed = slice->commit(reservation); + bool committed = slice.commit(reservation); EXPECT_TRUE(committed); other_reservation.len_ = 0; - committed = other_slice->commit(other_reservation); + committed = other_slice.commit(other_reservation); EXPECT_TRUE(committed); } { // Try to reserve more space than is available in the slice. - uint64_t reservable_size = slice->reservableSize(); - Slice::Reservation reservation = slice->reserve(reservable_size + 1); - expectReservationSuccess(reservation, *slice, reservable_size); - bool committed = slice->commit(reservation); - expectCommitSuccess(committed, *slice, initial_capacity, 0); + uint64_t reservable_size = slice.reservableSize(); + Slice::Reservation reservation = slice.reserve(reservable_size + 1); + expectReservationSuccess(reservation, slice, reservable_size); + bool committed = slice.commit(reservation); + expectCommitSuccess(committed, slice, initial_capacity, 0); } { // Now that the view has no more reservable space, verify that it rejects // subsequent reservation requests. - Slice::Reservation reservation = slice->reserve(1); - expectReservationFailure(reservation, *slice, 0); + Slice::Reservation reservation = slice.reserve(1); + expectReservationFailure(reservation, slice, 0); } } TEST_F(OwnedSliceTest, Drain) { // Create a slice and commit all the available space. - auto slice = OwnedSlice::create(100); - Slice::Reservation reservation = slice->reserve(slice->reservableSize()); - bool committed = slice->commit(reservation); + Slice slice{100}; + Slice::Reservation reservation = slice.reserve(slice.reservableSize()); + bool committed = slice.commit(reservation); EXPECT_TRUE(committed); - EXPECT_EQ(0, slice->reservableSize()); + EXPECT_EQ(0, slice.reservableSize()); // Drain some data from the front of the view and verify that the data start moves accordingly. - const uint8_t* original_data = static_cast(slice->data()); - uint64_t original_size = slice->dataSize(); - slice->drain(0); - EXPECT_EQ(original_data, slice->data()); - EXPECT_EQ(original_size, slice->dataSize()); - slice->drain(10); - EXPECT_EQ(original_data + 10, slice->data()); - EXPECT_EQ(original_size - 10, slice->dataSize()); - slice->drain(50); - EXPECT_EQ(original_data + 60, slice->data()); - EXPECT_EQ(original_size - 60, slice->dataSize()); + const uint8_t* original_data = static_cast(slice.data()); + uint64_t original_size = slice.dataSize(); + slice.drain(0); + EXPECT_EQ(original_data, slice.data()); + EXPECT_EQ(original_size, slice.dataSize()); + slice.drain(10); + EXPECT_EQ(original_data + 10, slice.data()); + EXPECT_EQ(original_size - 10, slice.dataSize()); + slice.drain(50); + EXPECT_EQ(original_data + 60, slice.data()); + EXPECT_EQ(original_size - 60, slice.dataSize()); // Drain all the remaining data. - slice->drain(slice->dataSize()); - EXPECT_EQ(0, slice->dataSize()); - EXPECT_EQ(original_size, slice->reservableSize()); + slice.drain(slice.dataSize()); + EXPECT_EQ(0, slice.dataSize()); + EXPECT_EQ(original_size, slice.reservableSize()); } TEST(UnownedSliceTest, CreateDelete) { @@ -201,7 +298,7 @@ TEST(UnownedSliceTest, CreateDelete) { [&release_callback_called](const void*, size_t, const BufferFragmentImpl*) { release_callback_called = true; }); - auto slice = std::make_unique(fragment); + auto slice = std::make_unique(fragment); EXPECT_EQ(11, slice->dataSize()); EXPECT_EQ(0, slice->reservableSize()); EXPECT_EQ(0, memcmp(slice->data(), input, slice->dataSize())); @@ -217,7 +314,7 @@ TEST(UnownedSliceTest, CreateDeleteOwnedBufferFragment) { {input, sizeof(input) - 1}, [&release_callback_called](const OwnedBufferFragmentImpl*) { release_callback_called = true; }); - auto slice = std::make_unique(*fragment); + auto slice = std::make_unique(*fragment); EXPECT_EQ(11, slice->dataSize()); EXPECT_EQ(0, slice->reservableSize()); EXPECT_EQ(0, memcmp(slice->data(), input, slice->dataSize())); @@ -240,7 +337,11 @@ TEST(SliceDequeTest, CreateDelete) { // Append a view to the deque. const std::string slice1 = "slice1"; slices.emplace_back( - std::make_unique(slice1, [&slice1_deleted]() { slice1_deleted = true; })); + *OwnedBufferFragmentImpl::create(slice1, [&slice1_deleted]( + const OwnedBufferFragmentImpl* fragment) { + slice1_deleted = true; + delete fragment; + }).release()); EXPECT_FALSE(slices.empty()); ASSERT_EQ(1, slices.size()); EXPECT_FALSE(slice1_deleted); @@ -249,7 +350,11 @@ TEST(SliceDequeTest, CreateDelete) { // Append another view to the deque, and verify that both views are accessible. const std::string slice2 = "slice2"; slices.emplace_back( - std::make_unique(slice2, [&slice2_deleted]() { slice2_deleted = true; })); + *OwnedBufferFragmentImpl::create(slice2, [&slice2_deleted]( + const OwnedBufferFragmentImpl* fragment) { + slice2_deleted = true; + delete fragment; + }).release()); EXPECT_FALSE(slices.empty()); ASSERT_EQ(2, slices.size()); EXPECT_FALSE(slice1_deleted); @@ -260,7 +365,11 @@ TEST(SliceDequeTest, CreateDelete) { // Prepend a view to the deque, to exercise the ring buffer wraparound case. const std::string slice3 = "slice3"; slices.emplace_front( - std::make_unique(slice3, [&slice3_deleted]() { slice3_deleted = true; })); + *OwnedBufferFragmentImpl::create(slice3, [&slice3_deleted]( + const OwnedBufferFragmentImpl* fragment) { + slice3_deleted = true; + delete fragment; + }).release()); EXPECT_FALSE(slices.empty()); ASSERT_EQ(3, slices.size()); EXPECT_FALSE(slice1_deleted);