From fe2a47b5a79f2e879ef7e84ea6e18d26d5b572c6 Mon Sep 17 00:00:00 2001 From: Jacques Lucke Date: Wed, 28 Feb 2024 22:22:21 +0100 Subject: [PATCH] BLI: add chunked list data structure that uses linear allocator This adds a new special purpose container data structure that can be used to gather many elements into many (potentially small) lists efficiently. I originally worked on this data structure because I might want to use it in #118772. However, also it's useful in the geometry nodes logger already. I'm measuring a 10-20% speed improvement in my many-math-nodes file when I enable logging for all sockets (not just the ones that are currently visible). Pull Request: https://projects.blender.org/blender/blender/pulls/118774 --- .../BLI_linear_allocator_chunked_list.hh | 190 ++++++++++++++++++ source/blender/blenlib/CMakeLists.txt | 2 + .../BLI_linear_allocator_chunked_list_test.cc | 102 ++++++++++ .../blender/nodes/NOD_geometry_nodes_log.hh | 15 +- .../nodes/geometry/nodes/node_geo_bake.cc | 2 +- .../intern/geometry_nodes_lazy_function.cc | 7 +- .../nodes/intern/geometry_nodes_log.cc | 5 +- .../nodes/intern/node_geometry_exec.cc | 2 + 8 files changed, 313 insertions(+), 12 deletions(-) create mode 100644 source/blender/blenlib/BLI_linear_allocator_chunked_list.hh create mode 100644 source/blender/blenlib/tests/BLI_linear_allocator_chunked_list_test.cc diff --git a/source/blender/blenlib/BLI_linear_allocator_chunked_list.hh b/source/blender/blenlib/BLI_linear_allocator_chunked_list.hh new file mode 100644 index 00000000000..d982a024efd --- /dev/null +++ b/source/blender/blenlib/BLI_linear_allocator_chunked_list.hh @@ -0,0 +1,190 @@ +/* SPDX-FileCopyrightText: 2024 Blender Authors + * + * SPDX-License-Identifier: GPL-2.0-or-later */ + +#pragma once + +#include + +#include "BLI_linear_allocator.hh" +#include "BLI_struct_equality_utils.hh" +#include "BLI_utility_mixins.hh" + +namespace blender::linear_allocator { + +/** + * The list is a linked list of segments containing multiple elements. The capacity of each segment + * is a template parameter because that removes the need to store it for every segment. + */ +template struct ChunkedListSegment { + /** Pointer to the next segment in the list. */ + ChunkedListSegment *next = nullptr; + /** + * Number of constructed elements in this segment. The constructed elements are always at the + * beginning of the array below. + */ + int64_t size = 0; + /** + * The memory that actually contains the values in the end. The values are constructed and + * destructed by higher level code. + */ + std::array, Capacity> values; +}; + +/** + * This is a special purpose container data structure that can be used to efficiently gather many + * elements into many (small) lists for later retrieval. Insertion order is *not* maintained. + * + * To use this data structure, one has to have a separate #LinearAllocator which is passed to the + * `append` function. This allows the same allocator to be used by many lists. Passing it into the + * append function also removes the need to store the allocator pointer in every list. + * + * It is an improvement over #Vector because it does not require any reallocations. #VectorList + * could also be used to overcome the reallocation issue. + * + * This data structure is also an improvement over #VectorList because: + * - It has a much lower memory footprint when empty. + * - Allows using a #LinearAllocator for all allocations, without storing the pointer to it in + * every vector. + * - It wastes less memory due to over-allocations. + */ +template class ChunkedList : NonCopyable { + private: + using Segment = ChunkedListSegment; + Segment *current_segment_ = nullptr; + + public: + ChunkedList() = default; + + ChunkedList(ChunkedList &&other) + { + current_segment_ = other.current_segment_; + other.current_segment_ = nullptr; + } + + ~ChunkedList() + { + /* This code assumes that the #ChunkedListSegment does not have to be destructed if the + * contained type is trivially destructible. */ + static_assert(std::is_trivially_destructible_v>); + if constexpr (!std::is_trivially_destructible_v) { + for (Segment *segment = current_segment_; segment; segment = segment->next) { + for (const int64_t i : IndexRange(segment->size)) { + T &value = *segment->values[i]; + std::destroy_at(&value); + } + } + } + } + + ChunkedList &operator=(ChunkedList &&other) + { + if (this == &other) { + return *this; + } + std::destroy_at(this); + new (this) ChunkedList(std::move(other)); + return *this; + } + + /** + * Add an element to the list. The insertion order is not maintained. The given allocator is used + * to allocate any extra memory that may be needed. + */ + void append(LinearAllocator<> &allocator, const T &value) + { + this->append_as(allocator, value); + } + + void append(LinearAllocator<> &allocator, T &&value) + { + this->append_as(allocator, std::move(value)); + } + + template void append_as(LinearAllocator<> &allocator, Args &&...args) + { + if (current_segment_ == nullptr || current_segment_->size == SegmentCapacity) { + /* Allocate a new segment if necessary. */ + static_assert(std::is_trivially_destructible_v); + Segment *new_segment = allocator.construct().release(); + new_segment->next = current_segment_; + current_segment_ = new_segment; + } + T *value = &*current_segment_->values[current_segment_->size++]; + new (value) T(std::forward(args)...); + } + + class ConstIterator { + private: + const Segment *segment_ = nullptr; + int64_t index_ = 0; + + public: + ConstIterator(const Segment *segment, int64_t index = 0) : segment_(segment), index_(index) {} + + ConstIterator &operator++() + { + index_++; + if (index_ == segment_->size) { + segment_ = segment_->next; + index_ = 0; + } + return *this; + } + + const T &operator*() const + { + return *segment_->values[index_]; + } + + BLI_STRUCT_EQUALITY_OPERATORS_2(ConstIterator, segment_, index_) + }; + + class MutableIterator { + private: + Segment *segment_ = nullptr; + int64_t index_ = 0; + + public: + MutableIterator(Segment *segment, int64_t index = 0) : segment_(segment), index_(index) {} + + MutableIterator &operator++() + { + index_++; + if (index_ == segment_->size) { + segment_ = segment_->next; + index_ = 0; + } + return *this; + } + + T &operator*() + { + return *segment_->values[index_]; + } + + BLI_STRUCT_EQUALITY_OPERATORS_2(MutableIterator, segment_, index_) + }; + + ConstIterator begin() const + { + return ConstIterator(current_segment_, 0); + } + + ConstIterator end() const + { + return ConstIterator(nullptr, 0); + } + + MutableIterator begin() + { + return MutableIterator(current_segment_, 0); + } + + MutableIterator end() + { + return MutableIterator(nullptr, 0); + } +}; + +} // namespace blender::linear_allocator diff --git a/source/blender/blenlib/CMakeLists.txt b/source/blender/blenlib/CMakeLists.txt index a186ab883af..6026ca611d9 100644 --- a/source/blender/blenlib/CMakeLists.txt +++ b/source/blender/blenlib/CMakeLists.txt @@ -263,6 +263,7 @@ set(SRC BLI_lazy_threading.hh BLI_length_parameterize.hh BLI_linear_allocator.hh + BLI_linear_allocator_chunked_list.hh BLI_link_utils.h BLI_linklist.h BLI_linklist_lockfree.h @@ -521,6 +522,7 @@ if(WITH_GTESTS) tests/BLI_kdtree_test.cc tests/BLI_length_parameterize_test.cc tests/BLI_linear_allocator_test.cc + tests/BLI_linear_allocator_chunked_list_test.cc tests/BLI_linklist_lockfree_test.cc tests/BLI_listbase_test.cc tests/BLI_map_test.cc diff --git a/source/blender/blenlib/tests/BLI_linear_allocator_chunked_list_test.cc b/source/blender/blenlib/tests/BLI_linear_allocator_chunked_list_test.cc new file mode 100644 index 00000000000..dec3337f014 --- /dev/null +++ b/source/blender/blenlib/tests/BLI_linear_allocator_chunked_list_test.cc @@ -0,0 +1,102 @@ +/* SPDX-FileCopyrightText: 2024 Blender Authors + * + * SPDX-License-Identifier: Apache-2.0 */ + +#include "testing/testing.h" + +#include + +#include "BLI_linear_allocator_chunked_list.hh" +#include "BLI_set.hh" + +#include "BLI_strict_flags.h" /* Keep last. */ + +namespace blender::linear_allocator::tests { + +TEST(LinearAllocator_ChunkedList, Append) +{ + LinearAllocator<> allocator; + ChunkedList list; + + list.append(allocator, "1"); + list.append(allocator, "2"); + list.append(allocator, "this_is_an_extra_long_string"); + + Set retrieved_values; + for (const std::string &value : const_cast &>(list)) { + retrieved_values.add(value); + } + EXPECT_EQ(retrieved_values.size(), 3); + EXPECT_TRUE(retrieved_values.contains("1")); + EXPECT_TRUE(retrieved_values.contains("2")); + EXPECT_TRUE(retrieved_values.contains("this_is_an_extra_long_string")); +} + +TEST(LinearAllocator_ChunkedList, AppendMany) +{ + LinearAllocator<> allocator; + ChunkedList list; + + for (const int64_t i : IndexRange(10000)) { + list.append(allocator, int(i)); + } + + Set values; + for (const int value : list) { + values.add(value); + } + + EXPECT_EQ(values.size(), 10000); +} + +TEST(LinearAllocator_ChunkedList, Move) +{ + LinearAllocator<> allocator; + ChunkedList a; + a.append(allocator, 1); + ChunkedList b = std::move(a); + + a.append(allocator, 2); + b.append(allocator, 3); + + { + Set a_values; + for (const int value : a) { + a_values.add(value); + } + Set b_values; + for (const int value : b) { + b_values.add(value); + } + + EXPECT_EQ(a_values.size(), 1); + EXPECT_TRUE(a_values.contains(2)); + + EXPECT_EQ(b_values.size(), 2); + EXPECT_TRUE(b_values.contains(1)); + EXPECT_TRUE(b_values.contains(3)); + } + + a = std::move(b); + /* Want to test self-move. Using std::move twice quiets a compiler warning. */ + a = std::move(std::move(a)); + + { + Set a_values; + for (const int value : a) { + a_values.add(value); + } + Set b_values; + for (const int value : b) { + b_values.add(value); + } + + EXPECT_EQ(a_values.size(), 2); + EXPECT_TRUE(a_values.contains(1)); + EXPECT_TRUE(a_values.contains(3)); + + EXPECT_TRUE(b_values.is_empty()); + } +} + +} // namespace blender::linear_allocator::tests diff --git a/source/blender/nodes/NOD_geometry_nodes_log.hh b/source/blender/nodes/NOD_geometry_nodes_log.hh index b800663e643..16682e3ccb0 100644 --- a/source/blender/nodes/NOD_geometry_nodes_log.hh +++ b/source/blender/nodes/NOD_geometry_nodes_log.hh @@ -33,6 +33,7 @@ #include "BLI_compute_context.hh" #include "BLI_enumerable_thread_specific.hh" #include "BLI_generic_pointer.hh" +#include "BLI_linear_allocator_chunked_list.hh" #include "BLI_multi_value_map.hh" #include "BKE_geometry_set.hh" @@ -207,13 +208,13 @@ class GeoTreeLogger { StringRefNull message; }; - Vector node_warnings; - Vector input_socket_values; - Vector output_socket_values; - Vector node_execution_times; - Vector viewer_node_logs; - Vector used_named_attributes; - Vector debug_messages; + linear_allocator::ChunkedList node_warnings; + linear_allocator::ChunkedList input_socket_values; + linear_allocator::ChunkedList output_socket_values; + linear_allocator::ChunkedList node_execution_times; + linear_allocator::ChunkedList viewer_node_logs; + linear_allocator::ChunkedList used_named_attributes; + linear_allocator::ChunkedList debug_messages; GeoTreeLogger(); ~GeoTreeLogger(); diff --git a/source/blender/nodes/geometry/nodes/node_geo_bake.cc b/source/blender/nodes/geometry/nodes/node_geo_bake.cc index 8cdd8655c0f..a0a3a1dd47f 100644 --- a/source/blender/nodes/geometry/nodes/node_geo_bake.cc +++ b/source/blender/nodes/geometry/nodes/node_geo_bake.cc @@ -217,7 +217,7 @@ class LazyFunctionForBakeNode final : public LazyFunction { user_data)) { tree_logger->node_warnings.append( - {node_.identifier, {NodeWarningType::Error, info->message}}); + *tree_logger->allocator, {node_.identifier, {NodeWarningType::Error, info->message}}); } this->set_default_outputs(params); } diff --git a/source/blender/nodes/intern/geometry_nodes_lazy_function.cc b/source/blender/nodes/intern/geometry_nodes_lazy_function.cc index 87e81387e81..fb88a0d01ee 100644 --- a/source/blender/nodes/intern/geometry_nodes_lazy_function.cc +++ b/source/blender/nodes/intern/geometry_nodes_lazy_function.cc @@ -330,7 +330,8 @@ class LazyFunctionForGeometryNode : public LazyFunction { if (geo_eval_log::GeoTreeLogger *tree_logger = local_user_data.try_get_tree_logger(*user_data)) { - tree_logger->node_execution_times.append({node_.identifier, start_time, end_time}); + tree_logger->node_execution_times.append(*tree_logger->allocator, + {node_.identifier, start_time, end_time}); } } @@ -1706,6 +1707,7 @@ class LazyFunctionForRepeatZone : public LazyFunction { user_data)) { tree_logger->node_warnings.append( + *tree_logger->allocator, {repeat_output_bnode_.identifier, {NodeWarningType::Info, N_("Inspection index is out of range")}}); } @@ -2004,7 +2006,8 @@ class GeometryNodesLazyFunctionLogger : public lf::GraphExecutor::Logger { if (!bsockets.is_empty()) { const bNodeSocket &bsocket = *bsockets[0]; const bNode &bnode = bsocket.owner_node(); - tree_logger->debug_messages.append({bnode.identifier, thread_id_str}); + tree_logger->debug_messages.append(*tree_logger->allocator, + {bnode.identifier, thread_id_str}); return true; } } diff --git a/source/blender/nodes/intern/geometry_nodes_log.cc b/source/blender/nodes/intern/geometry_nodes_log.cc index 67ced8d6a52..17d6ecad780 100644 --- a/source/blender/nodes/intern/geometry_nodes_log.cc +++ b/source/blender/nodes/intern/geometry_nodes_log.cc @@ -167,7 +167,8 @@ void GeoTreeLogger::log_value(const bNode &node, const bNodeSocket &socket, cons auto store_logged_value = [&](destruct_ptr value_log) { auto &socket_values = socket.in_out == SOCK_IN ? this->input_socket_values : this->output_socket_values; - socket_values.append({node.identifier, socket.index(), std::move(value_log)}); + socket_values.append(*this->allocator, + {node.identifier, socket.index(), std::move(value_log)}); }; auto log_generic_value = [&](const CPPType &type, const void *value) { @@ -225,7 +226,7 @@ void GeoTreeLogger::log_viewer_node(const bNode &viewer_node, bke::GeometrySet g destruct_ptr log = this->allocator->construct(); log->geometry = std::move(geometry); log->geometry.ensure_owns_direct_data(); - this->viewer_node_logs.append({viewer_node.identifier, std::move(log)}); + this->viewer_node_logs.append(*this->allocator, {viewer_node.identifier, std::move(log)}); } void GeoTreeLog::ensure_node_warnings() diff --git a/source/blender/nodes/intern/node_geometry_exec.cc b/source/blender/nodes/intern/node_geometry_exec.cc index 488f6287aaa..8dc034e526d 100644 --- a/source/blender/nodes/intern/node_geometry_exec.cc +++ b/source/blender/nodes/intern/node_geometry_exec.cc @@ -22,6 +22,7 @@ void GeoNodeExecParams::error_message_add(const NodeWarningType type, { if (geo_eval_log::GeoTreeLogger *tree_logger = this->get_local_tree_logger()) { tree_logger->node_warnings.append( + *tree_logger->allocator, {node_.identifier, {type, tree_logger->allocator->copy_string(message)}}); } } @@ -31,6 +32,7 @@ void GeoNodeExecParams::used_named_attribute(const StringRef attribute_name, { if (geo_eval_log::GeoTreeLogger *tree_logger = this->get_local_tree_logger()) { tree_logger->used_named_attributes.append( + *tree_logger->allocator, {node_.identifier, tree_logger->allocator->copy_string(attribute_name), usage}); } }