mirror of
https://github.com/IfcOpenShell/IfcOpenShell.git
synced 2026-08-06 07:51:47 +00:00
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 57cfd9d1fd)
This commit is contained in:
committed by
Dion Moult
parent
634a7add85
commit
4299968b6e
@@ -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]]]
|
||||
|
||||
|
||||
@@ -0,0 +1,73 @@
|
||||
# IfcOpenShell - IFC toolkit and geometry engine
|
||||
# Copyright (C) 2026 Dion Moult <dion@thinkmoult.com>
|
||||
#
|
||||
# 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 <http://www.gnu.org/licenses/>.
|
||||
|
||||
# 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])
|
||||
Reference in New Issue
Block a user