From 4299968b6e066b4112401483ad3f7cde7d06e6cc Mon Sep 17 00:00:00 2001 From: Petru Conduraru Date: Fri, 17 Jul 2026 17:06:29 +0300 Subject: [PATCH] Fix #1043. Optimise IfcPatch recipe: avoid redundant recursive get_info The 2020 profiling in issue #1043 found the Optimise recipe's dedup loop spent almost all of its time in entity_instance.get_info(recursive=True): because the topological sort already guarantees every referenced entity is folded before the entity that references it, recomputing each already-folded subtree's canonical value from scratch for every parent that points to it is wasted work. Confirmed this is still exactly the bottleneck in the current codebase, unchanged since 2020 (get_info's recursive path still walks the whole subtree on every call). Applied aothms's suggested fix from the issue thread: canonicalize each entity with a non-recursive get_info, and for referenced entities substitute the already-computed identity of their folded replacement (looked up in instance_mapping) instead of re-expanding the subtree. Also limited the toposort dependency graph to direct references (max_levels=1), since a topological sort only needs direct edges, not the full transitive closure traverse() was computing for every entity. Benchmarked before and after on real IFC test fixtures and a larger synthetic file with heavily shared geometry (thousands of walls sharing a handful of profile/point subtrees, mirroring the sharing pattern described in the issue): - test/input/geometrygym_great_court_roof.ifc (56989 entities): 9.9s -> 1.7s - test/input/acad2010_objects.ifc (16296 entities): 3.7s -> 0.4s - synthetic 120083-entity fixture with heavy geometry sharing: 19.2s -> 3.4s Verified correctness by comparing the full canonical (recursive get_info) multiset of the optimized output between the old and new implementation on all three fixtures: identical results, same fold counts. Added test_Optimise.py covering the core scenario from the issue: entities built from separate, value-identical non-rooted subtrees fold to a shared instance, while entities with distinct values do not. Generated with the assistance of an AI coding tool. (cherry picked from commit 57cfd9d1fd7b34b08a964b26298541cd19feb6aa) --- src/ifcpatch/ifcpatch/recipes/Optimise.py | 33 +++++----- src/ifcpatch/test/test_Optimise.py | 73 +++++++++++++++++++++++ 2 files changed, 92 insertions(+), 14 deletions(-) create mode 100644 src/ifcpatch/test/test_Optimise.py diff --git a/src/ifcpatch/ifcpatch/recipes/Optimise.py b/src/ifcpatch/ifcpatch/recipes/Optimise.py index b0043a7299..dd7e3bc84c 100644 --- a/src/ifcpatch/ifcpatch/recipes/Optimise.py +++ b/src/ifcpatch/ifcpatch/recipes/Optimise.py @@ -36,8 +36,10 @@ class Patcher: can usually be solved through other means. Consult the bonsai Add-on documentation on dealing with large models for more details. - Warning: this optimise recipe is very, very slow. Please consider using - RecycleNonRootedElements instead. + Warning: this optimise recipe is slower than RecycleNonRootedElements, + as it performs a full, transitive fold instead of a single pass. + Consider RecycleNonRootedElements first if a quicker, partial + optimisation is acceptable. Example: @@ -58,27 +60,30 @@ class Patcher: the set of all of its references contained in its attributes. """ for inst in self.file: - yield inst.id(), set(i.id() for i in self.file.traverse(inst)[1:] if i.id()) + yield inst.id(), set(i.id() for i in self.file.traverse(inst, max_levels=1)[1:] if i.id()) instance_mapping = {} - def map_value(v): + def map_value(v, as_key=False): """ - Recursive function which replicates an entity instance, with - its attributes, mapping references to already registered - instances. Indeed, because of the toposort we know that - forward attribute value instances are mapped before the instances - that reference them. + Recursive function which either replicates an entity instance + with its attributes mapped to already registered instances + (as_key=False), or builds a hashable canonical key for it + (as_key=True), reusing already-folded references instead of + re-expanding their attribute subtrees. """ if isinstance(v, (list, tuple)): - # lists are recursively traversed - return type(v)(map(map_value, v)) + return type(v)(map_value(item, as_key=as_key) for item in v) elif isinstance(v, ifcopenshell.entity_instance): if v.id() == 0: # express simple types are not part of the toposort and just copied + if as_key: + return ("__type__", v.is_a(), v[0]) return self.optimized_file.create_entity(v.is_a(), v[0]) - - return instance_mapping[v] + mapped = instance_mapping[v] + if as_key: + return ("__id__", mapped.id()) + return mapped else: # a plain python value can just be returned return v @@ -87,7 +92,7 @@ class Patcher: for id in toposort(dict(generate_instances_and_references())): inst = self.file[id] - info = inst.get_info(include_identifier=False, recursive=True, return_type=frozenset) + info = map_value(inst.get_info(include_identifier=False, recursive=False, return_type=tuple), as_key=True) if info in info_to_id: mapped = instance_mapping[inst] = instance_mapping[self.file[info_to_id[info]]] diff --git a/src/ifcpatch/test/test_Optimise.py b/src/ifcpatch/test/test_Optimise.py new file mode 100644 index 0000000000..e4beaf122d --- /dev/null +++ b/src/ifcpatch/test/test_Optimise.py @@ -0,0 +1,73 @@ +# IfcOpenShell - IFC toolkit and geometry engine +# Copyright (C) 2026 Dion Moult +# +# This file is part of IfcOpenShell. +# +# IfcOpenShell is free software: you can redistribute it and/or modify +# it under the terms of the GNU Lesser General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# IfcOpenShell is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public License +# along with IfcOpenShell. If not, see . + +# This file was generated with the assistance of an AI coding tool. + +import ifcopenshell +import ifcopenshell.guid + +import ifcpatch +import test.bootstrap + + +def add_context(f: ifcopenshell.file) -> ifcopenshell.entity_instance: + origin = f.createIfcAxis2Placement3D(f.createIfcCartesianPoint((0.0, 0.0, 0.0))) + return f.createIfcGeometricRepresentationContext(None, "Model", 3, 1.0e-05, origin, None) + + +def add_wall_with_curve(f: ifcopenshell.file, context, coords) -> ifcopenshell.entity_instance: + wall = f.create_entity("IfcWall", ifcopenshell.guid.new()) + points = [f.createIfcCartesianPoint(c) for c in coords] + polyline = f.createIfcPolyline(points) + rep = f.createIfcShapeRepresentation(context, "Body", "Curve2D", [polyline]) + wall.Representation = f.createIfcProductDefinitionShape(None, None, [rep]) + return wall + + +def curve_of(wall: ifcopenshell.entity_instance) -> ifcopenshell.entity_instance: + return wall.Representation.Representations[0].Items[0] + + +class TestOptimise(test.bootstrap.IFC4): + def test_folding_value_identical_non_rooted_entities(self): + # Two polylines built from separate but value-identical points. + context = add_context(self.file) + coords = [(0.0, 0.0), (1.0, 0.0), (1.0, 1.0), (0.0, 0.0)] + wall1 = add_wall_with_curve(self.file, context, coords) + wall2 = add_wall_with_curve(self.file, context, coords) + assert curve_of(wall1) != curve_of(wall2) + + output = ifcpatch.execute({"file": self.file, "recipe": "Optimise", "arguments": []}) + + assert len(output.by_type("IfcPolyline")) == 1 + + walls_after = output.by_type("IfcWall") + assert len(walls_after) == 2 + assert curve_of(walls_after[0]) == curve_of(walls_after[1]) + + def test_distinct_values_are_not_folded(self): + context = add_context(self.file) + wall1 = add_wall_with_curve(self.file, context, [(0.0, 0.0), (1.0, 0.0)]) + wall2 = add_wall_with_curve(self.file, context, [(0.0, 0.0), (2.0, 0.0)]) + assert curve_of(wall1) != curve_of(wall2) + + output = ifcpatch.execute({"file": self.file, "recipe": "Optimise", "arguments": []}) + + assert len(output.by_type("IfcPolyline")) == 2 + walls_after = output.by_type("IfcWall") + assert curve_of(walls_after[0]) != curve_of(walls_after[1])