From 4b642c5e0096c1de4dddae9fc1fd27d108da95d6 Mon Sep 17 00:00:00 2001 From: Dion Moult Date: Wed, 16 Sep 2026 09:03:06 +1000 Subject: [PATCH] ifcparse: make deleting and creating instances work on RocksDB-backed files 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 Claude-Session: https://claude.ai/code/session_01HNrXDmR88wKPCYwGE21SyH --- src/ifcparse/entity_instance_data.cpp | 34 ++++++++- src/ifcparse/file.cpp | 103 +++++++++++++++++--------- src/ifcparse/parse.cpp | 34 ++++++--- src/ifcparse/rocksdb_set_view.h | 27 +++++-- src/ifcparse/set_to_map_transformer.h | 14 +++- src/ifcparse/storage.h | 3 + 6 files changed, 161 insertions(+), 54 deletions(-) diff --git a/src/ifcparse/entity_instance_data.cpp b/src/ifcparse/entity_instance_data.cpp index b5d38d4280..35a2337c94 100644 --- a/src/ifcparse/entity_instance_data.cpp +++ b/src/ifcparse/entity_instance_data.cpp @@ -291,8 +291,38 @@ bool attribute_value::isNull() const unsigned int attribute_value::size() const { - // @todo - return array_.storage_ptr->apply_visitor(size_visitor{}, index_); + if (storage_model_ == 0) { + return array_.storage_ptr->apply_visitor(size_visitor{}, index_); + } +#ifdef IFOPSH_WITH_ROCKSDB + else { + // Same answers as size_visitor: element count for aggregates, -1 otherwise. + switch (type()) { + case Argument_EMPTY_AGGREGATE: + case Argument_AGGREGATE_OF_EMPTY_AGGREGATE: + return 0; + case Argument_AGGREGATE_OF_INT: + return (unsigned int)dispatch_get_>(array_, storage_model_, instance_name_, entity_or_type_, index_).size(); + case Argument_AGGREGATE_OF_DOUBLE: + return (unsigned int)dispatch_get_>(array_, storage_model_, instance_name_, entity_or_type_, index_).size(); + case Argument_AGGREGATE_OF_STRING: + return (unsigned int)dispatch_get_>(array_, storage_model_, instance_name_, entity_or_type_, index_).size(); + case Argument_AGGREGATE_OF_BINARY: + return (unsigned int)dispatch_get_>>(array_, storage_model_, instance_name_, entity_or_type_, index_).size(); + case Argument_AGGREGATE_OF_ENTITY_INSTANCE: + return (unsigned int)((std::vector)*this).size(); + case Argument_AGGREGATE_OF_AGGREGATE_OF_INT: + return (unsigned int)dispatch_get_>>(array_, storage_model_, instance_name_, entity_or_type_, index_).size(); + case Argument_AGGREGATE_OF_AGGREGATE_OF_DOUBLE: + return (unsigned int)dispatch_get_>>(array_, storage_model_, instance_name_, entity_or_type_, index_).size(); + case Argument_AGGREGATE_OF_AGGREGATE_OF_ENTITY_INSTANCE: + return (unsigned int)((std::vector>)*this).size(); + default: + return (unsigned int)-1; + } + } +#endif + throw std::logic_error("RocksDB storage is unavailable"); } ifcopenshell::argument_type attribute_value::type() const diff --git a/src/ifcparse/file.cpp b/src/ifcparse/file.cpp index 8c805e52c2..ec06efd103 100644 --- a/src/ifcparse/file.cpp +++ b/src/ifcparse/file.cpp @@ -155,7 +155,15 @@ ifcopenshell::impl::rocks_db_file_storage::rocks_db_file_storage(const std::stri : db(init_db(filepath, readonly)) , file(ffile) , instance_ids_(db.get(), "i|") - , instance_by_name_(&instance_ids_, [this](size_t v) { return assert_existance(v, entityinstance_ref); }) + , instance_by_name_( + &instance_ids_, + [this](size_t v) { return assert_existance(v, entityinstance_ref); }, + [this](size_t v) { + // The instance's keys are gone from the database; drop the + // cached handle so lookups don't keep resolving it. + std::lock_guard lock(instance_cache_mutex_); + instance_cache_.erase((uint32_t)v); + }) , bytype_(db.get(), "t|") , byguid_internal_(db.get(), "g|"), byguid_(&byguid_internal_, [this](size_t v) { return assert_existance(v, entityinstance_ref); }, [](const express::base& v) { return v.identity(); }) @@ -207,22 +215,21 @@ void ifcopenshell::impl::rocks_db_file_storage::process_deletion_inverse(const e auto id = inst.id(); { - // compute next prefix that does not start with v|{id}| + // Delete every record referencing inst: all keys under v|{id}|. The + // prefix with its last byte incremented is the exclusive upper bound + // ('}' follows '|'), so no iterator is needed to find the range end. auto prefix = "v|" + std::to_string(id) + "|"; - auto it = std::unique_ptr(db->NewIterator(rocksdb::ReadOptions())); - it->Seek(prefix); - while (it->Valid()) { - it->Next(); - if (!it->key().starts_with(prefix)) { - break; - } - } + auto upper_bound = prefix; + upper_bound.back() = '}'; rocksdb::WriteBatch batch; - batch.DeleteRange(prefix, it->key()); + batch.DeleteRange(prefix, upper_bound); db->Write(wopts, &batch); } + // Delete the records inst contributed through its own attributes: drop + // its id from the value lists of every instance it references. The values + // are uint32_t ids, as written by the serializer and register_inverse(). // This is based on traversal which needs instances to still be contained in the map. // another option would be to keep byid intact for the remainder of this loop auto entity_attributes = traverse(inst, 1); @@ -233,26 +240,24 @@ void ifcopenshell::impl::rocks_db_file_storage::process_deletion_inverse(const e const unsigned int name = entity_attribute.id(); // Do not update inverses for simple types (which have id()==0 in IfcOpenShell). if (name != 0) { - // Find instances entity -> other - // and update inverses from entity into other + auto prefix = "v|" + std::to_string(name) + "|"; + auto it = std::unique_ptr(db->NewIterator(rocksdb::ReadOptions())); + it->Seek(prefix); + while (it->Valid() && it->key().starts_with(prefix)) { + std::string s = it->value().ToString(); - { - auto prefix = "v|" + std::to_string(name) + "|"; - auto it = std::unique_ptr(db->NewIterator(rocksdb::ReadOptions())); - it->Seek(prefix); - while (it->Valid() && it->key().starts_with(prefix)) { - std::string s = it->value().ToString(); - - // Iterator are snapshotted? So don't get invalidated? - std::vector vals(s.size() / sizeof(size_t)); - memcpy(vals.data(), s.data(), s.size()); - vals.erase(std::find(vals.begin(), vals.end(), (size_t)id)); - s.resize(vals.size() * sizeof(size_t)); + // Iterator are snapshotted? So don't get invalidated? + std::vector vals(s.size() / sizeof(uint32_t)); + memcpy(vals.data(), s.data(), s.size()); + auto removed = std::remove(vals.begin(), vals.end(), (uint32_t)id); + if (removed != vals.end()) { + vals.erase(removed, vals.end()); + s.resize(vals.size() * sizeof(uint32_t)); memcpy(s.data(), vals.data(), s.size()); db->Put(wopts, it->key(), s); - - it->Next(); } + + it->Next(); } } } @@ -314,19 +319,47 @@ ifcopenshell::filetype ifcopenshell::guess_file_type(const std::string& fn) { } express::base ifcopenshell::impl::rocks_db_file_storage::create(const ifcopenshell::declaration* decl, int id) { +#ifndef IFOPSH_WITH_ROCKSDB (void)decl; (void)id; - return express::base{}; - /* - if (decl->as_entity() || decl->as_type_declaration()) { - auto* inst = file->schema()->instantiate(decl, rocks_db_attribute_storage{}); - // @todo maybe this needs to be set to file? In order to have a context (ie. rocksdb::db*) to write to? - inst->file_ = nullptr; - return file->add_entity(inst); + throw exception("RocksDB support not compiled in"); +#else + // Mirrors in_memory_file_storage::create(). The instance's attributes + // live in the database (written by set_attribute_value(), read back on + // access), so the cache only has to keep the identity of the handle + // stable: assert_existance() can reload it from the type record that + // add_type_ref() writes. + uint32_t instance_name; + if (decl->as_entity() != nullptr) { + if (id == -1) { + if (!id_counter_recalculated_) { + file->recalculate_id_counter(); + id_counter_recalculated_ = true; + } + instance_name = file->fresh_id(); + } else { + instance_name = id; + } + } else if (decl->as_type_declaration() != nullptr) { + instance_name = 0; } else { throw std::runtime_error("Requires and entity or type declaration"); } - */ + auto data = ifcopenshell::make_pointer_type(file, decl, instance_name, rocks_db_attribute_storage{}); + { + std::lock_guard lock(instance_cache_mutex_); + if (instance_name) { + instance_cache_.insert({instance_name, data}); + } else { + type_instance_cache_.insert({data->identity(), data}); + } + } + + express::base inst(data); + add_type_ref(inst); + + return inst; +#endif } express::base ifcopenshell::impl::in_memory_file_storage::create(const ifcopenshell::declaration* decl, int id) { diff --git a/src/ifcparse/parse.cpp b/src/ifcparse/parse.cpp index 8ab7fe12e5..94db03a16d 100644 --- a/src/ifcparse/parse.cpp +++ b/src/ifcparse/parse.cpp @@ -2636,16 +2636,32 @@ template void ifcopenshell::impl::in_memory_file_storage::read_from_stream(file_ #endif void file::recalculate_id_counter() { - /* - // @todo - entity_by_id::key_type k = 0; - for (auto& p : byid_) { - if (p.first > k) { - k = p.first; + unsigned int k = 0; + std::visit([&k](auto& x) { + if constexpr (std::is_same_v, impl::in_memory_file_storage>) { + for (const auto& p : x.byid_) { + k = std::max(k, (unsigned int)p.first); + } } - } - max_id_ = (unsigned int)k; - */ +#ifdef IFOPSH_WITH_ROCKSDB + else if constexpr (std::is_same_v, impl::rocks_db_file_storage>) { + // Keys sort as text, so the largest id can't be found by seeking; + // scan the i||_ type records, one per entity instance. + const std::string prefix = "i|"; + auto it = std::unique_ptr(x.db->NewIterator(rocksdb::ReadOptions())); + for (it->Seek(prefix); it->Valid() && it->key().starts_with(prefix); it->Next()) { + const auto key = it->key().ToString(); + if (key.size() > 2 && key.compare(key.size() - 2, 2, "|_") == 0) { + k = std::max(k, (unsigned int)std::stoul(key.substr(2, key.size() - 4))); + } + } + } +#endif + else { + throw std::runtime_error("Storage not initialized"); + } + }, storage_); + max_id_ = k; } class traversal_recorder { diff --git a/src/ifcparse/rocksdb_set_view.h b/src/ifcparse/rocksdb_set_view.h index 13e0f8e66e..2893b1b53c 100644 --- a/src/ifcparse/rocksdb_set_view.h +++ b/src/ifcparse/rocksdb_set_view.h @@ -23,6 +23,7 @@ #ifdef IFOPSH_WITH_ROCKSDB #include #include +#include #endif #include @@ -196,6 +197,27 @@ public: return iterator(); } + // Removes the element: every key under prefix + key + "|". The prefix + // with its last byte incremented is the exclusive upper bound ('}' + // follows '|'). Returns 1 if the element existed, 0 otherwise. + size_t erase(const key_type& key) { +#ifdef IFOPSH_WITH_ROCKSDB + if (find(key) == end()) { + return 0; + } + const std::string lower_bound = prefix_ + key_to_string(key) + "|"; + std::string upper_bound = lower_bound; + upper_bound.back() = '}'; + rocksdb::WriteBatch batch; + batch.DeleteRange(lower_bound, upper_bound); + db_->Write(rocksdb::WriteOptions{}, &batch); + return 1; +#else + static_cast(key); + return 0; +#endif + } + // Read-only find: returns an iterator to the element with the given key if it exists. iterator find(const key_type& key) const { #ifdef IFOPSH_WITH_ROCKSDB @@ -216,11 +238,6 @@ public: return end(); } - size_t erase(const key_type& key) { - static_cast(key); - // @todo - return 0; - } }; #endif diff --git a/src/ifcparse/set_to_map_transformer.h b/src/ifcparse/set_to_map_transformer.h index 3ec284cc34..7a11971b44 100644 --- a/src/ifcparse/set_to_map_transformer.h +++ b/src/ifcparse/set_to_map_transformer.h @@ -36,11 +36,17 @@ public: private: BaseSet* base_map_; Transform transform_; + std::function on_erase_; public: set_to_map_transformer(BaseSet* base_set, Transform transform) : base_map_(base_set), transform_(transform) {} + // on_erase runs after erase(key), whether or not the base set still + // held the key, so state derived from the set can be dropped. + set_to_map_transformer(BaseSet* base_set, Transform transform, std::function on_erase) + : base_map_(base_set), transform_(transform), on_erase_(std::move(on_erase)) {} + class iterator { public: using base_iterator = typename BaseSet::iterator; @@ -107,8 +113,10 @@ public: } size_t erase(const key_type& key) { - static_cast(key); - // @todo - return 0; + const size_t erased = base_map_->erase(key); + if (on_erase_) { + on_erase_(key); + } + return erased; } }; diff --git a/src/ifcparse/storage.h b/src/ifcparse/storage.h index 2322bb449b..656ebf2cf0 100644 --- a/src/ifcparse/storage.h +++ b/src/ifcparse/storage.h @@ -678,6 +678,9 @@ namespace ifcopenshell { typedef std::map entity_by_iden_cache; entity_by_iden_cache instance_cache_, type_instance_cache_; std::mutex instance_cache_mutex_; + // Opening a database doesn't visit every instance, so the file's + // id counter is recalculated on the first create(). + bool id_counter_recalculated_ = false; // @todo all these size_ts should probably be uint32_t for consistency with in-mem storage