From fcaf98361f6d3fbdf2d04bf3a6b06886e304bad3 Mon Sep 17 00:00:00 2001 From: Gunnar Beutner Date: Fri, 14 May 2021 20:53:04 +0200 Subject: [PATCH] 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 type. Unlike Vector 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 | 24 -- AK/ByteBuffer.h | 350 ++++++++---------- AK/Forward.h | 7 +- AK/StringBuilder.cpp | 16 +- AK/StringBuilder.h | 8 +- Kernel/CMakeLists.txt | 1 - Tests/LibC/snprintf-correctness.cpp | 2 +- Tests/LibC/strlcpy-correctness.cpp | 2 +- .../HexEditor/HexEditorWidget.cpp | 2 +- 9 files changed, 176 insertions(+), 236 deletions(-) delete mode 100644 AK/ByteBuffer.cpp diff --git a/AK/ByteBuffer.cpp b/AK/ByteBuffer.cpp deleted file mode 100644 index ab6b5a21b2..0000000000 --- a/AK/ByteBuffer.cpp +++ /dev/null @@ -1,24 +0,0 @@ -/* - * Copyright (c) 2020, the SerenityOS developers. - * - * SPDX-License-Identifier: BSD-2-Clause - */ - -#include - -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()); -} - -} diff --git a/AK/ByteBuffer.h b/AK/ByteBuffer.h index ff6114af26..3b188695b3 100644 --- a/AK/ByteBuffer.h +++ b/AK/ByteBuffer.h @@ -1,212 +1,180 @@ /* * Copyright (c) 2018-2020, Andreas Kling + * Copyright (c) 2021, Gunnar Beutner * * SPDX-License-Identifier: BSD-2-Clause */ #pragma once -#include -#include -#include #include #include #include namespace AK { +namespace Detail { -class ByteBufferImpl : public RefCounted { -public: - static NonnullRefPtr create_uninitialized(size_t size); - static NonnullRefPtr create_zeroed(size_t); - static NonnullRefPtr copy(const void*, size_t); - - void operator delete(void* ptr) - { - kfree(ptr); - } - - ByteBufferImpl() = delete; - - u8& operator[](size_t i) - { - VERIFY(i < m_size); - return m_data[i]; - } - const u8& operator[](size_t i) const - { - VERIFY(i < m_size); - return m_data[i]; - } - 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 span() { return { data(), size() }; } - Span 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) - { - VERIFY(size <= m_size); - m_size = size; - } - - void zero_fill(); - -private: - explicit ByteBufferImpl(size_t); - - size_t m_size { 0 }; - u8 m_data[]; -}; - +template class ByteBuffer { public: ByteBuffer() = default; - ByteBuffer(const ByteBuffer& other) - : m_impl(other.m_impl) + + ~ByteBuffer() { + clear(); } + + ByteBuffer(ByteBuffer const& other) + { + grow(other.size()); + VERIFY(m_size == other.size()); + VERIFY(!m_is_null); + __builtin_memcpy(data(), other.data(), other.size()); + } + ByteBuffer(ByteBuffer&& other) - : m_impl(move(other.m_impl)) { + move_from(move(other)); } + ByteBuffer& operator=(ByteBuffer&& other) { - if (this != &other) - m_impl = move(other.m_impl); + if (this != &other) { + if (!is_inline()) + kfree(m_outline_buffer); + move_from(move(other)); + } return *this; } - ByteBuffer& operator=(const ByteBuffer& other) + + ByteBuffer& operator=(ByteBuffer const& other) { - if (this != &other) - m_impl = other.m_impl; + 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; } - [[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())); } + [[nodiscard]] static ByteBuffer create_uninitialized(size_t size) + { + return ByteBuffer(size); + } - ~ByteBuffer() { clear(); } - void clear() { m_impl = nullptr; } + [[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; + } + + [[nodiscard]] static ByteBuffer copy(void const* data, size_t size) + { + auto buffer = create_uninitialized(size); + __builtin_memcpy(buffer.data(), data, size); + return buffer; + } + + [[nodiscard]] static ByteBuffer copy(ReadonlyBytes bytes) + { + return copy(bytes.data(), bytes.size()); + } + + template + bool operator==(ByteBuffer 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()); + } + + 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]; - } - [[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]] Bytes bytes() - { - if (m_impl) { - return m_impl->bytes(); - } - return {}; - } - [[nodiscard]] ReadonlyBytes bytes() const - { - if (m_impl) { - return m_impl->bytes(); - } - return {}; + VERIFY(i < m_size); + return data()[i]; } - [[nodiscard]] Span span() - { - if (m_impl) { - return m_impl->span(); - } - return {}; - } - [[nodiscard]] Span span() const - { - if (m_impl) { - return m_impl->span(); - } - return {}; - } + [[nodiscard]] bool is_empty() const { return !m_size; } + [[nodiscard]] size_t size() const { return m_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]] 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]] 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]] Bytes bytes() { return { data(), size() }; } + [[nodiscard]] ReadonlyBytes bytes() const { return { data(), size() }; } - [[nodiscard]] ByteBuffer isolated_copy() const - { - if (!m_impl) - return {}; - return copy(m_impl->data(), m_impl->size()); - } + [[nodiscard]] AK::Span span() { return { data(), size() }; } + [[nodiscard]] AK::Span span() const { return { data(), size() }; } + + [[nodiscard]] u8* offset_pointer(int offset) { return data() + offset; } + [[nodiscard]] u8 const* offset_pointer(int offset) const { return data() + offset; } + + [[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 (m_impl && size < m_impl->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 (!is_inline()) + kfree(m_outline_buffer); + m_size = 0; } - void append(const void* data, size_t data_size) + void grow(size_t new_size) + { + m_is_null = false; + if (new_size <= m_size) + return; + 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(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&& impl) - : m_impl(move(impl)) + ByteBuffer(size_t size) { + grow(size); + VERIFY(!m_is_null); + VERIFY(m_size == size); } - RefPtr m_impl; + 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; + } + + 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; + } + + bool is_inline() const { return m_size <= inline_capacity; } + size_t capacity() const { return is_inline() ? inline_capacity : m_outline_capacity; } + + 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 ByteBufferImpl::ByteBufferImpl(size_t size) - : m_size(size) -{ } - -inline void ByteBufferImpl::zero_fill() -{ - __builtin_memset(m_data, 0, m_size); } - -inline NonnullRefPtr ByteBufferImpl::create_uninitialized(size_t size) -{ - auto* buffer = kmalloc(sizeof(ByteBufferImpl) + size); - VERIFY(buffer); - return ::adopt_ref(*new (buffer) ByteBufferImpl(size)); -} - -inline NonnullRefPtr ByteBufferImpl::create_zeroed(size_t size) -{ - auto buffer = create_uninitialized(size); - if (size != 0) - __builtin_memset(buffer->data(), 0, size); - return buffer; -} - -inline NonnullRefPtr ByteBufferImpl::copy(const void* data, size_t size) -{ - auto buffer = create_uninitialized(size); - __builtin_memcpy(buffer->data(), data, size); - return buffer; -} - -} - -using AK::ByteBuffer; diff --git a/AK/Forward.h b/AK/Forward.h index 1e44690091..aa9234d835 100644 --- a/AK/Forward.h +++ b/AK/Forward.h @@ -10,8 +10,13 @@ namespace AK { -class Bitmap; +namespace Detail { +template class ByteBuffer; +} + +class Bitmap; +using ByteBuffer = AK::Detail::ByteBuffer<32>; class IPv4Address; class JsonArray; class JsonObject; diff --git a/AK/StringBuilder.cpp b/AK/StringBuilder.cpp index aa0329a947..68d5343578 100644 --- a/AK/StringBuilder.cpp +++ b/AK/StringBuilder.cpp @@ -21,23 +21,12 @@ inline void StringBuilder::will_append(size_t size) Checked needed_capacity = m_length; needed_capacity += size; VERIFY(!needed_capacity.has_overflow()); - if (needed_capacity < inline_capacity) - return; - Checked 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; } diff --git a/AK/StringBuilder.h b/AK/StringBuilder.h index e97b76b6af..1ad64d4f39 100644 --- a/AK/StringBuilder.h +++ b/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 m_buffer; size_t m_length { 0 }; }; diff --git a/Kernel/CMakeLists.txt b/Kernel/CMakeLists.txt index c2c3a4cc34..103ba80609 100644 --- a/Kernel/CMakeLists.txt +++ b/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 diff --git a/Tests/LibC/snprintf-correctness.cpp b/Tests/LibC/snprintf-correctness.cpp index 0df2a5cc30..a3de8da7b5 100644 --- a/Tests/LibC/snprintf-correctness.cpp +++ b/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); diff --git a/Tests/LibC/strlcpy-correctness.cpp b/Tests/LibC/strlcpy-correctness.cpp index e23765cd29..c9dfc24128 100644 --- a/Tests/LibC/strlcpy-correctness.cpp +++ b/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); diff --git a/Userland/Applications/HexEditor/HexEditorWidget.cpp b/Userland/Applications/HexEditor/HexEditorWidget.cpp index d5589f690e..b328e8533c 100644 --- a/Userland/Applications/HexEditor/HexEditorWidget.cpp +++ b/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;