AK: Turn ByteBuffer into a value type

Previously ByteBuffer would internally hold a RefPtr to the byte
buffer and would behave like a reference type, i.e. copying a
ByteBuffer would not create a duplicate byte buffer, but rather
two objects which refer to the same internal buffer.

This also changes ByteBuffer so that it has some internal capacity
much like the Vector<T> type. Unlike Vector<T> however a byte
buffer's data may be uninitialized.

With this commit ByteBuffer makes use of the kmalloc_good_size()
API to pick an optimal allocation size for its internal buffer.

AK/ByteBuffer.cpp

@@ -1,24 +0,0 @@
-/*
- * Copyright (c) 2020, the SerenityOS developers.
- *
- * SPDX-License-Identifier: BSD-2-Clause
- */
-
-#include <AK/ByteBuffer.h>
-
-namespace AK {
-
-bool ByteBuffer::operator==(const ByteBuffer& other) const
-{
-    if (is_empty() != other.is_empty())
-        return false;
-    if (is_empty())
-        return true;
-    if (size() != other.size())
-        return false;
-
-    // So they both have data, and the same length.
-    return !__builtin_memcmp(data(), other.data(), size());
-}
-
-}

AK/ByteBuffer.h

@@ -1,212 +1,180 @@
 /*
  * Copyright (c) 2018-2020, Andreas Kling <kling@serenityos.org>
+ * Copyright (c) 2021, Gunnar Beutner <gbeutner@serenityos.org>
  *
  * SPDX-License-Identifier: BSD-2-Clause
  */
 
 #pragma once
 
-#include <AK/NonnullRefPtr.h>
-#include <AK/RefCounted.h>
-#include <AK/RefPtr.h>
 #include <AK/Span.h>
 #include <AK/Types.h>
 #include <AK/kmalloc.h>
 
 namespace AK {
+namespace Detail {
 
-class ByteBufferImpl : public RefCounted<ByteBufferImpl> {
+template<size_t inline_capacity>
+class ByteBuffer {
 public:
-    static NonnullRefPtr<ByteBufferImpl> create_uninitialized(size_t size);
-    static NonnullRefPtr<ByteBufferImpl> create_zeroed(size_t);
-    static NonnullRefPtr<ByteBufferImpl> copy(const void*, size_t);
+    ByteBuffer() = default;
 
-    void operator delete(void* ptr)
+    ~ByteBuffer()
     {
-        kfree(ptr);
+        clear();
     }
 
-    ByteBufferImpl() = delete;
-
-    u8& operator[](size_t i)
+    ByteBuffer(ByteBuffer const& other)
     {
-        VERIFY(i < m_size);
-        return m_data[i];
+        grow(other.size());
+        VERIFY(m_size == other.size());
+        VERIFY(!m_is_null);
+        __builtin_memcpy(data(), other.data(), other.size());
     }
-    const u8& operator[](size_t i) const
+
+    ByteBuffer(ByteBuffer&& other)
     {
-        VERIFY(i < m_size);
-        return m_data[i];
+        move_from(move(other));
     }
-    bool is_empty() const { return !m_size; }
-    size_t size() const { return m_size; }
-
-    u8* data() { return m_data; }
-    const u8* data() const { return m_data; }
-
-    Bytes bytes() { return { data(), size() }; }
-    ReadonlyBytes bytes() const { return { data(), size() }; }
-
-    Span<u8> span() { return { data(), size() }; }
-    Span<const u8> span() const { return { data(), size() }; }
-
-    u8* offset_pointer(int offset) { return m_data + offset; }
-    const u8* offset_pointer(int offset) const { return m_data + offset; }
 
-    void* end_pointer() { return m_data + m_size; }
-    const void* end_pointer() const { return m_data + m_size; }
-
-    // NOTE: trim() does not reallocate.
-    void trim(size_t size)
+    ByteBuffer& operator=(ByteBuffer&& other)
     {
-        VERIFY(size <= m_size);
-        m_size = size;
+        if (this != &other) {
+            if (!is_inline())
+                kfree(m_outline_buffer);
+            move_from(move(other));
+        }
+        return *this;
     }
 
-    void zero_fill();
-
-private:
-    explicit ByteBufferImpl(size_t);
-
-    size_t m_size { 0 };
-    u8 m_data[];
-};
+    ByteBuffer& operator=(ByteBuffer const& other)
+    {
+        if (this != &other) {
+            if (m_size > other.size())
+                internal_trim(other.size(), true);
+            else
+                grow(other.size());
+            __builtin_memcpy(data(), other.data(), other.size());
+        }
+        return *this;
+    }
 
-class ByteBuffer {
-public:
-    ByteBuffer() = default;
-    ByteBuffer(const ByteBuffer& other)
-        : m_impl(other.m_impl)
+    [[nodiscard]] static ByteBuffer create_uninitialized(size_t size)
     {
+        return ByteBuffer(size);
     }
-    ByteBuffer(ByteBuffer&& other)
-        : m_impl(move(other.m_impl))
+
+    [[nodiscard]] static ByteBuffer create_zeroed(size_t size)
     {
+        auto buffer = create_uninitialized(size);
+        buffer.zero_fill();
+        VERIFY(size == 0 || (buffer[0] == 0 && buffer[size - 1] == 0));
+        return buffer;
     }
-    ByteBuffer& operator=(ByteBuffer&& other)
+
+    [[nodiscard]] static ByteBuffer copy(void const* data, size_t size)
     {
-        if (this != &other)
-            m_impl = move(other.m_impl);
-        return *this;
+        auto buffer = create_uninitialized(size);
+        __builtin_memcpy(buffer.data(), data, size);
+        return buffer;
     }
-    ByteBuffer& operator=(const ByteBuffer& other)
+
+    [[nodiscard]] static ByteBuffer copy(ReadonlyBytes bytes)
     {
-        if (this != &other)
-            m_impl = other.m_impl;
-        return *this;
+        return copy(bytes.data(), bytes.size());
     }
 
-    [[nodiscard]] static ByteBuffer create_uninitialized(size_t size) { return ByteBuffer(ByteBufferImpl::create_uninitialized(size)); }
-    [[nodiscard]] static ByteBuffer create_zeroed(size_t size) { return ByteBuffer(ByteBufferImpl::create_zeroed(size)); }
-    [[nodiscard]] static ByteBuffer copy(const void* data, size_t size) { return ByteBuffer(ByteBufferImpl::copy(data, size)); }
-    [[nodiscard]] static ByteBuffer copy(ReadonlyBytes bytes) { return ByteBuffer(ByteBufferImpl::copy(bytes.data(), bytes.size())); }
+    template<size_t other_inline_capacity>
+    bool operator==(ByteBuffer<other_inline_capacity> const& other) const
+    {
+        if (size() != other.size())
+            return false;
+
+        // So they both have data, and the same length.
+        return !__builtin_memcmp(data(), other.data(), size());
+    }
 
-    ~ByteBuffer() { clear(); }
-    void clear() { m_impl = nullptr; }
+    bool operator!=(ByteBuffer const& other) const { return !(*this == other); }
 
     operator bool() const { return !is_null(); }
     bool operator!() const { return is_null(); }
-    [[nodiscard]] bool is_null() const { return m_impl == nullptr; }
-
-    // Disable default implementations that would use surprising integer promotion.
-    bool operator==(const ByteBuffer& other) const;
-    bool operator!=(const ByteBuffer& other) const { return !(*this == other); }
-    bool operator<=(const ByteBuffer& other) const = delete;
-    bool operator>=(const ByteBuffer& other) const = delete;
-    bool operator<(const ByteBuffer& other) const = delete;
-    bool operator>(const ByteBuffer& other) const = delete;
+    [[nodiscard]] bool is_null() const { return m_is_null; }
 
     [[nodiscard]] u8& operator[](size_t i)
     {
-        VERIFY(m_impl);
-        return (*m_impl)[i];
+        VERIFY(i < m_size);
+        return data()[i];
     }
-    [[nodiscard]] u8 operator[](size_t i) const
+
+    [[nodiscard]] u8 const& operator[](size_t i) const
     {
-        VERIFY(m_impl);
-        return (*m_impl)[i];
+        VERIFY(i < m_size);
+        return data()[i];
     }
-    [[nodiscard]] bool is_empty() const { return !m_impl || m_impl->is_empty(); }
-    [[nodiscard]] size_t size() const { return m_impl ? m_impl->size() : 0; }
 
-    [[nodiscard]] u8* data() { return m_impl ? m_impl->data() : nullptr; }
-    [[nodiscard]] const u8* data() const { return m_impl ? m_impl->data() : nullptr; }
+    [[nodiscard]] bool is_empty() const { return !m_size; }
+    [[nodiscard]] size_t size() const { return m_size; }
 
-    [[nodiscard]] Bytes bytes()
-    {
-        if (m_impl) {
-            return m_impl->bytes();
-        }
-        return {};
-    }
-    [[nodiscard]] ReadonlyBytes bytes() const
-    {
-        if (m_impl) {
-            return m_impl->bytes();
-        }
-        return {};
-    }
+    [[nodiscard]] u8* data() { return is_inline() ? m_inline_buffer : m_outline_buffer; }
+    [[nodiscard]] u8 const* data() const { return is_inline() ? m_inline_buffer : m_outline_buffer; }
 
-    [[nodiscard]] Span<u8> span()
-    {
-        if (m_impl) {
-            return m_impl->span();
-        }
-        return {};
-    }
-    [[nodiscard]] Span<const u8> span() const
-    {
-        if (m_impl) {
-            return m_impl->span();
-        }
-        return {};
-    }
+    [[nodiscard]] Bytes bytes() { return { data(), size() }; }
+    [[nodiscard]] ReadonlyBytes bytes() const { return { data(), size() }; }
 
-    [[nodiscard]] u8* offset_pointer(int offset) { return m_impl ? m_impl->offset_pointer(offset) : nullptr; }
-    [[nodiscard]] const u8* offset_pointer(int offset) const { return m_impl ? m_impl->offset_pointer(offset) : nullptr; }
+    [[nodiscard]] AK::Span<u8> span() { return { data(), size() }; }
+    [[nodiscard]] AK::Span<const u8> span() const { return { data(), size() }; }
 
-    [[nodiscard]] void* end_pointer() { return m_impl ? m_impl->end_pointer() : nullptr; }
-    [[nodiscard]] const void* end_pointer() const { return m_impl ? m_impl->end_pointer() : nullptr; }
+    [[nodiscard]] u8* offset_pointer(int offset) { return data() + offset; }
+    [[nodiscard]] u8 const* offset_pointer(int offset) const { return data() + offset; }
 
-    [[nodiscard]] ByteBuffer isolated_copy() const
-    {
-        if (!m_impl)
-            return {};
-        return copy(m_impl->data(), m_impl->size());
-    }
+    [[nodiscard]] void* end_pointer() { return data() + m_size; }
+    [[nodiscard]] void const* end_pointer() const { return data() + m_size; }
 
-    // NOTE: trim() does not reallocate.
     void trim(size_t size)
     {
-        if (m_impl)
-            m_impl->trim(size);
+        internal_trim(size, false);
     }
 
     [[nodiscard]] ByteBuffer slice(size_t offset, size_t size) const
     {
-        if (is_null())
-            return {};
-
-        if (offset == 0 && size == this->size())
-            return *this;
-
         // I cannot hand you a slice I don't have
         VERIFY(offset + size <= this->size());
 
         return copy(offset_pointer(offset), size);
     }
 
-    void grow(size_t size)
+    void clear()
+    {
+        if (!is_inline())
+            kfree(m_outline_buffer);
+        m_size = 0;
+    }
+
+    void grow(size_t new_size)
     {
-        if (m_impl && size < m_impl->size())
+        m_is_null = false;
+        if (new_size <= m_size)
             return;
-        auto new_impl = ByteBufferImpl::create_uninitialized(size);
-        if (m_impl)
-            __builtin_memcpy(new_impl->data(), m_impl->data(), m_impl->size());
-        m_impl = new_impl;
+        if (new_size <= capacity()) {
+            m_size = new_size;
+            return;
+        }
+        u8* new_buffer;
+        auto new_capacity = kmalloc_good_size(new_size);
+        if (!is_inline()) {
+            new_buffer = (u8*)krealloc(m_outline_buffer, new_capacity);
+            VERIFY(new_buffer);
+        } else {
+            new_buffer = (u8*)kmalloc(new_capacity);
+            VERIFY(new_buffer);
+            __builtin_memcpy(new_buffer, data(), m_size);
+        }
+        m_outline_buffer = new_buffer;
+        m_outline_capacity = new_capacity;
+        m_size = new_size;
     }
 
-    void append(const void* data, size_t data_size)
+    void append(void const* data, size_t data_size)
     {
         if (data_size == 0)
             return;
@@ -216,12 +184,12 @@ public:
         __builtin_memcpy(this->data() + old_size, data, data_size);
     }
 
-    void operator+=(const ByteBuffer& other)
+    void operator+=(ByteBuffer const& other)
     {
         append(other.data(), other.size());
     }
 
-    void overwrite(size_t offset, const void* data, size_t data_size)
+    void overwrite(size_t offset, void const* data, size_t data_size)
     {
         // make sure we're not told to write past the end
         VERIFY(offset + data_size <= size());
@@ -230,53 +198,59 @@ public:
 
     void zero_fill()
     {
-        m_impl->zero_fill();
+        __builtin_memset(data(), 0, m_size);
     }
 
     operator Bytes() { return bytes(); }
     operator ReadonlyBytes() const { return bytes(); }
 
 private:
-    explicit ByteBuffer(RefPtr<ByteBufferImpl>&& impl)
-        : m_impl(move(impl))
+    ByteBuffer(size_t size)
     {
+        grow(size);
+        VERIFY(!m_is_null);
+        VERIFY(m_size == size);
     }
 
-    RefPtr<ByteBufferImpl> m_impl;
-};
-
-inline ByteBufferImpl::ByteBufferImpl(size_t size)
-    : m_size(size)
-{
-}
+    void move_from(ByteBuffer&& other)
+    {
+        m_is_null = other.m_is_null;
+        m_size = other.m_size;
+        if (other.m_size > inline_capacity) {
+            m_outline_buffer = other.m_outline_buffer;
+            m_outline_capacity = other.m_outline_capacity;
+        } else
+            __builtin_memcpy(m_inline_buffer, other.m_inline_buffer, other.m_size);
+        other.m_is_null = true;
+        other.m_size = 0;
+    }
 
-inline void ByteBufferImpl::zero_fill()
-{
-    __builtin_memset(m_data, 0, m_size);
-}
+    void internal_trim(size_t size, bool may_discard_existing_data)
+    {
+        VERIFY(size <= m_size);
+        if (!is_inline() && size <= inline_capacity) {
+            // m_inline_buffer and m_outline_buffer are part of a union, so save the pointer
+            auto outline_buffer = m_outline_buffer;
+            if (!may_discard_existing_data)
+                __builtin_memcpy(m_inline_buffer, outline_buffer, size);
+            kfree(outline_buffer);
+        }
+        m_size = size;
+    }
 
-inline NonnullRefPtr<ByteBufferImpl> ByteBufferImpl::create_uninitialized(size_t size)
-{
-    auto* buffer = kmalloc(sizeof(ByteBufferImpl) + size);
-    VERIFY(buffer);
-    return ::adopt_ref(*new (buffer) ByteBufferImpl(size));
-}
+    bool is_inline() const { return m_size <= inline_capacity; }
+    size_t capacity() const { return is_inline() ? inline_capacity : m_outline_capacity; }
 
-inline NonnullRefPtr<ByteBufferImpl> ByteBufferImpl::create_zeroed(size_t size)
-{
-    auto buffer = create_uninitialized(size);
-    if (size != 0)
-        __builtin_memset(buffer->data(), 0, size);
-    return buffer;
-}
+    size_t m_size { 0 };
+    bool m_is_null { true };
+    union {
+        u8 m_inline_buffer[inline_capacity];
+        struct {
+            u8* m_outline_buffer;
+            size_t m_outline_capacity;
+        };
+    };
+};
 
-inline NonnullRefPtr<ByteBufferImpl> ByteBufferImpl::copy(const void* data, size_t size)
-{
-    auto buffer = create_uninitialized(size);
-    __builtin_memcpy(buffer->data(), data, size);
-    return buffer;
 }
-
 }
-
-using AK::ByteBuffer;

AK/Forward.h

@@ -10,8 +10,13 @@
 
 namespace AK {
 
-class Bitmap;
+namespace Detail {
+template<size_t inline_capacity>
 class ByteBuffer;
+}
+
+class Bitmap;
+using ByteBuffer = AK::Detail::ByteBuffer<32>;
 class IPv4Address;
 class JsonArray;
 class JsonObject;

AK/StringBuilder.cpp

@@ -21,23 +21,12 @@ inline void StringBuilder::will_append(size_t size)
     Checked<size_t> needed_capacity = m_length;
     needed_capacity += size;
     VERIFY(!needed_capacity.has_overflow());
-    if (needed_capacity < inline_capacity)
-        return;
-    Checked<size_t> expanded_capacity = needed_capacity;
-    expanded_capacity *= 2;
-    VERIFY(!expanded_capacity.has_overflow());
-    if (m_buffer.is_null()) {
-        m_buffer.grow(expanded_capacity.value());
-        memcpy(m_buffer.data(), m_inline_buffer, m_length);
-    } else if (needed_capacity.value() > m_buffer.size()) {
-        m_buffer.grow(expanded_capacity.value());
-    }
+    m_buffer.grow(needed_capacity.value());
 }
 
 StringBuilder::StringBuilder(size_t initial_capacity)
+    : m_buffer(decltype(m_buffer)::create_uninitialized(initial_capacity))
 {
-    if (initial_capacity > inline_capacity)
-        m_buffer.grow(initial_capacity);
 }
 
 void StringBuilder::append(const StringView& str)
@@ -94,7 +83,6 @@ StringView StringBuilder::string_view() const
 void StringBuilder::clear()
 {
     m_buffer.clear();
-    m_inline_buffer[0] = '\0';
     m_length = 0;
 }
 

AK/StringBuilder.h

@@ -62,13 +62,11 @@ public:
 
 private:
     void will_append(size_t);
-    u8* data() { return m_buffer.is_null() ? m_inline_buffer : m_buffer.data(); }
-    const u8* data() const { return m_buffer.is_null() ? m_inline_buffer : m_buffer.data(); }
-    bool using_inline_buffer() const { return m_buffer.is_null(); }
+    u8* data() { return m_buffer.data(); }
+    const u8* data() const { return m_buffer.data(); }
 
     static constexpr size_t inline_capacity = 128;
-    u8 m_inline_buffer[inline_capacity];
-    ByteBuffer m_buffer;
+    AK::Detail::ByteBuffer<inline_capacity> m_buffer;
     size_t m_length { 0 };
 };
 

Kernel/CMakeLists.txt

@@ -256,7 +256,6 @@ set(KERNEL_SOURCES
 )
 
 set(AK_SOURCES
-    ../AK/ByteBuffer.cpp
     ../AK/FlyString.cpp
     ../AK/GenericLexer.cpp
     ../AK/Hex.cpp

Tests/LibC/snprintf-correctness.cpp

@@ -55,7 +55,7 @@ static bool test_single(const Testcase& testcase)
     // Setup
     ByteBuffer actual = ByteBuffer::create_uninitialized(SANDBOX_CANARY_SIZE + testcase.dest_n + SANDBOX_CANARY_SIZE);
     fill_with_random(actual.data(), actual.size());
-    ByteBuffer expected = actual.isolated_copy();
+    ByteBuffer expected = actual;
     VERIFY(actual.offset_pointer(0) != expected.offset_pointer(0));
     actual.overwrite(SANDBOX_CANARY_SIZE, testcase.dest, testcase.dest_n);
     expected.overwrite(SANDBOX_CANARY_SIZE, testcase.dest_expected, testcase.dest_expected_n);

Tests/LibC/strlcpy-correctness.cpp

@@ -57,7 +57,7 @@ static bool test_single(const Testcase& testcase)
     // Setup
     ByteBuffer actual = ByteBuffer::create_uninitialized(SANDBOX_CANARY_SIZE + testcase.dest_n + SANDBOX_CANARY_SIZE);
     fill_with_random(actual.data(), actual.size());
-    ByteBuffer expected = actual.isolated_copy();
+    ByteBuffer expected = actual;
     VERIFY(actual.offset_pointer(0) != expected.offset_pointer(0));
     actual.overwrite(SANDBOX_CANARY_SIZE, testcase.dest, testcase.dest_n);
     expected.overwrite(SANDBOX_CANARY_SIZE, testcase.dest_expected, testcase.dest_expected_n);

Userland/Applications/HexEditor/HexEditorWidget.cpp

@@ -169,7 +169,7 @@ void HexEditorWidget::initialize_menubar(GUI::Menubar& menubar)
     }));
     edit_menu.add_separator();
     edit_menu.add_action(GUI::Action::create("&Find", { Mod_Ctrl, Key_F }, Gfx::Bitmap::load_from_file("/res/icons/16x16/find.png"), [&](const GUI::Action&) {
-        auto old_buffer = m_search_buffer.isolated_copy();
+        auto old_buffer = m_search_buffer;
         if (FindDialog::show(window(), m_search_text, m_search_buffer) == GUI::InputBox::ExecOK) {
 
             bool same_buffers = false;