From f2a3b6fe97b1008640214c8688d17300a40c0618 Mon Sep 17 00:00:00 2001 From: Remco Burema Date: Wed, 20 Apr 2022 20:18:39 +0200 Subject: Revert "Fix occasional crash due to unsafe threading in tree support." This reverts commit 550d807b383104c29776f609fbe55df5ac915142. --- src/TreeModelVolumes.cpp | 7 +------ src/TreeModelVolumes.h | 8 +------- src/TreeSupport.cpp | 5 +++-- 3 files changed, 5 insertions(+), 15 deletions(-) diff --git a/src/TreeModelVolumes.cpp b/src/TreeModelVolumes.cpp index 1c6889ee8..7479d5a82 100644 --- a/src/TreeModelVolumes.cpp +++ b/src/TreeModelVolumes.cpp @@ -104,8 +104,6 @@ const Polygons& TreeModelVolumes::calculateCollision(const RadiusLayerPair& key) collision_areas = collision_areas.unionPolygons(collision_model.offset(radius)); } } - - const std::lock_guard lock(object_mutex_); const auto ret = collision_cache_.insert({key, std::move(collision_areas)}); assert(ret.second); return ret.first->second; @@ -137,8 +135,6 @@ const Polygons& TreeModelVolumes::calculateAvoidance(const RadiusLayerPair& key) } auto avoidance_areas = getAvoidance(radius, layer_idx - 1).offset(-max_move_).smooth(5); avoidance_areas = avoidance_areas.unionPolygons(getCollision(radius, layer_idx)); - - const std::lock_guard lock(object_mutex_); const auto ret = avoidance_cache_.insert({key, std::move(avoidance_areas)}); assert(ret.second); return ret.first->second; @@ -148,9 +144,8 @@ const Polygons& TreeModelVolumes::calculateInternalModel(const RadiusLayerPair& { const auto& radius = key.first; const auto& layer_idx = key.second; - const auto& internal_areas = getAvoidance(radius, layer_idx).difference(getCollision(radius, layer_idx)); - const std::lock_guard lock(object_mutex_); + const auto& internal_areas = getAvoidance(radius, layer_idx).difference(getCollision(radius, layer_idx)); const auto ret = internal_model_cache_.insert({key, internal_areas}); assert(ret.second); return ret.first->second; diff --git a/src/TreeModelVolumes.h b/src/TreeModelVolumes.h index 29d23b0d6..74f2b4cc3 100644 --- a/src/TreeModelVolumes.h +++ b/src/TreeModelVolumes.h @@ -4,7 +4,6 @@ #ifndef TREEMODELVOLUMES_H #define TREEMODELVOLUMES_H -#include #include #include "settings/EnumSettings.h" //To store whether X/Y or Z distance gets priority. @@ -21,7 +20,7 @@ class Settings; /*! * \brief Lazily generates tree guidance volumes. * - * \warning This class blocks on thread access. Use calls to this in threaded blocks sparingly. + * \warning This class is not currently thread-safe and should not be accessed in OpenMP blocks */ class TreeModelVolumes { @@ -196,11 +195,6 @@ private: mutable std::unordered_map collision_cache_; mutable std::unordered_map avoidance_cache_; mutable std::unordered_map internal_model_cache_; - - /*! - * \brief Used to make the class thread-safe. - */ - mutable std::mutex object_mutex_; }; } diff --git a/src/TreeSupport.cpp b/src/TreeSupport.cpp index 278cd3e16..6edfa7f29 100644 --- a/src/TreeSupport.cpp +++ b/src/TreeSupport.cpp @@ -31,9 +31,10 @@ namespace cura { -TreeSupport::TreeSupport(const SliceDataStorage& storage) : - volumes_(storage, Application::getInstance().current_slice->scene.current_mesh_group->settings) +TreeSupport::TreeSupport(const SliceDataStorage& storage) { + const Settings& mesh_group_settings = Application::getInstance().current_slice->scene.current_mesh_group->settings; + volumes_ = TreeModelVolumes(storage, mesh_group_settings); } void TreeSupport::generateSupportAreas(SliceDataStorage& storage) -- cgit v1.2.3 From 615896c97c8a90fff444d0767937ae03bd5e8fd1 Mon Sep 17 00:00:00 2001 From: Remco Burema Date: Wed, 20 Apr 2022 21:01:40 +0200 Subject: Alternate fix for crashes/deadlocks Tree-Support. Other fix could still crash and deadlock. Could have probably made it work, but the critical section using 'volumes_' within the scope of the parallel for can be very small, since the access to the tree envelope cash ('volumes_') occurs twice in quick succession. Much easier to make it its own little critical section and not worry about making that entire class thread-safe. That takes care of the crash. During investigation it was found that, since it had multiple critical sections in the loop, handled by the same mutex, _and_ a seprate atomic that was then accessed _inside_ one of the critical sections (what was I thinking...) it could deadlock or at the very least report the wrong progress (causing that part of the front-end to hang). Fixed that as well. part of CURA-9159 --- src/TreeSupport.cpp | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/TreeSupport.cpp b/src/TreeSupport.cpp index 6edfa7f29..b9ac0421b 100644 --- a/src/TreeSupport.cpp +++ b/src/TreeSupport.cpp @@ -99,8 +99,9 @@ void TreeSupport::drawCircles(SliceDataStorage& storage, const std::vector("support_tree_collision_resolution"); - std::atomic completed = 0; //To track progress in a multi-threaded environment. - std::mutex critical_sections; + size_t completed = 0; //To track progress, should be locked when altered. + std::mutex critical_section_volumes; + std::mutex critical_section_progress; cura::parallel_for(0, contact_nodes.size(), 1, [&](const size_t layer_nr) { Polygons support_layer; @@ -142,8 +143,12 @@ void TreeSupport::drawCircles(SliceDataStorage& storage, const std::vector(std::max(0, static_cast(layer_nr) - static_cast(z_distance_bottom_layers) + 1)); //Layer to test against to create a Z-distance. - support_layer = support_layer.difference(volumes_.getCollision(0, z_collision_layer)); //Subtract the model itself (sample 0 is with 0 diameter but proper X/Y offset). - roof_layer = roof_layer.difference(volumes_.getCollision(0, z_collision_layer)); + { + const std::lock_guard lock(critical_section_volumes); + + support_layer = support_layer.difference(volumes_.getCollision(0, z_collision_layer)); //Subtract the model itself (sample 0 is with 0 diameter but proper X/Y offset). + roof_layer = roof_layer.difference(volumes_.getCollision(0, z_collision_layer)); + } support_layer = support_layer.difference(roof_layer); //We smooth this support as much as possible without altering single circles. So we remove any line less than the side length of those circles. const double diameter_angle_scale_factor_this_layer = static_cast(storage.support.supportLayers.size() - layer_nr - tip_layers) * diameter_angle_scale_factor; //Maximum scale factor. @@ -181,19 +186,14 @@ void TreeSupport::drawCircles(SliceDataStorage& storage, const std::vector critical_section_support_max_layer_nr(critical_sections); + const std::lock_guard lock(critical_section_progress); if (!storage.support.supportLayers[layer_nr].support_infill_parts.empty() || !storage.support.supportLayers[layer_nr].support_roof.empty()) { storage.support.layer_nr_max_filled_layer = std::max(storage.support.layer_nr_max_filled_layer, static_cast(layer_nr)); } - } - - ++completed; - - { - std::lock_guard critical_section_progress(critical_sections); + ++completed; const double progress_contact_nodes = contact_nodes.size() * PROGRESS_WEIGHT_DROPDOWN; const double progress_current = completed * PROGRESS_WEIGHT_AREAS; const double progress_total = completed * PROGRESS_WEIGHT_AREAS; -- cgit v1.2.3