From 91622b97b3ab4c07dce79ae368f42a0ac51d8049 Mon Sep 17 00:00:00 2001 From: Dion Moult Date: Mon, 14 Sep 2026 07:16:25 +1000 Subject: [PATCH] ifcparse: keep unresolved references in the attribute slots The parser could not resolve a #name when it read it, because the instance may be defined further down the file, so it left the slot empty and appended (owner, attribute, name) to a side table that a second pass walked. The table held one entry per reference for the whole read: 64 MB on a 58 MB model, the high-water mark of opening. Now the reference stays where the tokenizer put it: the attribute slot holds the instance_reference, or the reference_or_simple_type aggregate for a list (mixed with inline typed values or not), until every instance has been read, and resolve_instance_references() walks each instance's slots and swaps names for instances. Ordering is what the tokenizer produced; nothing is re-derived. A missing name becomes null in a scalar and is dropped from an aggregate, as before; the error keeps its offset. The three transient alternatives are appended to the attribute pack and to argument_type in lock step and are never visible once a file is loaded. Simple type instances read inline (IfcPropertySetDefinitionSet) have their own slots, so their references need no diversion. The table remains for the header entities and for streaming consumers of instance_streamer::references(), which leave resolve_references_in_place off. TXG 58 MB / 210_King 147 MB / OKgate22 231 MB, single thread: time unchanged (1.05 / 2.69 / 4.99 s), memory after the parse 287 -> 274, 698 -> 654, 1086 -> 1036 MB, peak 400 -> 365, 965 -> 871, 1471 -> 1347 MB. 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/argument_type.h | 7 ++ src/ifcparse/entity_instance_data.cpp | 3 + src/ifcparse/file.h | 3 + src/ifcparse/instance_data.h | 25 +++- src/ifcparse/parse.cpp | 113 +++++++++++++++--- src/ifcparse/storage.h | 13 ++ .../tests/test_ifcopenshell_parse.cpp | 92 ++++++++++++++ 7 files changed, 240 insertions(+), 16 deletions(-) diff --git a/src/ifcparse/argument_type.h b/src/ifcparse/argument_type.h index 064d49e8e1..3e2647c471 100644 --- a/src/ifcparse/argument_type.h +++ b/src/ifcparse/argument_type.h @@ -49,6 +49,13 @@ enum argument_type { Argument_AGGREGATE_OF_AGGREGATE_OF_DOUBLE, Argument_AGGREGATE_OF_AGGREGATE_OF_ENTITY_INSTANCE, + // Parse-time only: a reference (#name), or an aggregate holding + // references, not resolved to instances yet. Never visible once a file + // is loaded. + Argument_UNRESOLVED_REFERENCE, + Argument_UNRESOLVED_REFERENCE_AGGREGATE, + Argument_UNRESOLVED_REFERENCE_AGGREGATE_OF_AGGREGATE, + Argument_UNKNOWN }; diff --git a/src/ifcparse/entity_instance_data.cpp b/src/ifcparse/entity_instance_data.cpp index b5d38d4280..63f014ee31 100644 --- a/src/ifcparse/entity_instance_data.cpp +++ b/src/ifcparse/entity_instance_data.cpp @@ -21,6 +21,9 @@ public: int operator()(const empty_aggregate& /*unused*/) const { return 0; } int operator()(const empty_aggregate_of_aggregate& /*unused*/) const { return 0; } int operator()(const std::vector& i) const { return (int)i.size(); } + int operator()(const instance_reference& /*i*/) const { return -1; } + int operator()(const std::vector& i) const { return (int)i.size(); } + int operator()(const std::vector>& i) const { return (int)i.size(); } int operator()(const std::vector& i) const { return (int)i.size(); } int operator()(const std::vector>& i) const { return (int)i.size(); } int operator()(const std::vector>& i) const { return (int)i.size(); } diff --git a/src/ifcparse/file.h b/src/ifcparse/file.h index bca196f98a..8da94b4741 100644 --- a/src/ifcparse/file.h +++ b/src/ifcparse/file.h @@ -166,6 +166,9 @@ private: instance_streamer(Reader* stream, ifcopenshell::file* owner_file = nullptr, ifcopenshell::logger& logger = ifcopenshell::logger::root()); void bypass_types(const std::set& type_names); + void resolve_references_in_place(bool value) { + storage_.resolve_references_in_place = value; + } void yield_header_instances(bool enabled) { yield_header_instances_ = enabled; } diff --git a/src/ifcparse/instance_data.h b/src/ifcparse/instance_data.h index b40ad3197f..c507e2fdf2 100644 --- a/src/ifcparse/instance_data.h +++ b/src/ifcparse/instance_data.h @@ -89,6 +89,21 @@ namespace impl { static std::string get() { return "derived"; } }; + template <> + struct variant_type_name { + static std::string get() { return "unresolved reference"; } + }; + + template <> + struct variant_type_name> { + static std::string get() { return "unresolved reference aggregate"; } + }; + + template <> + struct variant_type_name>> { + static std::string get() { return "unresolved reference aggregate of aggregates"; } + }; + template <> struct variant_type_name { static std::string get() { return "int"; } @@ -214,7 +229,15 @@ typedef parameter_pack < // An aggregate of an aggregate of floats. E.g. ((1., 2.3), (4.)) std::vector>, // An aggregate of an aggregate of entities. E.g. ((#1, #2), (#3)) - std::vector>> + std::vector>, + // PARSE-TIME ONLY: a reference, or an aggregate mixing references and + // inline typed values, exactly as the tokenizer produced it, held in + // the slot until every instance has been read and then replaced by the + // three forms above. Never present once a file is loaded. Their indices + // match the Argument_UNRESOLVED_* members of argument_type. + instance_reference, + std::vector, + std::vector>> type_variant_parameter_pack; template diff --git a/src/ifcparse/parse.cpp b/src/ifcparse/parse.cpp index 1c545aa36a..e3d0e5f864 100644 --- a/src/ifcparse/parse.cpp +++ b/src/ifcparse/parse.cpp @@ -953,23 +953,30 @@ void set_direct_attribute( in_memory_attribute_storage& storage, std::optional instance_name, ifcopenshell::unresolved_references* references_to_resolve, + bool resolve_in_place, size_t attribute_index, int resolve_reference_index, const T& value ) { - if constexpr (std::is_same_v, ifcopenshell::reference_or_simple_type>) { - if (instance_name && references_to_resolve) { - references_to_resolve->push_back(std::make_pair( - ifcopenshell::mutable_attribute_value{(uint32_t) *instance_name, resolve_reference_index == -1 ? (uint8_t) attribute_index : (uint8_t) resolve_reference_index}, - value - )); - } - } else if constexpr (std::is_same_v, std::vector>) { - if (instance_name && references_to_resolve) { - references_to_resolve->push_back({{(uint32_t) *instance_name, resolve_reference_index == -1 ? (uint8_t) attribute_index : (uint8_t) resolve_reference_index}, value}); - } - } else if constexpr (std::is_same_v, std::vector>>) { - if (instance_name && references_to_resolve) { + constexpr bool holds_references = + std::is_same_v, ifcopenshell::reference_or_simple_type> || + std::is_same_v, std::vector> || + std::is_same_v, std::vector>>; + if constexpr (holds_references) { + // A diverted reference (resolve_reference_index != -1) belongs to a + // simple type instance nested in an attribute of the owner. In place + // it is written into that instance's own slot, so nothing is diverted. + if (resolve_in_place && instance_name) { + if constexpr (std::is_same_v, ifcopenshell::reference_or_simple_type>) { + if (const auto* reference = std::get_if(&value)) { + storage.set(attribute_index, *reference); + } else { + storage.set(attribute_index, std::get(value)); + } + } else { + storage.set(attribute_index, value); + } + } else if (instance_name && references_to_resolve) { references_to_resolve->push_back({{(uint32_t) *instance_name, resolve_reference_index == -1 ? (uint8_t) attribute_index : (uint8_t) resolve_reference_index}, value}); } } else { @@ -1090,7 +1097,7 @@ shared_pointer_type ifcopenshell::impl::in_memory_file_storage::load( auto aggregate = read_direct_aggregate(*this, tokens, entity_instance_name, entity, reference_attribute_index, aggregate_parameter_type(parameter_type), logger_.get()); std::visit([&](const auto& value) { if constexpr (!std::is_same_v, blank>) { - set_direct_attribute(storage, entity_instance_name, references_to_resolve, attribute_index_within_data, attribute_index, value); + set_direct_attribute(storage, entity_instance_name, references_to_resolve, resolve_references_in_place, attribute_index_within_data, attribute_index, value); } }, aggregate.storage); } else { @@ -1117,7 +1124,7 @@ shared_pointer_type ifcopenshell::impl::in_memory_file_storage::load( } if (retain_value) { dispatch_token_direct(next, declared_type(parameter_type), (int) attribute_index_within_data, logger_.get(), [&](const auto& value) { - set_direct_attribute(storage, entity_instance_name, references_to_resolve, attribute_index_within_data, attribute_index, value); + set_direct_attribute(storage, entity_instance_name, references_to_resolve, resolve_references_in_place, attribute_index_within_data, attribute_index, value); }); } } @@ -2451,6 +2458,71 @@ std::optional>; +void ifcopenshell::impl::in_memory_file_storage::resolve_instance_references(const shared_pointer_type& data, const std::vector& bypassed) { + if (!data->storage_) { + return; + } + auto& slots = *data->storage_; + const uint32_t owner = data->id(); + // Looks the name up; false, with the error logged, if it is missing. + // A bypassed name is dropped silently, as the reference table does. + const auto resolve = [&](const instance_reference& reference, size_t attribute_index, express::base& result) { + if (std::binary_search(bypassed.begin(), bypassed.end(), (unsigned)reference.v)) { + return false; + } + auto it = byid_.find((uint32_t)reference.v); + if (it == byid_.end()) { + logger_.get().error("Instance reference #" + std::to_string(reference.v) + " used by instance #" + std::to_string(owner) + " at attribute index " + std::to_string(attribute_index) + " not found at offset " + std::to_string(reference.file_offset)); + return false; + } + result = express::base(it->second); + return true; + }; + // An aggregate element: the instance it names, or the inline typed + // value it already is. + const auto resolve_element = [&](const reference_or_simple_type& element, size_t attribute_index, std::vector& into) { + if (const auto* reference = std::get_if(&element)) { + express::base instance; + if (resolve(*reference, attribute_index, instance)) { + into.push_back(instance); + } + } else { + into.push_back(std::get(element)); + } + }; + for (size_t i = 0; i < slots.size(); ++i) { + if (slots.template has(i)) { + express::base instance; + if (resolve(slots.template get(i), i, instance)) { + slots.set(i, instance); + } else { + slots.set(i, blank{}); + } + } else if (slots.template has>(i)) { + const auto elements = std::move(slots.template get>(i)); + std::vector instances; + instances.reserve(elements.size()); + for (const auto& element : elements) { + resolve_element(element, i, instances); + } + slots.set(i, std::move(instances)); + } else if (slots.template has>>(i)) { + const auto nested_elements = std::move(slots.template get>>(i)); + std::vector> nested; + nested.reserve(nested_elements.size()); + for (const auto& elements : nested_elements) { + std::vector instances; + instances.reserve(elements.size()); + for (const auto& element : elements) { + resolve_element(element, i, instances); + } + nested.push_back(std::move(instances)); + } + slots.set(i, std::move(nested)); + } + } +} + template void ifcopenshell::impl::in_memory_file_storage::read_from_stream(Reader* s, const ifcopenshell::schema_definition*& schema, unsigned int& max_id, const std::set& typed_to_bypass) { schema = nullptr; @@ -2497,6 +2569,7 @@ void ifcopenshell::impl::in_memory_file_storage::read_from_stream(Reader* s, con auto ifcroot_type_ = schema->declaration_by_name("IfcRoot"); streamer.bypass_types(typed_to_bypass); + streamer.resolve_references_in_place(true); logger_.get().status("Scanning file..."); @@ -2553,6 +2626,16 @@ void ifcopenshell::impl::in_memory_file_storage::read_from_stream(Reader* s, con const auto& bypassed = streamer.bypassed_instances(); + // The names left in the attribute slots, then those of the simple type + // instances read inline (a select such as IfcPropertySetDefinitionSet). + for (auto it = byid_.begin(); it != byid_.end(); ++it) { + resolve_instance_references(it->second, bypassed); + } + for (const auto& data : read_simple_type_instances) { + resolve_instance_references(data, bypassed); + } + + // What was read with in-place storage off: the header entities. for (const auto& p : streamer.references()) { const auto& ref = p.first.name_; const auto& refattr = p.first.index_; diff --git a/src/ifcparse/storage.h b/src/ifcparse/storage.h index 6e8257b193..7f47de644f 100644 --- a/src/ifcparse/storage.h +++ b/src/ifcparse/storage.h @@ -576,6 +576,14 @@ namespace ifcopenshell { const ifcopenshell::schema_definition* schema; unresolved_references* references_to_resolve = nullptr; + // When set, a reference read into an instance's attribute stays + // in the attribute slot as the instance_reference (or the + // reference_or_simple_type aggregate) the tokenizer produced, + // instead of being copied into references_to_resolve, and + // resolve_instance_references() replaces it with the instance + // once every instance has been read. read_from_stream() turns it + // on; streaming consumers of references() leave it off. + bool resolve_references_in_place = false; typedef std::map> entities_by_type; typedef std::unordered_map entity_instance_by_name_storage; @@ -650,6 +658,11 @@ namespace ifcopenshell { shared_pointer_type load(ifcopenshell::spf_lexer* tokens, std::optional entity_instance_name, const ifcopenshell::declaration* declaration, const ifcopenshell::entity* entity, int attribute_index = -1, bool coerce_attribute_count = true); template void try_read_semicolon(ifcopenshell::spf_lexer* tokens) const; + // Replaces the names left in `data`'s attribute slots by in-place + // reference storage with the instances they name; a name that is + // missing or bypassed becomes null in a scalar and is dropped + // from an aggregate. + void resolve_instance_references(const shared_pointer_type& data, const std::vector& bypassed); void register_inverse(unsigned referenced_id, const ifcopenshell::entity* from_entity, int instance_id, int attribute_index); void unregister_inverse(unsigned referenced_id, const ifcopenshell::entity* from_entity, const express::base& entity, int attribute_index); diff --git a/src/ifcparse/tests/test_ifcopenshell_parse.cpp b/src/ifcparse/tests/test_ifcopenshell_parse.cpp index e055f28a82..1f7a7eeac7 100644 --- a/src/ifcparse/tests/test_ifcopenshell_parse.cpp +++ b/src/ifcparse/tests/test_ifcopenshell_parse.cpp @@ -5,6 +5,8 @@ #include #include #include +#include +#include #include #include #include @@ -342,3 +344,93 @@ TEST_CASE("The index token policy ends every token where the full policy does, w lexer.next(); CHECK(lexer.next().as_string() == "a\xc2\xa7" "b"); } + +namespace { +const char* const reference_resolution_spf = + "ISO-10303-21;\n" + "HEADER;\n" + "FILE_DESCRIPTION(('ViewDefinition [CoordinationView]'),'2;1');\n" + "FILE_NAME('','',(''),(''),'','','');\n" + "FILE_SCHEMA(('IFC4'));\n" + "ENDSEC;\n" + "DATA;\n" + "#1=IFCCARTESIANPOINT((0.,0.,0.));\n" + "#2=IFCCARTESIANPOINT((1.,0.,0.));\n" + "#3=IFCCARTESIANPOINT((0.,1.,0.));\n" + "#4=IFCPOLYLINE((#1,#2,#3));\n" + "#5=IFCTRIMMEDCURVE(#4,(IFCPARAMETERVALUE(0.),#1),(IFCPARAMETERVALUE(1.)),.T.,.PARAMETER.);\n" + "#6=IFCPROPERTYSINGLEVALUE('A',$,IFCLABEL('x'),$);\n" + "#7=IFCPROPERTYSET('0YvctVUKr0kugbFTf53O9L',$,'Pset',$,(#6,#999));\n" + "#8=IFCWALL('1F$7lN9$r5MOA_lpAoNM52',$,$,$,$,$,$,$,$);\n" + "#9=IFCRELDEFINESBYPROPERTIES('2F$7lN9$r5MOA_lpAoNM53',$,$,$,(#8),#7);\n" + "#10=IFCBSPLINESURFACEWITHKNOTS(1,1,((#1,#2),(#3,#999)),.UNSPECIFIED.,.F.,.F.,.U.,(2,2),(2,2),(0.,1.),(0.,1.),.UNSPECIFIED.);\n" + "#11=IFCRELAGGREGATES('3F$7lN9$r5MOA_lpAoNM54',$,$,$,#999,(#8));\n" + "ENDSEC;\n" + "END-ISO-10303-21;\n"; +} + +TEST_CASE("References are resolved in place: scalars, lists, nested lists, mixed selects and missing names", "[ifcparse]") { + std::string data(reference_resolution_spf); + ifcopenshell::file file(data.data(), (int)data.size()); + REQUIRE(file.good()); + + const std::vector points = file.instance_by_id(4).get_attribute_value(0); + REQUIRE(points.size() == 3); + CHECK(points[0].id() == 1); + CHECK(points[2].id() == 3); + + // A select-typed list mixing an inline typed value with a reference. + const std::vector trim1 = file.instance_by_id(5).get_attribute_value(1); + REQUIRE(trim1.size() == 2); + CHECK(trim1[0].declaration().name() == "IfcParameterValue"); + CHECK(trim1[1].id() == 1); + const std::vector trim2 = file.instance_by_id(5).get_attribute_value(2); + REQUIRE(trim2.size() == 1); + CHECK(trim2[0].declaration().name() == "IfcParameterValue"); + + // A missing name is dropped from a list and nulls a scalar. + const std::vector properties = file.instance_by_id(7).get_attribute_value(4); + REQUIRE(properties.size() == 1); + CHECK(properties[0].id() == 6); + CHECK(file.instance_by_id(11).get_attribute_value(4).isNull()); + const std::vector related = file.instance_by_id(11).get_attribute_value(5); + REQUIRE(related.size() == 1); + CHECK(related[0].id() == 8); + + const express::base definition = file.instance_by_id(9).get_attribute_value(5); + REQUIRE(definition); + CHECK(definition.id() == 7); + + const std::vector> control_points = file.instance_by_id(10).get_attribute_value(2); + REQUIRE(control_points.size() == 2); + REQUIRE(control_points[0].size() == 2); + CHECK(control_points[0][1].id() == 2); + REQUIRE(control_points[1].size() == 1); + CHECK(control_points[1][0].id() == 3); + + // Inverses were registered for every reference, resolved or not. + CHECK(file.instances_by_reference(1).size() == 3); + CHECK(file.instances_by_reference(8).size() == 2); +} + +TEST_CASE("References to bypassed instances are dropped from slots and from mixed lists", "[ifcparse]") { + const auto path = std::filesystem::temp_directory_path() / "ifcopenshell_reference_resolution_test.ifc"; + { + std::ofstream out(path); + out << reference_resolution_spf; + } + ifcopenshell::file file(ifcopenshell::uninitialized_tag{}); + file.bypass_type("IfcCartesianPoint"); + REQUIRE(file.initialize(path.string())); + std::filesystem::remove(path); + + const std::vector points = file.instance_by_id(4).get_attribute_value(0); + CHECK(points.empty()); + const std::vector trim1 = file.instance_by_id(5).get_attribute_value(1); + REQUIRE(trim1.size() == 1); + CHECK(trim1[0].declaration().name() == "IfcParameterValue"); + const std::vector> control_points = file.instance_by_id(10).get_attribute_value(2); + REQUIRE(control_points.size() == 2); + CHECK(control_points[0].empty()); + CHECK(control_points[1].empty()); +}