From e4412f1f599bea034dea608b8c7dcc4408d90066 Mon Sep 17 00:00:00 2001 From: AnotherTest Date: Fri, 16 Apr 2021 16:33:24 +0430 Subject: [PATCH] AK+Kernel: Make IntrusiveList capable of holding non-raw pointers This should allow creating intrusive lists that have smart pointers, while remaining free (compared to the impl before this commit) when holding raw pointers :^) As a sidenote, this also adds a `RawPtr` type, which is just equivalent to `T*`. Note that this does not actually use such functionality, but is only expected to pave the way for #6369, to replace NonnullRefPtrVector with intrusive lists. As it is with zero-cost things, this makes the interface a bit less nice by requiring the type name of what an `IntrusiveListNode` holds (and optionally its container, if not RawPtr), and also requiring the type of the container (normally `RawPtr`) on the `IntrusiveList` instance. --- AK/IntrusiveList.h | 180 +++++++++++++-------- AK/StdLibExtras.h | 11 ++ Kernel/FileSystem/BlockBasedFileSystem.cpp | 6 +- Kernel/Process.h | 2 +- Kernel/Scheduler.cpp | 2 +- Kernel/Thread.h | 6 +- Kernel/WorkQueue.h | 4 +- Userland/Libraries/LibCore/Object.cpp | 4 +- Userland/Libraries/LibCore/Object.h | 4 +- Userland/Libraries/LibJS/Heap/Allocator.h | 2 +- Userland/Libraries/LibJS/Heap/HeapBlock.h | 2 +- 11 files changed, 143 insertions(+), 80 deletions(-) diff --git a/AK/IntrusiveList.h b/AK/IntrusiveList.h index b5fd221bd7..411f6ca220 100644 --- a/AK/IntrusiveList.h +++ b/AK/IntrusiveList.h @@ -28,20 +28,25 @@ #include #include +#include +#include namespace AK { +template> class IntrusiveListNode; + +template class IntrusiveListStorage { private: - friend class IntrusiveListNode; - template + friend class IntrusiveListNode; + template T_::*member> friend class IntrusiveList; - IntrusiveListNode* m_first { nullptr }; - IntrusiveListNode* m_last { nullptr }; + IntrusiveListNode* m_first { nullptr }; + IntrusiveListNode* m_last { nullptr }; }; -template +template T::*member> class IntrusiveList { public: IntrusiveList(); @@ -53,27 +58,29 @@ public: void prepend(T& n); void remove(T& n); [[nodiscard]] bool contains(const T&) const; - [[nodiscard]] T* first() const; - [[nodiscard]] T* last() const; + [[nodiscard]] Container first() const; + [[nodiscard]] Container last() const; - [[nodiscard]] T* take_first(); - [[nodiscard]] T* take_last(); + [[nodiscard]] Container take_first(); + [[nodiscard]] Container take_last(); class Iterator { public: Iterator() = default; Iterator(T* value) - : m_value(value) + : m_value(move(value)) { } - T& operator*() const { return *m_value; } - T* operator->() const { return m_value; } + const T& operator*() const { return *m_value; } + auto operator->() const { return m_value; } + T& operator*() { return *m_value; } + auto operator->() { return m_value; } bool operator==(const Iterator& other) const { return other.m_value == m_value; } bool operator!=(const Iterator& other) const { return !(*this == other); } Iterator& operator++() { - m_value = IntrusiveList::next(m_value); + m_value = IntrusiveList::next(m_value); return *this; } Iterator& erase(); @@ -94,12 +101,12 @@ public: } const T& operator*() const { return *m_value; } - const T* operator->() const { return m_value; } + auto operator->() const { return m_value; } bool operator==(const ConstIterator& other) const { return other.m_value == m_value; } bool operator!=(const ConstIterator& other) const { return !(*this == other); } ConstIterator& operator++() { - m_value = IntrusiveList::next(const_cast(m_value)); + m_value = IntrusiveList::next(m_value); return *this; } @@ -112,68 +119,81 @@ public: private: static T* next(T* current); - static T* node_to_value(IntrusiveListNode& node); - IntrusiveListStorage m_storage; + static const T* next(const T* current); + static T* node_to_value(IntrusiveListNode& node); + IntrusiveListStorage m_storage; }; +template +struct SelfReferenceIfNeeded { + Contained reference = nullptr; +}; +template +struct SelfReferenceIfNeeded { +}; + +template class IntrusiveListNode { public: ~IntrusiveListNode(); void remove(); bool is_in_list() const; + static constexpr bool IsRaw = IsPointer; + private: - template + template T_::*member> friend class IntrusiveList; - IntrusiveListStorage* m_storage = nullptr; - IntrusiveListNode* m_next = nullptr; - IntrusiveListNode* m_prev = nullptr; + IntrusiveListStorage* m_storage = nullptr; + IntrusiveListNode* m_next = nullptr; + IntrusiveListNode* m_prev = nullptr; + SelfReferenceIfNeeded m_self; }; -template -inline typename IntrusiveList::Iterator& IntrusiveList::Iterator::erase() +template T::*member> +inline typename IntrusiveList::Iterator& IntrusiveList::Iterator::erase() { - T* old = m_value; - m_value = IntrusiveList::next(m_value); + auto old = m_value; + m_value = IntrusiveList::next(m_value); (old->*member).remove(); return *this; } -template -inline IntrusiveList::IntrusiveList() - +template T::*member> +inline IntrusiveList::IntrusiveList() { } -template -inline IntrusiveList::~IntrusiveList() +template T::*member> +inline IntrusiveList::~IntrusiveList() { clear(); } -template -inline void IntrusiveList::clear() +template T::*member> +inline void IntrusiveList::clear() { while (m_storage.m_first) m_storage.m_first->remove(); } -template -inline bool IntrusiveList::is_empty() const +template T::*member> +inline bool IntrusiveList::is_empty() const { return m_storage.m_first == nullptr; } -template -inline void IntrusiveList::append(T& n) +template T::*member> +inline void IntrusiveList::append(T& n) { - auto& nnode = n.*member; - if (nnode.m_storage) - nnode.remove(); + remove(n); + auto& nnode = n.*member; nnode.m_storage = &m_storage; nnode.m_prev = m_storage.m_last; nnode.m_next = nullptr; + if constexpr (!RemoveReference::IsRaw) + nnode.m_self.reference = &n; // Note: Self-reference ensures that the object will keep a ref to itself when the Container is a smart pointer. if (m_storage.m_last) m_storage.m_last->m_next = &nnode; @@ -182,8 +202,8 @@ inline void IntrusiveList::append(T& n) m_storage.m_first = &nnode; } -template -inline void IntrusiveList::prepend(T& n) +template T::*member> +inline void IntrusiveList::prepend(T& n) { auto& nnode = n.*member; if (nnode.m_storage) @@ -192,6 +212,8 @@ inline void IntrusiveList::prepend(T& n) nnode.m_storage = &m_storage; nnode.m_prev = nullptr; nnode.m_next = m_storage.m_first; + if constexpr (!RemoveReference::IsRaw) + nnode.m_self.reference = &n; if (m_storage.m_first) m_storage.m_first->m_prev = &nnode; @@ -200,29 +222,29 @@ inline void IntrusiveList::prepend(T& n) m_storage.m_last = &nnode; } -template -inline void IntrusiveList::remove(T& n) +template T::*member> +inline void IntrusiveList::remove(T& n) { auto& nnode = n.*member; if (nnode.m_storage) nnode.remove(); } -template -inline bool IntrusiveList::contains(const T& n) const +template T::*member> +inline bool IntrusiveList::contains(const T& n) const { auto& nnode = n.*member; return nnode.m_storage == &m_storage; } -template -inline T* IntrusiveList::first() const +template T::*member> +inline Container IntrusiveList::first() const { return m_storage.m_first ? node_to_value(*m_storage.m_first) : nullptr; } -template -inline T* IntrusiveList::take_first() +template T::*member> +inline Container IntrusiveList::take_first() { if (auto* ptr = first()) { remove(*ptr); @@ -231,8 +253,8 @@ inline T* IntrusiveList::take_first() return nullptr; } -template -inline T* IntrusiveList::take_last() +template T::*member> +inline Container IntrusiveList::take_last() { if (auto* ptr = last()) { remove(*ptr); @@ -241,34 +263,42 @@ inline T* IntrusiveList::take_last() return nullptr; } -template -inline T* IntrusiveList::last() const +template T::*member> +inline Container IntrusiveList::last() const { return m_storage.m_last ? node_to_value(*m_storage.m_last) : nullptr; } -template -inline T* IntrusiveList::next(T* current) +template T::*member> +inline const T* IntrusiveList::next(const T* current) +{ + auto& nextnode = (current->*member).m_next; + const T* nextstruct = nextnode ? node_to_value(*nextnode) : nullptr; + return nextstruct; +} + +template T::*member> +inline T* IntrusiveList::next(T* current) { auto& nextnode = (current->*member).m_next; T* nextstruct = nextnode ? node_to_value(*nextnode) : nullptr; return nextstruct; } -template -inline typename IntrusiveList::Iterator IntrusiveList::begin() +template T::*member> +inline typename IntrusiveList::Iterator IntrusiveList::begin() { return m_storage.m_first ? Iterator(node_to_value(*m_storage.m_first)) : Iterator(); } -template -inline typename IntrusiveList::ConstIterator IntrusiveList::begin() const +template T::*member> +inline typename IntrusiveList::ConstIterator IntrusiveList::begin() const { return m_storage.m_first ? ConstIterator(node_to_value(*m_storage.m_first)) : ConstIterator(); } -template -inline T* IntrusiveList::node_to_value(IntrusiveListNode& node) +template T::*member> +inline T* IntrusiveList::node_to_value(IntrusiveListNode& node) { // Note: Since this might seem odd, here's an explanation on what this function actually does: // `node` is a reference that resides in some part of the actual value (of type T), the @@ -279,13 +309,15 @@ inline T* IntrusiveList::node_to_value(IntrusiveListNode& node) return bit_cast(bit_cast(&node) - bit_cast(member)); } -inline IntrusiveListNode::~IntrusiveListNode() +template +inline IntrusiveListNode::~IntrusiveListNode() { if (m_storage) remove(); } -inline void IntrusiveListNode::remove() +template +inline void IntrusiveListNode::remove() { VERIFY(m_storage); if (m_storage->m_first == this) @@ -299,13 +331,33 @@ inline void IntrusiveListNode::remove() m_prev = nullptr; m_next = nullptr; m_storage = nullptr; + if constexpr (!IsRaw) + m_self.reference = nullptr; } -inline bool IntrusiveListNode::is_in_list() const +template +inline bool IntrusiveListNode::is_in_list() const { return m_storage != nullptr; } +// Specialise IntrusiveList(Node) for NonnullRefPtr +// By default, intrusive lists cannot contain null entries anyway, so switch to RefPtr +// and just make the user-facing functions deref the pointers. +template +struct IntrusiveListNode> : public IntrusiveListNode> { +}; + +template> T::*member> +class IntrusiveList, member> : public IntrusiveList, member> { +public: + [[nodiscard]] NonnullRefPtr first() const { return *IntrusiveList, member>::first(); } + [[nodiscard]] NonnullRefPtr last() const { return *IntrusiveList, member>::last(); } + + [[nodiscard]] NonnullRefPtr take_first() { return *IntrusiveList, member>::take_first(); } + [[nodiscard]] NonnullRefPtr take_last() { return *IntrusiveList, member>::take_last(); } +}; + } using AK::IntrusiveList; diff --git a/AK/StdLibExtras.h b/AK/StdLibExtras.h index 8f1845b6d3..482ceaea9d 100644 --- a/AK/StdLibExtras.h +++ b/AK/StdLibExtras.h @@ -49,6 +49,13 @@ constexpr T&& move(T& arg) using std::move; +namespace AK::Detail { +template +struct _RawPtr { + using Type = T*; +}; +} + namespace AK { template @@ -122,6 +129,9 @@ constexpr T exchange(T& slot, U&& value) return old_value; } +template +using RawPtr = typename Detail::_RawPtr::Type; + } using AK::array_size; @@ -132,4 +142,5 @@ using AK::exchange; using AK::forward; using AK::max; using AK::min; +using AK::RawPtr; using AK::swap; diff --git a/Kernel/FileSystem/BlockBasedFileSystem.cpp b/Kernel/FileSystem/BlockBasedFileSystem.cpp index 8bd2e70d00..93fcbff6f9 100644 --- a/Kernel/FileSystem/BlockBasedFileSystem.cpp +++ b/Kernel/FileSystem/BlockBasedFileSystem.cpp @@ -32,7 +32,7 @@ namespace Kernel { struct CacheEntry { - IntrusiveListNode list_node; + IntrusiveListNode list_node; BlockBasedFS::BlockIndex block_index { 0 }; u8* data { nullptr }; bool has_data { false }; @@ -117,8 +117,8 @@ private: BlockBasedFS& m_fs; size_t m_entry_count { 10000 }; mutable HashMap m_hash; - mutable IntrusiveList m_clean_list; - mutable IntrusiveList m_dirty_list; + mutable IntrusiveList, &CacheEntry::list_node> m_clean_list; + mutable IntrusiveList, &CacheEntry::list_node> m_dirty_list; KBuffer m_cached_block_data; KBuffer m_entries; bool m_dirty { false }; diff --git a/Kernel/Process.h b/Kernel/Process.h index 7848baa312..925c5872cd 100644 --- a/Kernel/Process.h +++ b/Kernel/Process.h @@ -120,7 +120,7 @@ protected: mode_t m_umask { 022 }; VirtualAddress m_signal_trampoline; Atomic m_thread_count { 0 }; - IntrusiveList m_thread_list; + IntrusiveList, &Thread::m_process_thread_list_node> m_thread_list; u8 m_termination_status { 0 }; u8 m_termination_signal { 0 }; }; diff --git a/Kernel/Scheduler.cpp b/Kernel/Scheduler.cpp index f6c85fabb0..4b8e33ee56 100644 --- a/Kernel/Scheduler.cpp +++ b/Kernel/Scheduler.cpp @@ -73,7 +73,7 @@ Atomic g_finalizer_has_work { false }; READONLY_AFTER_INIT static Process* s_colonel_process; struct ThreadReadyQueue { - IntrusiveList thread_list; + IntrusiveList, &Thread::m_ready_queue_node> thread_list; }; static SpinLock g_ready_queues_lock; static u32 g_ready_queues_mask; diff --git a/Kernel/Thread.h b/Kernel/Thread.h index 1b26e37d98..9bc1f33662 100644 --- a/Kernel/Thread.h +++ b/Kernel/Thread.h @@ -88,7 +88,7 @@ class Thread friend class Process; friend class ProtectedProcessBase; friend class Scheduler; - friend class ThreadReadyQueue; + friend struct ThreadReadyQueue; static SpinLock g_tid_map_lock; static HashMap* g_tid_map; @@ -1129,7 +1129,7 @@ public: private: Thread(NonnullRefPtr, NonnullOwnPtr kernel_stack_region); - IntrusiveListNode m_process_thread_list_node; + IntrusiveListNode m_process_thread_list_node; int m_runnable_priority { -1 }; friend class WaitQueue; @@ -1202,7 +1202,7 @@ private: TSS m_tss {}; TrapFrame* m_current_trap { nullptr }; u32 m_saved_critical { 1 }; - IntrusiveListNode m_ready_queue_node; + IntrusiveListNode m_ready_queue_node; Atomic m_cpu { 0 }; u32 m_cpu_affinity { THREAD_AFFINITY_DEFAULT }; u32 m_ticks_left { 0 }; diff --git a/Kernel/WorkQueue.h b/Kernel/WorkQueue.h index bf62d84cc0..918ced00ef 100644 --- a/Kernel/WorkQueue.h +++ b/Kernel/WorkQueue.h @@ -75,7 +75,7 @@ public: private: struct WorkItem { - IntrusiveListNode m_node; + IntrusiveListNode m_node; void (*function)(void*); void* data; void (*free_data)(void*); @@ -86,7 +86,7 @@ private: RefPtr m_thread; WaitQueue m_wait_queue; - IntrusiveList m_items; + IntrusiveList, &WorkItem::m_node> m_items; SpinLock m_lock; }; diff --git a/Userland/Libraries/LibCore/Object.cpp b/Userland/Libraries/LibCore/Object.cpp index fc57e8be56..13c57baebf 100644 --- a/Userland/Libraries/LibCore/Object.cpp +++ b/Userland/Libraries/LibCore/Object.cpp @@ -34,9 +34,9 @@ namespace Core { -IntrusiveList& Object::all_objects() +IntrusiveList, &Object::m_all_objects_list_node>& Object::all_objects() { - static IntrusiveList objects; + static IntrusiveList, &Object::m_all_objects_list_node> objects; return objects; } diff --git a/Userland/Libraries/LibCore/Object.h b/Userland/Libraries/LibCore/Object.h index 4b0ced96be..21980c8b0e 100644 --- a/Userland/Libraries/LibCore/Object.h +++ b/Userland/Libraries/LibCore/Object.h @@ -67,7 +67,7 @@ class Object AK_MAKE_NONCOPYABLE(Object); AK_MAKE_NONMOVABLE(Object); - IntrusiveListNode m_all_objects_list_node; + IntrusiveListNode m_all_objects_list_node; public: virtual ~Object(); @@ -124,7 +124,7 @@ public: JsonValue property(const StringView& name) const; const HashMap>& properties() const { return m_properties; } - static IntrusiveList& all_objects(); + static IntrusiveList, &Object::m_all_objects_list_node>& all_objects(); void dispatch_event(Core::Event&, Object* stay_within = nullptr); diff --git a/Userland/Libraries/LibJS/Heap/Allocator.h b/Userland/Libraries/LibJS/Heap/Allocator.h index fd89323de7..1b20ea9fe6 100644 --- a/Userland/Libraries/LibJS/Heap/Allocator.h +++ b/Userland/Libraries/LibJS/Heap/Allocator.h @@ -63,7 +63,7 @@ public: private: const size_t m_cell_size; - typedef IntrusiveList BlockList; + typedef IntrusiveList, &HeapBlock::m_list_node> BlockList; BlockList m_full_blocks; BlockList m_usable_blocks; }; diff --git a/Userland/Libraries/LibJS/Heap/HeapBlock.h b/Userland/Libraries/LibJS/Heap/HeapBlock.h index 459ae5f80a..c4677a477f 100644 --- a/Userland/Libraries/LibJS/Heap/HeapBlock.h +++ b/Userland/Libraries/LibJS/Heap/HeapBlock.h @@ -86,7 +86,7 @@ public: return cell_from_possible_pointer((FlatPtr)cell); } - IntrusiveListNode m_list_node; + IntrusiveListNode m_list_node; private: HeapBlock(Heap&, size_t cell_size);