mirror of
https://github.com/IfcOpenShell/IfcOpenShell.git
synced 2026-09-21 14:23:53 +00:00
ifcparse: unregister a deleted instance's inverse records via its attributes (#9467)
process_deletion_inverse() called inverse_index::remove_source(), which
walked every record in the file's inverse index to find the ones whose
source is the deleted instance: O(R) per deletion, the dominant cost of
file.remove() on large files now that the lookup side no longer re-sorts.
The records a deleted instance contributed are exactly the entity
references in its own attributes, so walk those with the same visitor
build_inverses_() uses for registration and remove each record with a
targeted binary search instead. remove_source() has no callers left and
is deleted.
Also use the ordered view of batch_deletion_ids_ (a boost multi_index
that already had one) for the is-this-referencer-also-being-deleted
check in process_deletion_(), which was a linear std::find over the
sequenced view: O(b) per referencing instance made batch deletion of b
instances quadratic.
file.remove on 300 IfcPropertySet of a 155 MB IFC4 model (201k IfcRoot)
drops from 3.15 ms to 0.17 ms per call, batched removal of 2000 from
3.34 ms to 0.17 ms per call, root.remove_product on 100 walls from
332 ms to 131 ms per call.
Claude-Session: https://claude.ai/code/session_01HNrXDmR88wKPCYwGE21SyH
(cherry picked from commit 938442303f)
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
+15
-2
@@ -2940,7 +2940,7 @@ void file::process_deletion_(const express::base& entity) {
|
|||||||
// entity being deleted are not deleted themselves.
|
// entity being deleted are not deleted themselves.
|
||||||
if (!references.empty()) {
|
if (!references.empty()) {
|
||||||
for (auto& related_instance : references) {
|
for (auto& related_instance : references) {
|
||||||
if (std::find(batch_deletion_ids_.begin(), batch_deletion_ids_.end(), related_instance.id()) != batch_deletion_ids_.end()) {
|
if (batch_deletion_ids_.get<1>().count((int)related_instance.id()) != 0) {
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -3021,7 +3021,20 @@ void ifcopenshell::impl::in_memory_file_storage::process_deletion_inverse(const
|
|||||||
|
|
||||||
// Delete inverses into entity
|
// Delete inverses into entity
|
||||||
byref_excl_.erase(id);
|
byref_excl_.erase(id);
|
||||||
byref_excl_.remove_source(id);
|
|
||||||
|
// Delete the records the entity contributed through its own attributes.
|
||||||
|
// Walking the attributes mirrors build_inverses_, so every record with
|
||||||
|
// this source is covered without scanning the whole index for it.
|
||||||
|
const auto* decl = entity.declaration().as_entity();
|
||||||
|
if (decl == nullptr) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
std::function<void(const express::base&, int)> fn = [this, id, decl](const express::base& attr, int idx) {
|
||||||
|
if (attr.declaration().as_entity() != nullptr) {
|
||||||
|
byref_excl_.remove(attr.id(), id, (uint16_t)decl->index_in_schema(), idx);
|
||||||
|
}
|
||||||
|
};
|
||||||
|
apply_individual_instance_visitor(entity).apply(fn);
|
||||||
}
|
}
|
||||||
|
|
||||||
namespace {
|
namespace {
|
||||||
|
|||||||
@@ -444,27 +444,6 @@ namespace ifcopenshell {
|
|||||||
return true;
|
return true;
|
||||||
}
|
}
|
||||||
|
|
||||||
// Nothing indexes records by source, so this walks the whole index.
|
|
||||||
void remove_source(uint32_t source_id) {
|
|
||||||
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.
|
// Finalizes bulk loading. Subsequent add() calls go to the delta.
|
||||||
void sort() const {
|
void sort() const {
|
||||||
if (!sorted_) {
|
if (!sorted_) {
|
||||||
|
|||||||
@@ -165,3 +165,80 @@ TEST_CASE("Inverse lookups stay consistent across interleaved adds, removals and
|
|||||||
}
|
}
|
||||||
CHECK(referencing_ids(target) == std::vector<int>{(int)trimmed.id()});
|
CHECK(referencing_ids(target) == std::vector<int>{(int)trimmed.id()});
|
||||||
}
|
}
|
||||||
|
|
||||||
|
TEST_CASE("Deleting an instance unregisters the records its own attributes contributed", "[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");
|
||||||
|
const auto* trimmed_curve_declaration = file.schema()->declaration_by_name("IfcTrimmedCurve");
|
||||||
|
|
||||||
|
auto target = file.create(point_declaration);
|
||||||
|
auto second = file.create(point_declaration);
|
||||||
|
|
||||||
|
// A reference registered before the first lookup lands in the base tier,
|
||||||
|
// one registered after it in the delta.
|
||||||
|
auto base_referencer = file.create(polyline_declaration);
|
||||||
|
base_referencer.set_attribute_value(0, std::vector<express::base>{target, target});
|
||||||
|
REQUIRE(file.instances_by_reference(target.id()).size() == 2);
|
||||||
|
auto delta_referencer = file.create(polyline_declaration);
|
||||||
|
delta_referencer.set_attribute_value(0, std::vector<express::base>{target, second});
|
||||||
|
REQUIRE(file.instances_by_reference(target.id()).size() == 3);
|
||||||
|
|
||||||
|
// Duplicate references in one aggregate contribute two records; deleting
|
||||||
|
// the source must drop both.
|
||||||
|
file.remove_entity(base_referencer);
|
||||||
|
CHECK(file.instances_by_reference(target.id()).size() == 1);
|
||||||
|
|
||||||
|
// Referencing the same instance through two attributes contributes a
|
||||||
|
// record per attribute; deleting the source must drop them all.
|
||||||
|
auto trimmed = file.create(trimmed_curve_declaration);
|
||||||
|
trimmed.set_attribute_value(1, std::vector<express::base>{second});
|
||||||
|
trimmed.set_attribute_value(2, std::vector<express::base>{second});
|
||||||
|
CHECK(file.instances_by_reference(second.id()).size() == 3);
|
||||||
|
file.remove_entity(trimmed);
|
||||||
|
CHECK(file.instances_by_reference(second.id()).size() == 1);
|
||||||
|
|
||||||
|
// Deleting the target first prunes it out of the source's attribute, so
|
||||||
|
// deleting the source afterwards finds nothing left to unregister.
|
||||||
|
file.remove_entity(target);
|
||||||
|
file.remove_entity(delta_referencer);
|
||||||
|
CHECK(file.instances_by_reference(second.id()).empty());
|
||||||
|
CHECK(file.get_total_inverses(second.id()) == 0);
|
||||||
|
}
|
||||||
|
|
||||||
|
TEST_CASE("Batch deletion prunes surviving referencers and leaves no stale records", "[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 kept_point = file.create(point_declaration);
|
||||||
|
std::vector<express::base> doomed_points;
|
||||||
|
for (int i = 0; i < 50; ++i) {
|
||||||
|
doomed_points.push_back(file.create(point_declaration));
|
||||||
|
}
|
||||||
|
|
||||||
|
// The survivor references every doomed point plus the kept one; a doomed
|
||||||
|
// referencer references the kept point.
|
||||||
|
auto survivor = file.create(polyline_declaration);
|
||||||
|
auto survivor_points = doomed_points;
|
||||||
|
survivor_points.push_back(kept_point);
|
||||||
|
survivor.set_attribute_value(0, survivor_points);
|
||||||
|
auto doomed_referencer = file.create(polyline_declaration);
|
||||||
|
doomed_referencer.set_attribute_value(0, std::vector<express::base>{kept_point});
|
||||||
|
REQUIRE(file.instances_by_reference(kept_point.id()).size() == 2);
|
||||||
|
|
||||||
|
file.batch();
|
||||||
|
for (auto& point : doomed_points) {
|
||||||
|
file.remove_entity(point);
|
||||||
|
}
|
||||||
|
file.remove_entity(doomed_referencer);
|
||||||
|
file.unbatch();
|
||||||
|
|
||||||
|
CHECK((std::vector<express::base>)survivor.get_attribute_value(0) == std::vector<express::base>{kept_point});
|
||||||
|
CHECK(file.instances_by_reference(kept_point.id()).size() == 1);
|
||||||
|
CHECK(file.get_total_inverses(kept_point.id()) == 1);
|
||||||
|
for (auto& point : doomed_points) {
|
||||||
|
CHECK(file.instances_by_reference(point.id()).empty());
|
||||||
|
}
|
||||||
|
CHECK(file.instances_by_reference(doomed_referencer.id()).empty());
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user