diff --git a/source/blender/blenkernel/BKE_geometry_set.hh b/source/blender/blenkernel/BKE_geometry_set.hh index a70fa8cfb4e..02e2c2b484d 100644 --- a/source/blender/blenkernel/BKE_geometry_set.hh +++ b/source/blender/blenkernel/BKE_geometry_set.hh @@ -38,6 +38,9 @@ class CurvesEditHints; class Instances; } // namespace blender::bke +class GeometryComponent; +using GeometryComponentPtr = blender::ImplicitSharingPtr; + /** * This is the base class for specialized geometry component types. A geometry component uses * implicit sharing to avoid read-only copies. It also integrates with attribute API, which @@ -50,7 +53,7 @@ class GeometryComponent : public blender::ImplicitSharingMixin { public: GeometryComponent(GeometryComponentType type); virtual ~GeometryComponent() = default; - static GeometryComponent *create(GeometryComponentType component_type); + static GeometryComponentPtr create(GeometryComponentType component_type); int attribute_domain_size(eAttrDomain domain) const; @@ -64,6 +67,9 @@ class GeometryComponent : public blender::ImplicitSharingMixin { /* The returned component should be of the same type as the type this is called on. */ virtual GeometryComponent *copy() const = 0; + /** Remove referenced data from the geometry component. */ + virtual void clear() = 0; + /* Direct data is everything except for instances of objects/collections. * If this returns true, the geometry set can be cached and is still valid after e.g. modifier * evaluation ends. Instances can only be valid as long as the data they instance is valid. */ @@ -76,6 +82,7 @@ class GeometryComponent : public blender::ImplicitSharingMixin { private: void delete_self() override; + void delete_data_only() override; }; template @@ -101,7 +108,6 @@ inline constexpr bool is_geometry_component_v = std::is_base_of_v; /* Indexed by #GeometryComponentType. */ std::array components_; @@ -377,7 +383,7 @@ class MeshComponent : public GeometryComponent { ~MeshComponent(); GeometryComponent *copy() const override; - void clear(); + void clear() override; bool has_mesh() const; /** * Clear the component and replace it with the new mesh. @@ -431,7 +437,7 @@ class PointCloudComponent : public GeometryComponent { ~PointCloudComponent(); GeometryComponent *copy() const override; - void clear(); + void clear() override; bool has_pointcloud() const; /** * Clear the component and replace it with the new point cloud. @@ -493,7 +499,7 @@ class CurveComponent : public GeometryComponent { ~CurveComponent(); GeometryComponent *copy() const override; - void clear(); + void clear() override; bool has_curves() const; /** * Clear the component and replace it with the new curve. @@ -534,7 +540,7 @@ class InstancesComponent : public GeometryComponent { ~InstancesComponent(); GeometryComponent *copy() const override; - void clear(); + void clear() override; const blender::bke::Instances *get_for_read() const; blender::bke::Instances *get_for_write(); @@ -568,7 +574,7 @@ class VolumeComponent : public GeometryComponent { ~VolumeComponent(); GeometryComponent *copy() const override; - void clear(); + void clear() override; bool has_volume() const; /** * Clear the component and replace it with the new volume. @@ -621,6 +627,8 @@ class GeometryComponentEditData final : public GeometryComponent { bool owns_direct_data() const final; void ensure_owns_direct_data() final; + void clear() override; + /** * The first node that does topology changing operations on curves should store the curve point * positions it retrieved as input. Without this, information about the deformed positions is diff --git a/source/blender/blenkernel/intern/customdata.cc b/source/blender/blenkernel/intern/customdata.cc index a5477ee4462..d489b79047b 100644 --- a/source/blender/blenkernel/intern/customdata.cc +++ b/source/blender/blenkernel/intern/customdata.cc @@ -2358,21 +2358,30 @@ CustomData CustomData_shallow_copy_remove_non_bmesh_attributes(const CustomData class CustomDataLayerImplicitSharing : public ImplicitSharingInfo { private: const void *data_; - const int totelem_; + int totelem_; const eCustomDataType type_; public: CustomDataLayerImplicitSharing(const void *data, const int totelem, const eCustomDataType type) - : ImplicitSharingInfo(1), data_(data), totelem_(totelem), type_(type) + : ImplicitSharingInfo(), data_(data), totelem_(totelem), type_(type) { } private: void delete_self_with_data() override { - free_layer_data(type_, data_, totelem_); + if (data_ != nullptr) { + free_layer_data(type_, data_, totelem_); + } MEM_delete(this); } + + void delete_data_only() override + { + free_layer_data(type_, data_, totelem_); + data_ = nullptr; + totelem_ = 0; + } }; /** Create a #ImplicitSharingInfo that takes ownership of the data. */ @@ -2395,7 +2404,10 @@ static void ensure_layer_data_is_mutable(CustomDataLayer &layer, const int totel /* Can not be shared without implicit-sharing data. */ return; } - if (layer.sharing_info->is_shared()) { + if (layer.sharing_info->is_mutable()) { + layer.sharing_info->tag_ensured_mutable(); + } + else { const eCustomDataType type = eCustomDataType(layer.type); const void *old_data = layer.data; /* Copy the layer before removing the user because otherwise the data might be freed while diff --git a/source/blender/blenkernel/intern/geometry_component_curves.cc b/source/blender/blenkernel/intern/geometry_component_curves.cc index 62f8e50a3b7..4e5192d6085 100644 --- a/source/blender/blenkernel/intern/geometry_component_curves.cc +++ b/source/blender/blenkernel/intern/geometry_component_curves.cc @@ -41,7 +41,7 @@ GeometryComponent *CurveComponent::copy() const void CurveComponent::clear() { - BLI_assert(this->is_mutable()); + BLI_assert(this->is_mutable() || this->is_expired()); if (curves_ != nullptr) { if (ownership_ == GeometryOwnershipType::Owned) { BKE_id_free(nullptr, curves_); diff --git a/source/blender/blenkernel/intern/geometry_component_edit_data.cc b/source/blender/blenkernel/intern/geometry_component_edit_data.cc index 9a5cd1aa904..9b684ca7615 100644 --- a/source/blender/blenkernel/intern/geometry_component_edit_data.cc +++ b/source/blender/blenkernel/intern/geometry_component_edit_data.cc @@ -29,6 +29,12 @@ void GeometryComponentEditData::ensure_owns_direct_data() /* Nothing to do. */ } +void GeometryComponentEditData::clear() +{ + BLI_assert(this->is_mutable() || this->is_expired()); + curves_edit_hints_.reset(); +} + void GeometryComponentEditData::remember_deformed_curve_positions_if_necessary( GeometrySet &geometry) { diff --git a/source/blender/blenkernel/intern/geometry_component_instances.cc b/source/blender/blenkernel/intern/geometry_component_instances.cc index ce465d67f21..3af5d4696d9 100644 --- a/source/blender/blenkernel/intern/geometry_component_instances.cc +++ b/source/blender/blenkernel/intern/geometry_component_instances.cc @@ -56,7 +56,7 @@ GeometryComponent *InstancesComponent::copy() const void InstancesComponent::clear() { - BLI_assert(this->is_mutable()); + BLI_assert(this->is_mutable() || this->is_expired()); if (ownership_ == GeometryOwnershipType::Owned) { delete instances_; } diff --git a/source/blender/blenkernel/intern/geometry_component_mesh.cc b/source/blender/blenkernel/intern/geometry_component_mesh.cc index b723060eada..dd8ee041e11 100644 --- a/source/blender/blenkernel/intern/geometry_component_mesh.cc +++ b/source/blender/blenkernel/intern/geometry_component_mesh.cc @@ -44,7 +44,7 @@ GeometryComponent *MeshComponent::copy() const void MeshComponent::clear() { - BLI_assert(this->is_mutable()); + BLI_assert(this->is_mutable() || this->is_expired()); if (mesh_ != nullptr) { if (ownership_ == GeometryOwnershipType::Owned) { BKE_id_free(nullptr, mesh_); diff --git a/source/blender/blenkernel/intern/geometry_component_pointcloud.cc b/source/blender/blenkernel/intern/geometry_component_pointcloud.cc index 0e43a83ed31..f9261fdcfce 100644 --- a/source/blender/blenkernel/intern/geometry_component_pointcloud.cc +++ b/source/blender/blenkernel/intern/geometry_component_pointcloud.cc @@ -31,7 +31,7 @@ GeometryComponent *PointCloudComponent::copy() const void PointCloudComponent::clear() { - BLI_assert(this->is_mutable()); + BLI_assert(this->is_mutable() || this->is_expired()); if (pointcloud_ != nullptr) { if (ownership_ == GeometryOwnershipType::Owned) { BKE_id_free(nullptr, pointcloud_); diff --git a/source/blender/blenkernel/intern/geometry_component_volume.cc b/source/blender/blenkernel/intern/geometry_component_volume.cc index 9fdffdeb473..1a73dc269f7 100644 --- a/source/blender/blenkernel/intern/geometry_component_volume.cc +++ b/source/blender/blenkernel/intern/geometry_component_volume.cc @@ -29,7 +29,7 @@ GeometryComponent *VolumeComponent::copy() const void VolumeComponent::clear() { - BLI_assert(this->is_mutable()); + BLI_assert(this->is_mutable() || this->is_expired()); if (volume_ != nullptr) { if (ownership_ == GeometryOwnershipType::Owned) { BKE_id_free(nullptr, volume_); diff --git a/source/blender/blenkernel/intern/geometry_set.cc b/source/blender/blenkernel/intern/geometry_set.cc index 1e6ee8ee3ab..9731223c76f 100644 --- a/source/blender/blenkernel/intern/geometry_set.cc +++ b/source/blender/blenkernel/intern/geometry_set.cc @@ -41,7 +41,7 @@ using blender::bke::Instances; GeometryComponent::GeometryComponent(GeometryComponentType type) : type_(type) {} -GeometryComponent *GeometryComponent::create(GeometryComponentType component_type) +GeometryComponentPtr GeometryComponent::create(GeometryComponentType component_type) { switch (component_type) { case GEO_COMPONENT_TYPE_MESH: @@ -58,7 +58,7 @@ GeometryComponent *GeometryComponent::create(GeometryComponentType component_typ return new GeometryComponentEditData(); } BLI_assert_unreachable(); - return nullptr; + return {}; } int GeometryComponent::attribute_domain_size(const eAttrDomain domain) const @@ -97,6 +97,11 @@ void GeometryComponent::delete_self() delete this; } +void GeometryComponent::delete_data_only() +{ + this->clear(); +} + /** \} */ /* -------------------------------------------------------------------- */ @@ -120,6 +125,7 @@ GeometryComponent &GeometrySet::get_component_for_write(GeometryComponentType co } if (component_ptr->is_mutable()) { /* If the referenced component is already mutable, return it directly. */ + component_ptr->tag_ensured_mutable(); return *component_ptr; } /* If the referenced component is shared, make a copy. The copy is not shared and is @@ -571,7 +577,7 @@ void GeometrySet::gather_attributes_for_propagation( using namespace blender::bke; /* Only needed right now to check if an attribute is built-in on this component type. * TODO: Get rid of the dummy component. */ - const GeometryComponent *dummy_component = GeometryComponent::create(dst_component_type); + const GeometryComponentPtr dummy_component = GeometryComponent::create(dst_component_type); this->attribute_foreach( component_types, include_instances, @@ -611,7 +617,6 @@ void GeometrySet::gather_attributes_for_propagation( }; r_attributes.add_or_modify(attribute_id, add_info, modify_info); }); - delete dummy_component; } static void gather_component_types_recursive(const GeometrySet &geometry_set, diff --git a/source/blender/blenlib/BLI_implicit_sharing.hh b/source/blender/blenlib/BLI_implicit_sharing.hh index c37594e6c49..4b4099b46c2 100644 --- a/source/blender/blenlib/BLI_implicit_sharing.hh +++ b/source/blender/blenlib/BLI_implicit_sharing.hh @@ -37,32 +37,91 @@ namespace blender { */ class ImplicitSharingInfo : NonCopyable, NonMovable { private: - mutable std::atomic users_; + /** + * Number of users that want to own the shared data. This can be in multiple states: + * - 0: The data is expired and likely freed. It must not be accessed anymore. The + * #ImplicitSharingInfo may still be alive when there are weak users. + * - 1: The data is mutable by the single owner. + * - >1: The data is shared and therefore immutable. + */ + mutable std::atomic strong_users_ = 1; + /** + * Number of users that only keep a reference to the `ImplicitSharingInfo` but don't need to own + * the shared data. One additional weak user is added as long as there is at least one strong + * user. Together with the `version_` below this adds an efficient way to detect if data has been + * changed. + */ + mutable std::atomic weak_users_ = 1; + /** + * The data referenced by an #ImplicitSharingInfo can change over time. This version is + * incremented whenever the referenced data is about to be changed. This allows checking if the + * data has been changed between points in time. + */ + mutable std::atomic version_ = 0; public: - ImplicitSharingInfo(const int initial_users) : users_(initial_users) {} - virtual ~ImplicitSharingInfo() { - BLI_assert(this->is_mutable()); + BLI_assert(strong_users_ == 0); + BLI_assert(weak_users_ == 0); } - /** True if there are other const references to the resource, meaning it cannot be modified. */ - bool is_shared() const - { - return users_.load(std::memory_order_relaxed) >= 2; - } - - /** Whether the resource can be modified without a copy because there is only one owner. */ + /** Whether the resource can be modified inplace because there is only one owner. */ bool is_mutable() const { - return !this->is_shared(); + return strong_users_.load(std::memory_order_relaxed) == 1; + } + + /** + * Weak users don't protect the referenced data from being freed. If the data is freed while + * there is still a weak referenced, this returns true. + */ + bool is_expired() const + { + return strong_users_.load(std::memory_order_acquire) == 0; } /** Call when a the data has a new additional owner. */ void add_user() const { - users_.fetch_add(1, std::memory_order_relaxed); + BLI_assert(!this->is_expired()); + strong_users_.fetch_add(1, std::memory_order_relaxed); + } + + /** + * Adding a weak owner prevents the #ImplicitSharingInfo from being freed but not the referenced + * data. + * + * \note Unlike std::shared_ptr a weak user cannot be turned into a strong user. This is + * because some code might change the referenced data assuming that there is only one strong user + * while a new strong user is added by another thread. + */ + void add_weak_user() const + { + weak_users_.fetch_add(1, std::memory_order_relaxed); + } + + /** + * Call this when making sure that the referenced data is mutable, which also implies that it is + * about to be modified. This allows other code to detect whether data has not been changed very + * efficiently. + */ + void tag_ensured_mutable() const + { + BLI_assert(this->is_mutable()); + /* This might not need an atomic increment when the #version method below is only called when + * the code calling it is a strong user of this sharing info. Better be safe and use an atomic + * for now. */ + version_.fetch_add(1, std::memory_order_acq_rel); + } + + /** + * Get a version number that is increased when the data is modified. It can be used to detect if + * data has been changed. + */ + int64_t version() const + { + return version_.load(std::memory_order_acquire); } /** @@ -71,17 +130,51 @@ class ImplicitSharingInfo : NonCopyable, NonMovable { */ void remove_user_and_delete_if_last() const { - const int old_user_count = users_.fetch_sub(1, std::memory_order_acq_rel); + const int old_user_count = strong_users_.fetch_sub(1, std::memory_order_acq_rel); BLI_assert(old_user_count >= 1); const bool was_last_user = old_user_count == 1; if (was_last_user) { + const int old_weak_user_count = weak_users_.load(std::memory_order_acquire); + BLI_assert(old_weak_user_count >= 1); + if (old_weak_user_count == 1) { + /* If the weak user count is 1 it means that there is no actual weak user. The 1 just + * indicates that there was still at least one strong user. */ + weak_users_ = 0; + const_cast(this)->delete_self_with_data(); + } + else { + /* There is still at least one actual weak user, so don't free the sharing info yet. The + * data can be freed though. */ + const_cast(this)->delete_data_only(); + /* Also remove the "fake" weak user that indicated that there was at least one strong + * user.*/ + this->remove_weak_user_and_delete_if_last(); + } + } + } + + /** + * This might just decrement the weak user count or might delete the data. Should be used in + * conjunction with #add_weak_user. + */ + void remove_weak_user_and_delete_if_last() const + { + const int old_weak_user_count = weak_users_.fetch_sub(1, std::memory_order_acq_rel); + BLI_assert(old_weak_user_count >= 1); + const bool was_last_weak_user = old_weak_user_count == 1; + if (was_last_weak_user) { + /* It's possible that the data has been freed before already, but now it is definitely freed + * together with the sharing info. */ const_cast(this)->delete_self_with_data(); } } private: - /** Has to free the #ImplicitSharingInfo and the referenced data. */ + /** Has to free the #ImplicitSharingInfo and the referenced data. The data might have been freed + * before by #delete_data_only already. This case should be handled here. */ virtual void delete_self_with_data() = 0; + /** Can free the referenced data but the #ImplicitSharingInfo still has to be kept alive. */ + virtual void delete_data_only() {} }; /** @@ -89,8 +182,6 @@ class ImplicitSharingInfo : NonCopyable, NonMovable { * class can be used with #ImplicitSharingPtr. */ class ImplicitSharingMixin : public ImplicitSharingInfo { - public: - ImplicitSharingMixin() : ImplicitSharingInfo(1) {} private: void delete_self_with_data() override diff --git a/source/blender/blenlib/intern/implicit_sharing.cc b/source/blender/blenlib/intern/implicit_sharing.cc index bb78756334a..3625e7babae 100644 --- a/source/blender/blenlib/intern/implicit_sharing.cc +++ b/source/blender/blenlib/intern/implicit_sharing.cc @@ -13,7 +13,7 @@ class MEMFreeImplicitSharing : public ImplicitSharingInfo { public: void *data; - MEMFreeImplicitSharing(void *data) : ImplicitSharingInfo(1), data(data) + MEMFreeImplicitSharing(void *data) : data(data) { BLI_assert(data != nullptr); } @@ -44,7 +44,10 @@ void *make_trivial_data_mutable_impl(void *old_data, } BLI_assert(*sharing_info != nullptr); - if ((*sharing_info)->is_shared()) { + if ((*sharing_info)->is_mutable()) { + (*sharing_info)->tag_ensured_mutable(); + } + else { void *new_data = MEM_mallocN_aligned(size, alignment, __func__); memcpy(new_data, old_data, size); (*sharing_info)->remove_user_and_delete_if_last(); @@ -85,6 +88,7 @@ void *resize_trivial_array_impl(void *old_data, * could theoretically give better performance if the data can be reused in place. */ void *new_data = static_cast(MEM_reallocN(old_data, new_size)); info->data = new_data; + (*sharing_info)->tag_ensured_mutable(); return new_data; } } diff --git a/source/blender/blenlib/tests/BLI_implicit_sharing_test.cc b/source/blender/blenlib/tests/BLI_implicit_sharing_test.cc index 93cf01f739d..72d95b67483 100644 --- a/source/blender/blenlib/tests/BLI_implicit_sharing_test.cc +++ b/source/blender/blenlib/tests/BLI_implicit_sharing_test.cc @@ -28,6 +28,11 @@ class SharedDataContainer { public: SharedDataContainer() : data_(MEM_new(__func__)) {} + const ImplicitSharingInfo *sharing_info() const + { + return data_.get(); + } + const ImplicitlySharedData *get_for_read() const { return data_.get(); @@ -39,6 +44,7 @@ class SharedDataContainer { return nullptr; } if (data_->is_mutable()) { + data_->tag_ensured_mutable(); return data_.get(); } data_ = data_->copy(); @@ -80,4 +86,29 @@ TEST(implicit_sharing, CopyOnWriteAccess) EXPECT_EQ(d.get_for_write(), data_b1); } +TEST(implicit_sharing, WeakUser) +{ + SharedDataContainer a; + const ImplicitSharingInfo *sharing_info = a.sharing_info(); + EXPECT_FALSE(sharing_info->is_expired()); + EXPECT_TRUE(sharing_info->is_mutable()); + sharing_info->add_weak_user(); + EXPECT_FALSE(sharing_info->is_expired()); + EXPECT_TRUE(sharing_info->is_mutable()); + a = {}; + EXPECT_TRUE(sharing_info->is_expired()); + sharing_info->remove_weak_user_and_delete_if_last(); +} + +TEST(implicit_sharing, Version) +{ + SharedDataContainer a; + const ImplicitSharingInfo *sharing_info = a.sharing_info(); + const int old_version = sharing_info->version(); + a.get_for_read(); + EXPECT_EQ(old_version, sharing_info->version()); + a.get_for_write(); + EXPECT_LT(old_version, sharing_info->version()); +} + } // namespace blender::tests diff --git a/source/blender/editors/mesh/editmesh_undo.cc b/source/blender/editors/mesh/editmesh_undo.cc index f8e5a4367d6..6f23f9b3dd6 100644 --- a/source/blender/editors/mesh/editmesh_undo.cc +++ b/source/blender/editors/mesh/editmesh_undo.cc @@ -281,7 +281,10 @@ static void um_arraystore_cd_compact(CustomData *cdata, * solution might be to not pass "dynamic" layers (see `layer_type_is_dynamic`) to the * array store at all. */ BLI_assert(layer->sharing_info->is_mutable()); - MEM_delete(layer->sharing_info); + /* Intentionally don't call #MEM_delete, because we want to free the sharing info without + * the data here. In general this would not be allowed because one can't be sure how to + * free the data without the sharing info. */ + MEM_freeN(const_cast(layer->sharing_info)); } MEM_freeN(layer->data); layer->data = nullptr;