Merge pull request #25053 from rockwotj/iobuf_move

src/v/base/vassert.h

@@ -50,3 +50,15 @@ inline dummyassert g_assert_log;
             __builtin_trap();                                                  \
         }                                                                      \
     } while (0)
+
+/**
+ * same as vassert but only debug mode. Use over assert for better
+ * error messages.
+ */
+#ifndef NDEBUG
+// NOLINTNEXTLINE(cppcoreguidelines-macro-usage)
+#define dassert(x, msg, args...) vassert(x, msg, ##args)
+#else
+// NOLINTNEXTLINE(cppcoreguidelines-macro-usage)
+#define dassert(...)
+#endif

src/v/bytes/BUILD

@@ -18,7 +18,6 @@ redpanda_cc_library(
     visibility = ["//visibility:public"],
     deps = [
         "//src/v/base",
-        "//src/v/container:intrusive",
         "@fmt",
         "@seastar",
     ],

src/v/bytes/details/io_byte_iterator.h

@@ -19,9 +19,7 @@
 namespace details {
 class io_byte_iterator {
 public:
-    using io_const_iterator
-      = uncounted_intrusive_list<io_fragment, &io_fragment::hook>::
-        const_iterator;
+    using io_const_iterator = io_fragment_list::const_iterator;
 
     // iterator_traits
     using difference_type = void;

src/v/bytes/details/io_fragment.h

@@ -12,12 +12,16 @@
 #pragma once
 
 #include "base/seastarx.h"
+#include "base/vassert.h"
 #include "bytes/details/out_of_range.h"
-#include "container/intrusive_list_helpers.h"
 
 #include <seastar/core/temporary_buffer.hh>
 
+#include <iterator>
+#include <utility>
+
 namespace details {
+
 class io_fragment {
 public:
     /**
@@ -38,7 +42,11 @@ class io_fragment {
     io_fragment& operator=(io_fragment&& o) noexcept = delete;
     io_fragment(const io_fragment& o) = delete;
     io_fragment& operator=(const io_fragment& o) = delete;
-    ~io_fragment() noexcept = default;
+    ~io_fragment() noexcept {
+        dassert(
+          !_next && !_prev,
+          "io_fragment cleanup failure and possible memory leak");
+    }
 
     bool is_empty() const { return _used_bytes == 0; }
     size_t available_bytes() const { return _buf.size() - _used_bytes; }
@@ -94,14 +102,269 @@ class io_fragment {
     char* get_current() { return _buf.get_write() + _used_bytes; }
     char* get_write() { return _buf.get_write(); }
 
-    // NOLINTNEXTLINE(cppcoreguidelines-non-private-member-variables-in-classes,misc-non-private-member-variables-in-classes)
-    safe_intrusive_list_hook hook;
-
 private:
+    friend class io_fragment_list;
+
+    io_fragment* _next = nullptr;
+    io_fragment* _prev = nullptr;
+
     ss::temporary_buffer<char> _buf;
     size_t _used_bytes;
 };
 
+/**
+ * A double-ly linked (intrusive) list of `io_fragment`.
+ *
+ * We use a hand rolled list instead of boost::intrusive_list to optimize
+ * certain codepaths, namely the move constructor was about 4x faster in the
+ * hand rolled implementation over boost.
+ *
+ * We're also able to enhance the list with features like iterator invalidation
+ * checking and the like.
+ *
+ * Note that this container does not own the memory for the io_fragments it
+ * contains, iobuf is responsible for that.
+ */
+class io_fragment_list {
+    template<bool Const, bool Forward>
+    class iter {
+    public:
+        using iterator_category = std::forward_iterator_tag;
+        using container_type = typename std::
+          conditional_t<Const, const io_fragment_list, io_fragment_list>;
+        using value_type =
+          typename std::conditional_t<Const, const io_fragment, io_fragment>;
+        using difference_type = std::ptrdiff_t;
+        using pointer = value_type*;
+        using reference = value_type&;
+
+    private:
+        iter([[maybe_unused]] container_type* container, pointer curr)
+          : _current(curr)
+#ifndef NDEBUG
+          , _my_generation(container->_generation)
+          , _container(container)
+#endif
+        {
+        }
+
+    public:
+        iter() noexcept = default;
+        // Support conversion to const_iterator
+        // NOLINTNEXTLINE(hicpp-explicit-conversions)
+        operator iter<true, Forward>() const {
+            check_generation();
+#ifndef NDEBUG
+            return {_container, _current};
+#else
+            return {nullptr, _current};
+#endif
+        }
+
+        reference operator*() const {
+            check_generation();
+            return *_current;
+        }
+        pointer operator->() const {
+            check_generation();
+            return _current;
+        }
+        iter& operator++() {
+            check_generation();
+            if constexpr (Forward) {
+                _current = _current->_next;
+            } else {
+                _current = _current->_prev;
+            }
+            return *this;
+        }
+        iter operator++(int) {
+            auto tmp = *this;
+            ++*this;
+            return tmp;
+        }
+        bool operator==(const iter& o) const {
+            check_generation();
+            return _current == o._current;
+        }
+
+    private:
+        friend class io_fragment_list;
+
+        inline void check_generation() const {
+            dassert(
+              _container->_generation == _my_generation,
+              "Attempting to use an invalidated iterator. The corresponding "
+              "iobuf has been mutated since this iterator was constructed.");
+        }
+
+        pointer _current = nullptr;
+#ifndef NDEBUG
+        size_t _my_generation = 0;
+        container_type* _container = nullptr;
+#endif
+    };
+
+public:
+    io_fragment_list() noexcept = default;
+    io_fragment_list(io_fragment_list&& o) noexcept
+      : _head(std::exchange(o._head, nullptr))
+      , _tail(std::exchange(o._tail, nullptr)) {
+        o.update_generation();
+    }
+    io_fragment_list(const io_fragment_list&) = delete;
+    io_fragment_list& operator=(io_fragment_list&&) = delete;
+    io_fragment_list& operator=(const io_fragment_list&) = delete;
+    ~io_fragment_list() noexcept {
+        dassert(
+          _head == nullptr && _tail == nullptr,
+          "io_fragment_list cleanup failure and possible memory leak");
+    };
+
+    using iterator = iter<false, true>;
+    using reverse_iterator = iter<false, false>;
+    using const_iterator = iter<true, true>;
+
+    bool empty() const {
+        check_consistency();
+        return _head == nullptr;
+    }
+
+    io_fragment& front() {
+        check_consistency();
+        return *_head;
+    }
+
+    void pop_front() {
+        check_consistency();
+        io_fragment* h = _head;
+        _head = h->_next;
+        h->_next = nullptr;
+        if (_head != nullptr) {
+            _head->_prev = nullptr;
+        } else {
+            _tail = nullptr;
+        }
+        update_generation();
+    }
+
+    void push_front(io_fragment& elem) {
+        check_consistency();
+        if (_head == nullptr) {
+            _tail = &elem;
+        } else {
+            _head->_prev = &elem;
+        }
+        elem._next = _head;
+        _head = &elem;
+        update_generation();
+    }
+
+    io_fragment& back() {
+        check_consistency();
+        return *_tail;
+    }
+    const io_fragment& back() const {
+        check_consistency();
+        return *_tail;
+    }
+
+    void pop_back() {
+        check_consistency();
+        io_fragment* t = _tail;
+        _tail = _tail->_prev;
+        t->_prev = nullptr;
+        if (_tail != nullptr) {
+            _tail->_next = nullptr;
+        } else {
+            _head = nullptr;
+        }
+        update_generation();
+    }
+
+    void push_back(io_fragment& elem) {
+        dassert(
+          elem._prev == nullptr, "pushing back io_fragment in another iobuf");
+        dassert(
+          elem._next == nullptr, "pushing back io_fragment in another iobuf");
+        check_consistency();
+        if (_tail == nullptr) {
+            _head = &elem;
+        } else {
+            _tail->_next = &elem;
+        }
+        elem._prev = _tail;
+        _tail = &elem;
+        update_generation();
+    }
+
+    template<typename Func>
+    void clear_and_dispose(Func func) {
+        check_consistency();
+        io_fragment* current = _head;
+        while (current != nullptr) {
+            io_fragment* next = current->_next;
+            if (next != nullptr) {
+                dassert(
+                  next->_prev == current, "io_fragment_list inconsistency");
+                next->_prev = nullptr;
+            }
+            current->_next = nullptr;
+            func(current);
+            current = next;
+        }
+        _head = nullptr;
+        _tail = nullptr;
+        update_generation();
+    }
+
+    iterator begin() { return {this, _head}; }
+    iterator end() { return {this, nullptr}; }
+    reverse_iterator rbegin() { return {this, _tail}; }
+    reverse_iterator rend() { return {this, nullptr}; }
+    const_iterator cbegin() const { return {this, _head}; }
+    const_iterator cend() const { return {this, nullptr}; }
+
+private:
+    friend class iter<true, true>;
+    friend class iter<false, true>;
+    friend class iter<false, false>;
+    inline void update_generation() {
+#ifndef NDEBUG
+        ++_generation;
+#endif
+    }
+
+    inline void check_consistency() const {
+        // We don't check full consistency, just the head and tail elements, so
+        // we don't make debug mode *that* much slower.
+#ifndef NDEBUG
+        dassert(
+          (_head == nullptr) == (_tail == nullptr),
+          "head and tail consistency");
+        if (_head == nullptr) {
+            return;
+        }
+        dassert(!_head->_prev, "head must not have prev");
+        dassert(!_tail->_next, "tail must not have next");
+        dassert(
+          _head == _tail || _head->_next, "head must have next if size > 1");
+        dassert(
+          _head == _tail || _tail->_prev, "tail must have prev if size > 1");
+        dassert(
+          (_head->_next == _tail) == (_tail->_prev == _head),
+          "If head points to tail, then tail should point to head (size == 2 "
+          "consistency).");
+#endif
+    }
+
+    io_fragment* _head = nullptr;
+    io_fragment* _tail = nullptr;
+#ifndef NDEBUG
+    size_t _generation = 0;
+#endif
+};
+
 inline void __attribute__((noinline)) dispose_io_fragment(io_fragment* f) {
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Wfree-nonheap-object"

src/v/bytes/details/io_iterator_consumer.h

@@ -46,9 +46,7 @@
 namespace details {
 class io_iterator_consumer {
 public:
-    using io_const_iterator
-      = uncounted_intrusive_list<io_fragment, &io_fragment::hook>::
-        const_iterator;
+    using io_const_iterator = io_fragment_list::const_iterator;
 
     io_iterator_consumer(
       const io_const_iterator& begin, const io_const_iterator& end) noexcept

src/v/bytes/details/io_placeholder.h

@@ -13,19 +13,17 @@
 
 #include "bytes/details/io_fragment.h"
 #include "bytes/details/out_of_range.h"
-#include "container/intrusive_list_helpers.h"
 
 namespace details {
 class io_placeholder {
 public:
-    using iterator
-      = uncounted_intrusive_list<io_fragment, &io_fragment::hook>::iterator;
+    using iterator = io_fragment_list::iterator;
 
     io_placeholder() noexcept = default;
 
     io_placeholder(
-      const iterator& iter, size_t initial_index, size_t max_size_to_write)
-      : _iter(iter)
+      io_fragment& iter, size_t initial_index, size_t max_size_to_write)
+      : _current(&iter)
       , _byte_index(initial_index)
       , _remaining_size(max_size_to_write) {}
 
@@ -47,13 +45,13 @@ class io_placeholder {
 
     // the first byte of the _current_ iterator + offset
     // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic)
-    const char* index() const { return _iter->get() + _byte_index; }
+    const char* index() const { return _current->get() + _byte_index; }
 
     // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic)
-    char* mutable_index() { return _iter->get_write() + _byte_index; }
+    char* mutable_index() { return _current->get_write() + _byte_index; }
 
 private:
-    iterator _iter;
+    io_fragment* _current = nullptr;
     size_t _byte_index{0};
     size_t _remaining_size{0};
 };