From f05b7b84a906e2f9d1dfe8c4610f6e77fefe5f91 Mon Sep 17 00:00:00 2001 From: Dion Moult Date: Wed, 9 Sep 2026 16:39:56 +1000 Subject: [PATCH] ifcparse: stop re-sorting the whole inverse index on every read after a write The in-memory inverse_index was one flat vector with a sorted flag. Every add() cleared the flag and every lookup re-sorted the entire vector, so a loop that creates an instance and then reads an inverse (api.pset.add_pset, for one) cost O(R log R) per iteration on a file with R references: 400 ms per add_pset call on a 155 MB model, against 0.4 ms on v0.8.0. Split the index into two tiers. The flat vector stays as the base: bulk loading appends to it and sort() finalizes it once, as before, and removals tombstone in place. Records added after that go to a delta bucketed by referenced id, and lookups read the base range followed by the bucket. compact() folds the delta back and drops tombstones once either outgrows the live base, so folding is amortised O(1) per mutation. The four in-memory callers in parse.cpp move from an iterator range to for_each()/count(). The map-shaped legacy interface that variant_map needs is kept and materialises from both tiers. add_pset on the 155 MB model: 400 ms -> 0.25 ms per call. Unit test covers interleaved create-then-read, repointing across both tiers, deletion of sources and targets, and tombstoning past the compaction threshold. This commit was written by an AI coding tool and has not been verified by a human. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_013wcN7XquTfUi4vsKQ4KchL --- src/ifcparse/parse.cpp | 36 ++- src/ifcparse/storage.h | 259 ++++++++++++++---- .../tests/test_ifcopenshell_parse.cpp | 69 +++++ 3 files changed, 298 insertions(+), 66 deletions(-) diff --git a/src/ifcparse/parse.cpp b/src/ifcparse/parse.cpp index 52b663c88a..38af6a04e4 100644 --- a/src/ifcparse/parse.cpp +++ b/src/ifcparse/parse.cpp @@ -3091,11 +3091,10 @@ std::vector file::instances_by_reference(int t) { std::vector ret; std::visit([this, t, &ret](auto& x) { if constexpr (std::is_same_v, impl::in_memory_file_storage>) { - auto range = x.byref_excl_.equal_range((uint32_t)t); - ret.reserve(ret.size() + (size_t)std::distance(range.first, range.second)); - for (auto it = range.first; it != range.second; ++it) { - ret.push_back(instance_by_id(it->source_id)); - } + ret.reserve(ret.size() + x.byref_excl_.count((uint32_t)t)); + x.byref_excl_.for_each((uint32_t)t, [this, &ret](const impl::inverse_record& record) { + ret.push_back(instance_by_id(record.source_id)); + }); } #ifdef IFOPSH_WITH_ROCKSDB else if constexpr (std::is_same_v, impl::rocks_db_file_storage>) { @@ -3251,11 +3250,10 @@ std::vector file::get_inverse_indices_by_id(int instance_id) { if constexpr (std::is_same_v, std::monostate>) { } else if constexpr (std::is_same_v, impl::in_memory_file_storage>) { handled = true; - auto range = x.byref_excl_.equal_range((uint32_t)instance_id); - return_value.reserve((size_t)std::distance(range.first, range.second)); - for (auto it = range.first; it != range.second; ++it) { - return_value.push_back(it->attribute_index); - } + return_value.reserve(x.byref_excl_.count((uint32_t)instance_id)); + x.byref_excl_.for_each((uint32_t)instance_id, [&return_value](const impl::inverse_record& record) { + return_value.push_back(record.attribute_index); + }); } else if constexpr (std::is_same_v, impl::rocks_db_file_storage>) { #ifdef IFOPSH_WITH_ROCKSDB // @todo no lower/upper_bounds() implemented yet @@ -3320,13 +3318,12 @@ std::vector file::get_inverse(int instance_id, const ifcopenshe visit_subtypes(type->as_entity(), [&source_types](const ifcopenshell::declaration* ent) { source_types[ent->index_in_schema()] = 1; }); - auto range = x.byref_excl_.equal_range((uint32_t)instance_id); - for (auto it = range.first; it != range.second; ++it) { - if (it->source_entity < source_types.size() && source_types[it->source_entity] && - (attribute_index == -1 || it->attribute_index == attribute_index)) { - return_value.push_back(instance_by_id(it->source_id).template as()); + x.byref_excl_.for_each((uint32_t)instance_id, [this, &source_types, attribute_index, &return_value](const impl::inverse_record& record) { + if (record.source_entity < source_types.size() && source_types[record.source_entity] && + (attribute_index == -1 || record.attribute_index == attribute_index)) { + return_value.push_back(instance_by_id(record.source_id).template as()); } - } + }); } #ifdef IFOPSH_WITH_ROCKSDB else if constexpr (std::is_same_v, impl::rocks_db_file_storage>) { @@ -3366,10 +3363,9 @@ size_t file::get_total_inverses(int instance_id) { std::visit([&counted_ids, instance_id](auto& x) { if constexpr (std::is_same_v, std::monostate>) { } else if constexpr (std::is_same_v, impl::in_memory_file_storage>) { - auto range = x.byref_excl_.equal_range((uint32_t)instance_id); - for (auto it = range.first; it != range.second; ++it) { - counted_ids.insert(it->source_id); - } + x.byref_excl_.for_each((uint32_t)instance_id, [&counted_ids](const impl::inverse_record& record) { + counted_ids.insert(record.source_id); + }); } else if constexpr (std::is_same_v, impl::rocks_db_file_storage>) { // @todo } diff --git a/src/ifcparse/storage.h b/src/ifcparse/storage.h index 975cef540a..1ff6b9de9c 100644 --- a/src/ifcparse/storage.h +++ b/src/ifcparse/storage.h @@ -31,6 +31,7 @@ namespace rocksdb { #include #include #include +#include #include #include #include @@ -218,6 +219,29 @@ namespace ifcopenshell { int16_t attribute_index; }; + // Which instances reference a given instance, and through which + // attribute. One index serves the whole file. + // + // Two tiers keep every operation cheap without giving up the compact + // flat layout that parsing relies on: + // + // - base_: one flat vector. Bulk loading appends to it unsorted and + // sort() finalizes it once; lookups then binary-search it. Removing + // a record tombstones it in place (attribute_index set to + // dead_attribute) rather than erasing, so removal doesn't shift the + // vector. + // - delta_: records added after sort(), bucketed by referenced_id. + // A lookup reads the base range and then the bucket. + // + // compact() folds the delta into the base and drops tombstones. add() + // and the removal methods run it once the delta or the tombstones + // outgrow the live base (capped by delta_fold_limit), so folding is + // amortised O(1) per mutation and the delta's memory stays bounded. + // + // Before the split every lookup re-sorted the entire vector if + // anything had been added since the previous lookup, so a loop that + // creates an instance and then reads an inverse cost O(R log R) per + // iteration on a file with R references. class inverse_index { public: typedef std::map, std::vector> legacy_bucket; @@ -227,11 +251,21 @@ namespace ifcopenshell { typedef legacy_map::value_type value_type; typedef legacy_map::iterator iterator; typedef legacy_map::const_iterator const_iterator; - typedef std::vector::const_iterator record_iterator; private: - mutable std::vector records_; - mutable bool sorted_ = true; + typedef std::vector::const_iterator base_iterator; + + // Attribute indices are small and non-negative, so the minimum + // value can't collide with a live record. + static constexpr int16_t dead_attribute = std::numeric_limits::min(); + static constexpr size_t delta_fold_limit = size_t(1) << 20; + + // Lookups on a const index still need to finalize bulk loading. + mutable std::vector base_; + mutable bool sorted_ = false; + size_t dead_ = 0; + std::unordered_map> delta_; + size_t delta_size_ = 0; mutable std::unique_ptr materialized_; static bool record_less(const inverse_record& a, const inverse_record& b) { @@ -247,12 +281,67 @@ namespace ifcopenshell { return a.source_id < b.source_id; } - static bool referenced_less(const inverse_record& a, uint32_t referenced_id) { - return a.referenced_id < referenced_id; + struct referenced_id_less { + bool operator()(const inverse_record& a, uint32_t referenced_id) const { + return a.referenced_id < referenced_id; + } + bool operator()(uint32_t referenced_id, const inverse_record& a) const { + return referenced_id < a.referenced_id; + } + }; + + static bool same_record(const inverse_record& a, const inverse_record& b) { + return a.referenced_id == b.referenced_id && + a.source_id == b.source_id && + a.source_entity == b.source_entity && + a.attribute_index == b.attribute_index; } - static bool referenced_less(uint32_t referenced_id, const inverse_record& a) { - return referenced_id < a.referenced_id; + static bool is_dead(const inverse_record& record) { + return record.attribute_index == dead_attribute; + } + + void kill(inverse_record& record) { + record.attribute_index = dead_attribute; + ++dead_; + } + + size_t live_base_size() const { + return base_.size() - dead_; + } + + std::pair base_range(uint32_t referenced_id) const { + sort(); + return std::equal_range(base_.cbegin(), base_.cend(), referenced_id, referenced_id_less{}); + } + + std::pair::iterator, std::vector::iterator> mutable_base_range(uint32_t referenced_id) { + sort(); + return std::equal_range(base_.begin(), base_.end(), referenced_id, referenced_id_less{}); + } + + void compact() { + sort(); + if (dead_ != 0) { + base_.erase(std::remove_if(base_.begin(), base_.end(), is_dead), base_.end()); + dead_ = 0; + } + const auto base_end = (std::ptrdiff_t)base_.size(); + base_.reserve(base_.size() + delta_size_); + for (const auto& bucket : delta_) { + base_.insert(base_.end(), bucket.second.begin(), bucket.second.end()); + } + delta_.clear(); + delta_size_ = 0; + std::sort(base_.begin() + base_end, base_.end(), record_less); + std::inplace_merge(base_.begin(), base_.begin() + base_end, base_.end(), record_less); + invalidate_materialized(); + } + + void compact_if_tombstones_dominate() { + if (dead_ > live_base_size()) { + compact(); + } } void invalidate_materialized() const { @@ -262,9 +351,20 @@ namespace ifcopenshell { legacy_map& materialize() const { if (!materialized_) { materialized_ = std::make_unique(); - materialized_->reserve(records_.size()); - for (const auto& record : records_) { + materialized_->reserve(size()); + const auto insert = [this](const inverse_record& record) { (*materialized_)[(int)record.referenced_id][{(short)record.source_entity, (short)record.attribute_index}].push_back(record.source_id); + }; + sort(); + for (const auto& record : base_) { + if (!is_dead(record)) { + insert(record); + } + } + for (const auto& bucket : delta_) { + for (const auto& record : bucket.second) { + insert(record); + } } } return *materialized_; @@ -274,14 +374,20 @@ namespace ifcopenshell { inverse_index() = default; inverse_index(const inverse_index& other) - : records_(other.records_) + : base_(other.base_) , sorted_(other.sorted_) + , dead_(other.dead_) + , delta_(other.delta_) + , delta_size_(other.delta_size_) {} inverse_index& operator=(const inverse_index& other) { if (this != &other) { - records_ = other.records_; + base_ = other.base_; sorted_ = other.sorted_; + dead_ = other.dead_; + delta_ = other.delta_; + delta_size_ = other.delta_size_; materialized_.reset(); } return *this; @@ -291,73 +397,121 @@ namespace ifcopenshell { inverse_index& operator=(inverse_index&&) noexcept = default; void reserve(size_t size) { - records_.reserve(size); + base_.reserve(size); } void add(uint32_t referenced_id, uint32_t source_id, uint16_t source_entity, int attribute_index) { - records_.push_back({referenced_id, source_id, source_entity, (int16_t)attribute_index}); - sorted_ = false; + const inverse_record record{referenced_id, source_id, source_entity, (int16_t)attribute_index}; + if (sorted_) { + delta_[referenced_id].push_back(record); + ++delta_size_; + if (delta_size_ > std::min(live_base_size(), delta_fold_limit)) { + compact(); + } + } else { + base_.push_back(record); + } invalidate_materialized(); } bool remove(uint32_t referenced_id, uint32_t source_id, uint16_t source_entity, int attribute_index) { const inverse_record needle{referenced_id, source_id, source_entity, (int16_t)attribute_index}; - auto it = std::find_if(records_.begin(), records_.end(), [&needle](const inverse_record& record) { - return record.referenced_id == needle.referenced_id && - record.source_id == needle.source_id && - record.source_entity == needle.source_entity && - record.attribute_index == needle.attribute_index; - }); - if (it == records_.end()) { + const auto matches = [&needle](const inverse_record& record) { + return same_record(record, needle); + }; + auto bucket = delta_.find(referenced_id); + if (bucket != delta_.end()) { + auto& records = bucket->second; + auto it = std::find_if(records.begin(), records.end(), matches); + if (it != records.end()) { + records.erase(it); + --delta_size_; + if (records.empty()) { + delta_.erase(bucket); + } + invalidate_materialized(); + return true; + } + } + auto range = mutable_base_range(referenced_id); + auto it = std::find_if(range.first, range.second, matches); + if (it == range.second) { return false; } - records_.erase(it); + kill(*it); + compact_if_tombstones_dominate(); invalidate_materialized(); return true; } + // Nothing indexes records by source, so this walks the whole index. void remove_source(uint32_t source_id) { - records_.erase(std::remove_if(records_.begin(), records_.end(), [source_id](const inverse_record& record) { - return record.source_id == source_id; - }), records_.end()); + sort(); + for (auto& record : base_) { + if (!is_dead(record) && record.source_id == source_id) { + kill(record); + } + } + for (auto it = delta_.begin(); it != delta_.end();) { + auto& records = it->second; + auto removed = std::remove_if(records.begin(), records.end(), [source_id](const inverse_record& record) { + return record.source_id == source_id; + }); + delta_size_ -= (size_t)std::distance(removed, records.end()); + records.erase(removed, records.end()); + it = records.empty() ? delta_.erase(it) : std::next(it); + } + compact_if_tombstones_dominate(); invalidate_materialized(); } + // Finalizes bulk loading. Subsequent add() calls go to the delta. void sort() const { if (!sorted_) { - std::sort(records_.begin(), records_.end(), record_less); + std::sort(base_.begin(), base_.end(), record_less); sorted_ = true; invalidate_materialized(); } } - std::pair equal_range(uint32_t referenced_id) const { - sort(); - return std::equal_range(records_.begin(), records_.end(), referenced_id, [](const auto& a, const auto& b) { - if constexpr (std::is_same_v, inverse_record>) { - return referenced_less(a, b); - } else { - return referenced_less(a, b); + // Visits every live record referencing referenced_id: the base + // records in record_less order, then the delta in insertion order. + template + void for_each(uint32_t referenced_id, Fn&& fn) const { + auto range = base_range(referenced_id); + for (auto it = range.first; it != range.second; ++it) { + if (!is_dead(*it)) { + fn(*it); } - }); + } + auto bucket = delta_.find(referenced_id); + if (bucket != delta_.end()) { + for (const auto& record : bucket->second) { + fn(record); + } + } } - const std::vector& records() const { - sort(); - return records_; + size_t count(uint32_t referenced_id) const { + size_t n = 0; + for_each(referenced_id, [&n](const inverse_record&) { ++n; }); + return n; } bool empty() const { - return records_.empty(); + return size() == 0; } size_t size() const { - return records_.size(); + return live_base_size() + delta_size_; } void clear() { - records_.clear(); - sorted_ = true; + base_.clear(); + sorted_ = false; + dead_ = 0; + delta_.clear(); + delta_size_ = 0; materialized_.reset(); } @@ -385,13 +539,26 @@ namespace ifcopenshell { return materialize().find(key); } + // Removes every record referencing key. size_t erase(const key_type& key) { - const auto old_size = records_.size(); - records_.erase(std::remove_if(records_.begin(), records_.end(), [key](const inverse_record& record) { - return record.referenced_id == (uint32_t)key; - }), records_.end()); + const auto referenced_id = (uint32_t)key; + size_t removed = 0; + auto range = mutable_base_range(referenced_id); + for (auto it = range.first; it != range.second; ++it) { + if (!is_dead(*it)) { + kill(*it); + ++removed; + } + } + auto bucket = delta_.find(referenced_id); + if (bucket != delta_.end()) { + removed += bucket->second.size(); + delta_size_ -= bucket->second.size(); + delta_.erase(bucket); + } + compact_if_tombstones_dominate(); invalidate_materialized(); - return old_size - records_.size(); + return removed; } std::pair insert(const value_type& value) { diff --git a/src/ifcparse/tests/test_ifcopenshell_parse.cpp b/src/ifcparse/tests/test_ifcopenshell_parse.cpp index 4fe391b41e..6e90ed270a 100644 --- a/src/ifcparse/tests/test_ifcopenshell_parse.cpp +++ b/src/ifcparse/tests/test_ifcopenshell_parse.cpp @@ -96,3 +96,72 @@ TEST_CASE("Aggregate inverse updates preserve reference multiplicity", "[ifcpars CHECK(inverse_count(segment_c) == 2); CHECK(inverse_count(segment_d) == 1); } + +TEST_CASE("Inverse lookups stay consistent across interleaved adds, removals and reads", "[ifcparse]") { + ifcopenshell::file file(ifcopenshell::schema_by_name("IFC4")); + const auto* point_declaration = file.schema()->declaration_by_name("IfcCartesianPoint"); + const auto* polyline_declaration = file.schema()->declaration_by_name("IfcPolyline"); + auto target = file.create(point_declaration); + auto other = file.create(point_declaration); + + const auto referencing_ids = [&file](const express::base& instance) { + std::vector ids; + for (const auto& referencing : file.instances_by_reference(instance.id())) { + ids.push_back(referencing.id()); + } + std::sort(ids.begin(), ids.end()); + return ids; + }; + + // Reading an inverse after every write is the pattern that used to + // re-sort the whole index per iteration. Enough iterations to fold the + // delta into the base several times over. + std::vector polylines; + std::vector expected; + for (int i = 0; i < 600; ++i) { + auto polyline = file.create(polyline_declaration); + polyline.set_attribute_value(0, std::vector{target}); + polylines.push_back(polyline); + expected.push_back(polyline.id()); + REQUIRE(file.instances_by_reference(target.id()).size() == (size_t)i + 1); + } + CHECK(referencing_ids(target) == expected); + CHECK(file.get_inverse_indices_by_id(target.id()) == std::vector(600, 0)); + CHECK(file.get_total_inverses(target.id()) == 600); + + // Repointing an attribute removes the old record and adds a new one, + // whether the record lives in the base or in the delta. + for (int i = 0; i < 600; i += 7) { + polylines[i].set_attribute_value(0, std::vector{other}); + expected.erase(std::find(expected.begin(), expected.end(), polylines[i].id())); + } + CHECK(referencing_ids(target) == expected); + CHECK(file.instances_by_reference(other.id()).size() == 86); + + // The same source referencing the target through two attributes yields two records. + const auto* trimmed_curve_declaration = file.schema()->declaration_by_name("IfcTrimmedCurve"); + auto trimmed = file.create(trimmed_curve_declaration); + trimmed.set_attribute_value(1, std::vector{target}); + trimmed.set_attribute_value(2, std::vector{target}); + CHECK(file.instances_by_reference(target.id()).size() == expected.size() + 2); + CHECK(file.get_total_inverses(target.id()) == expected.size() + 1); + trimmed.set_attribute_value(2, std::vector{other}); + expected.push_back((int)trimmed.id()); + CHECK(referencing_ids(target) == expected); + + // Deleting a referencing instance drops its records; deleting the + // target drops the records into it. + file.remove_entity(polylines[1]); + expected.erase(std::find(expected.begin(), expected.end(), polylines[1].id())); + CHECK(referencing_ids(target) == expected); + file.remove_entity(other); + CHECK(file.instances_by_reference(other.id()).empty()); + + // Removing most of the base tombstones it past the compaction threshold. + for (int i = 2; i < 600; ++i) { + if (i % 7 != 0) { + file.remove_entity(polylines[i]); + } + } + CHECK(referencing_ids(target) == std::vector{(int)trimmed.id()}); +}