Review: recalculate_id_counter() scanned every i|<id>|_ record because
ids were written as decimal text, which doesn't sort numerically. Make
every numeric key segment fixed-width 16-digit lowercase hex, produced
and parsed by key_to_string()/key_from_string() in rocksdb_map_adapter.h,
with named builders (rocksdb_key::attribute, header_attribute,
type_record, inverse, inverse_prefix, type_list, upper_bound) that the
storage, entity_instance_data.cpp, read_schema() and the serializer use
instead of assembling "i|" + std::to_string(id) + ... by hand. Keys now
sort by id, an instance's records are contiguous in id order, and the
largest id is the last key under i|, which recalculate_id_counter()
seeks to. The two dormant to_string_fixed_width() helpers are gone.
This changes the on-disk layout; databases converted before this commit
have to be re-converted.
Also review: instance_cache_ eviction went through an on-erase hook on
set_to_map_transformer, a std::function call under a mutex per deleted
instance. Drop the hook; file::remove_entity() and unbatch() call
file::erase_instances_(ids), which on RocksDB is
rocks_db_file_storage::erase_instances(): one WriteBatch of DeleteRanges
and one lock for the whole batch.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HNrXDmR88wKPCYwGE21SyH
file.remove() on a RocksDB-backed file segfaulted. Reducing it turned up
five gaps that each made editing such a file crash or silently do
nothing:
- process_deletion_inverse() decoded the v| inverse-record values as
size_t while the serializer, register_inverse(), unregister_inverse()
and instances_by_reference() use uint32_t, so std::find failed and
vals.erase(end()) was undefined behaviour. It also took the DeleteRange
end from an iterator that is invalid when the instance has no inverse
records. Decode as uint32_t, remove every occurrence guarded on
"found", and derive the range end from the prefix itself.
- attribute_value::size() ignored storage_model_, so every aggregate
assignment on a RocksDB instance threw "Invalid variant index" from
set_attribute_value(). Branch on the storage model like the sibling
accessors and count the deserialized aggregate.
- rocks_db_file_storage::create() was a stub returning an empty handle,
which anything creating an instance then dereferenced. Implement it
after in_memory_file_storage::create().
- max_id_ is only initialised by the in-memory parse, so a RocksDB file
would have handed out ids that overwrite existing instances.
Implement the recalculate_id_counter() stub per backend and run it
once before the first fresh_id() on RocksDB.
- byid_.erase() was a no-op: set_to_map_transformer::erase() and
rocksdb_set_view::erase() were stubs. The deleted instance's attribute
keys and cached handle survived, entity_names() still listed it and
reopening the database threw. Erase deletes every key under the
instance's prefix; the transformer forwards to it and takes an
on-erase hook the storage uses to drop the cached handle.
root.remove_product on the first 200 products of a 61 MB model now
leaves the same surviving ids and inverse counts whether the file was
opened from SPF or converted to RocksDB.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HNrXDmR88wKPCYwGE21SyH
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>
Rename header-scope aliases, enums, and helper types while retaining descriptive names where dropping the suffix would create a collision.
Generated with the assistance of an AI coding tool.
Setting an IfcInteger/IfcTimeStamp typed attribute (e.g. IfcOwnerHistory.CreationDate)
outside the signed 32-bit range corrupted the value instead of raising, since the
Python wrapper's set_attribute_value_py() truncated it with a plain static_cast<int>
before handing it to the C++ storage. Unix timestamps before 1901-12-13 or after
2038-01-19 silently wrapped around (e.g. 3000000000 became -1294967296) rather than
being rejected or stored correctly. Fixes#3058, equivalent to PR #8683 but ported to
this branch's rewritten ifcparse (snake_case files, variant_array/instance_data
storage, SWIG PyObject-based attribute setter) instead of the old IfcEntityInstanceData
sources, which no longer exist here.
The scalar slot of the attribute variant (Argument_INT) becomes int64_t. Integer
aggregates (Argument_AGGREGATE_OF_INT, e.g. CoordIndex) and instance/reference
identifiers stay 32-bit, since neither is the value that overflows here; this narrow
scope is kept on its own technical merits (aggregates and identifiers were never the
source of the bug, and widening them would be a much larger, riskier change for no
benefit) even though aothms said compatibility isn't a concern on this v0.9-track
branch. express::Base::set_attribute_value promotes the schema-generated int to
int64_t at a single choke point, so the generated setters keep compiling unchanged.
The STEP lexer, writer, and SWIG wrapper (set_attribute_value_py, pythonize) are all
widened together, since widening only the Python-facing setter would have silently
wrapped the value on file write instead of raising.
Verified in a build (IFC2X3 and IFC4, BUILD_IFCGEOM off, no kernels): pre-1901,
post-2038, both 32-bit boundaries, and a 9e12 value all round trip exactly both in
memory and through STEP text serialization (write then reopen). A value outside the
64-bit range now raises a clean exception instead of corrupting data. Ordinary
in-range integers and integer aggregates (e.g. IfcTriangulatedFaceSet.CoordIndex) are
unaffected. The existing util/test_attribute.py and test_file.py suites pass
unchanged; test_entity_instance.py has 5 pre-existing failures unrelated to this
change (confirmed identical on an unfixed build of this branch, caused by a missing
get_info_2 binding and _patch_swig_comparisons never being implemented here).
Generated with the assistance of an AI coding tool.
Fixes builds with newer GCC/libstdc++ that no longer provide <cstdint>,
<cstring>, <cfloat>, <memory>, <algorithm> etc. transitively. Also
disambiguates visit<> calls in taxonomy.h with the full namespace and
casts the character value in IfcCharacterDecoder to uint32_t to silence
ambiguous overload warnings.
Fixes builds with newer GCC/libstdc++ that no longer provide <cstdint>,
<cstring>, <cfloat>, <memory>, <algorithm> etc. transitively. Also
disambiguates visit<> calls in taxonomy.h with the full namespace and
casts the character value in IfcCharacterDecoder to uint32_t to silence
ambiguous overload warnings.
Read-only handles reject Flush/CompactRange, so the destructor's status
assertion always fired on shutdown when the streamer's sidecar was
opened with read_only=true. Track the flag and skip the write path; also
guard against a null db when the initial open failed.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Dion Moult