ifcparse: two heap allocations per instance instead of four

Each instance was four separate allocations: the instance_data record, the
attribute array object it pointed at, that array's index bytes, and its
slot storage. A 58 MB model made 10.4 million mallocs to load 918k
instances, and massif attributed 99 MB of its 450 MB peak to malloc
bookkeeping alone.

variant_array now allocates the size byte, the per-slot type indices and
the slots as one block, and instance_data holds the array in a
std::optional instead of behind a pointer (an empty optional keeps the
meaning the null pointer had: attribute storage constructed on the fly
from the RocksDB backend). No ownership or lifetime changes; the same
object owns the same data.

Parse, C++ file constructor, on top of the previous commits:
  TXG            58 MB   0.96 -> 0.94 s   steady 310 -> 262 MB   peak 383 -> 335 MB
  210_King      148 MB   2.74 -> 2.63 s   steady 758 -> 630 MB   peak 948 -> 820 MB
  OKgate22      232 MB   3.70 -> 3.61 s   steady 1165 -> 971 MB  peak 1447 -> 1252 MB
mallocs while loading TXG: 10.39M -> 7.56M.

This commit was written by an AI coding tool and has not been verified by
a human.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013wcN7XquTfUi4vsKQ4KchL

src/ifcparse/instance_data.h

@@ -42,6 +42,7 @@
 
 #include <boost/optional.hpp>
 #include <memory>
+#include <optional>
 #include <boost/logic/tribool.hpp>
 #include <boost/dynamic_bitset.hpp>
 
@@ -509,8 +510,8 @@ class IFC_PARSE_API instance_data {
     void populate_derived_();
 
   public:
-      // Since rocks_db_attribute_storage has no members this is not a variant<in_memory, rocks> but in_memory*, where nullptr means a rocks_db_attribute_storage is constructed on the fly given the context from instance data.
+      // Since rocks_db_attribute_storage has no members this is not a variant<in_memory, rocks> but an optional in_memory storage, where an empty optional means a rocks_db_attribute_storage is constructed on the fly given the context from instance data.
-      in_memory_attribute_storage* storage_;
+      std::optional<in_memory_attribute_storage> storage_;
 
       const ifcopenshell::declaration* declaration() const {
           return declaration_;
@@ -529,13 +530,13 @@ class IFC_PARSE_API instance_data {
       }
 
       instance_data(ifcopenshell::file* file, const ifcopenshell::declaration* declaration, uint32_t id, in_memory_attribute_storage&& storage)
-          : file_(file), declaration_(declaration), identity_(counter_++), id_(id), storage_(new in_memory_attribute_storage(std::move(storage)))
+          : file_(file), declaration_(declaration), identity_(counter_++), id_(id), storage_(std::move(storage))
       {
             populate_derived_();
       }
 
       instance_data(ifcopenshell::file* file, const ifcopenshell::declaration* declaration, uint32_t id, rocks_db_attribute_storage&& storage)
-          : file_(file), declaration_(declaration), identity_(counter_++), id_(id), storage_(nullptr)
+          : file_(file), declaration_(declaration), identity_(counter_++), id_(id), storage_(std::nullopt)
       {
           static_cast<void>(storage);
           populate_derived_();
@@ -544,7 +545,7 @@ class IFC_PARSE_API instance_data {
       /*
       // now that there are referenced as shared_ptr there is no move constructor anymore
       instance_data(instance_data&& other) noexcept
-          : file_(other.file_), id_(other.id_), declaration_(other.declaration_), storage_(std::exchange(other.storage_, nullptr))
+          : file_(other.file_), id_(other.id_), declaration_(other.declaration_), storage_(std::move(other.storage_))
       {}
       */
 
@@ -558,15 +559,15 @@ class IFC_PARSE_API instance_data {
       // same
       instance_data& operator=(instance_data&& other) noexcept {
           if (this != &other) {
-              delete storage_;
+              storage_ = std::move(other.storage_);
-              storage_ = std::exchange(other.storage_, nullptr);
+              other.storage_.reset();
           }
           return *this;
       }
       */
 
       ~instance_data() {
-          delete storage_;
+          storage_.reset();
       }
 
     attribute_value get_attribute_value(size_t attribute_index) const;

src/ifcparse/parse.cpp

@@ -3599,7 +3599,7 @@ instance_data::instance_data(const instance_data& data)
 attribute_value instance_data::get_attribute_value(size_t index) const
 {
     if (storage_) {
-        return attribute_value(storage_, (uint8_t)index);
+        return attribute_value(&*storage_, (uint8_t)index);
     } else {
         auto* const storage = std::visit([](auto& m) -> ifcopenshell::impl::rocks_db_file_storage* {
             using U = std::decay_t<decltype(m)>;

src/ifcparse/variant_array.h

@@ -140,9 +140,10 @@ class variant_array {
 public:
     using types_tuple = ::impl::mapped_types<Types...>;
 
+    // The size byte, the per-slot type indices and the slots live in one
+    // allocation: [size][index * size][padding][slot * size].
     variant_array(size_t size)
-        : size_and_indices_(size ? new uint8_t[size + 1] : nullptr)
+        : size_and_indices_(size ? new uint8_t[block_bytes_(size)] : nullptr)
-        , storage_(size ? new storage_type[size] : nullptr)
     {
         if (size) {
             size_and_indices_[0] = (uint8_t)size;
@@ -156,10 +157,8 @@ public:
 
     variant_array(variant_array&& other) noexcept
         : size_and_indices_(other.size_and_indices_)
-        , storage_(other.storage_)
     {
         other.size_and_indices_ = nullptr;
-        other.storage_ = nullptr;
     }
 
     variant_array& operator=(variant_array&& other) noexcept {
@@ -167,10 +166,7 @@ public:
             free_();
 
             size_and_indices_ = other.size_and_indices_;
-            storage_ = other.storage_;
-
             other.size_and_indices_ = nullptr;
-            other.storage_ = nullptr;
         }
         return *this;
     }
@@ -192,9 +188,9 @@ public:
         size_and_indices_[index + 1] = ::impl::TypeIndex_v<u, Types...>;
         using v = typename std::tuple_element<::impl::TypeIndex_v<u, Types...>, ::impl::mapped_types<Types... >>::type;
         if constexpr (::impl::is_unique_ptr<v>::value) {
-            new(&storage_[index]) v(new u(value));
+            new(&slots_()[index]) v(new u(value));
         } else {
-            new(&storage_[index]) u(std::forward<T>(value));
+            new(&slots_()[index]) u(std::forward<T>(value));
         }
     }
 
@@ -223,9 +219,9 @@ public:
         }
         using v = typename std::tuple_element<::impl::TypeIndex_v<T, Types...>, ::impl::mapped_types<Types... >>::type;
         if constexpr (::impl::is_unique_ptr<v>::value) {
-            return **reinterpret_cast<v*>(&storage_[index]);
+            return **reinterpret_cast<v*>(&slots_()[index]);
         } else {
-            return *reinterpret_cast<v*>(&storage_[index]);
+            return *reinterpret_cast<v*>(&slots_()[index]);
         }
     }
 
@@ -251,9 +247,9 @@ public:
         }
         using v = typename std::tuple_element<::impl::TypeIndex_v<T, Types...>, ::impl::mapped_types<Types... >>::type;
         if constexpr (::impl::is_unique_ptr<v>::value) {
-            return **reinterpret_cast<const v*>(&storage_[index]);
+            return **reinterpret_cast<const v*>(&slots_()[index]);
         } else {
-            return *reinterpret_cast<const v*>(&storage_[index]);
+            return *reinterpret_cast<const v*>(&slots_()[index]);
         }
     }
 
@@ -273,9 +269,20 @@ public:
 
 private:
     using storage_type = typename ::impl::make_union_from_tuple<::impl::mapped_types<Types...>>::type;
-
+    static_assert(alignof(storage_type) <= __STDCPP_DEFAULT_NEW_ALIGNMENT__, "slots must fit the alignment new[] guarantees");
     uint8_t* size_and_indices_;
-    storage_type* storage_;
+
+    // The slots follow the index bytes in the same block.
+    storage_type* slots_() const {
+        return reinterpret_cast<storage_type*>(size_and_indices_ + slots_offset_(size_and_indices_[0]));
+    }
+
+    static constexpr size_t slots_offset_(size_t size) {
+        return (size + 1 + alignof(storage_type) - 1) / alignof(storage_type) * alignof(storage_type);
+    }
+    static constexpr size_t block_bytes_(size_t size) {
+        return slots_offset_(size) + size * sizeof(storage_type);
+    }
 
     void destroy_at_index(std::size_t index) {
         destroy_type_at_index(index, std::integral_constant<std::size_t, sizeof...(Types)>{});
@@ -287,7 +294,6 @@ private:
                 destroy_at_index(i);
             }
             delete[] size_and_indices_;
-            delete[] storage_;
         }
     }
 
@@ -296,7 +302,7 @@ private:
         if (size_and_indices_[index + 1] == Index - 1) {
             using t = typename std::tuple_element_t<Index - 1, ::impl::mapped_types<Types...>>;
             if constexpr (!std::is_trivially_destructible<t>::value) {
-                reinterpret_cast<t*>(&storage_[index])->~t();
+                reinterpret_cast<t*>(&slots_()[index])->~t();
             }
             size_and_indices_[index + 1] = sizeof...(Types);
         } else {
@@ -313,9 +319,9 @@ private:
         if (size_and_indices_[index + 1] == Index - 1) {
             using t = typename std::tuple_element_t<Index - 1, ::impl::mapped_types<Types...>>;
             if constexpr (::impl::is_unique_ptr<t>::value) {
-                return visitor(**reinterpret_cast<t*>(&storage_[index]));
+                return visitor(**reinterpret_cast<t*>(&slots_()[index]));
             } else {
-                return visitor(*reinterpret_cast<t*>(&storage_[index]));
+                return visitor(*reinterpret_cast<t*>(&slots_()[index]));
             }
         }
         return apply_visitor_impl(std::forward<Visitor>(visitor), index, std::integral_constant<std::size_t, Index - 1>{});