fixed csg tree cache concurrency bug (#356)

Author: pca006132

src/manifold/src/csg_tree.cpp

@@ -223,17 +223,19 @@ CsgOpNode::CsgOpNode() {}
 
 CsgOpNode::CsgOpNode(const std::vector<std::shared_ptr<CsgNode>> &children,
                      OpType op)
-    : impl_(std::make_shared<Impl>()) {
-  impl_->children_ = children;
+    : impl_(Impl{}) {
+  auto impl = impl_.GetGuard();
+  impl->children_ = children;
   SetOp(op);
   // opportunistically flatten the tree without costly evaluation
   GetChildren(false);
 }
 
 CsgOpNode::CsgOpNode(std::vector<std::shared_ptr<CsgNode>> &&children,
                      OpType op)
-    : impl_(std::make_shared<Impl>()) {
-  impl_->children_ = children;
+    : impl_(Impl{}) {
+  auto impl = impl_.GetGuard();
+  impl->children_ = children;
   SetOp(op);
   // opportunistically flatten the tree without costly evaluation
   GetChildren(false);
@@ -243,27 +245,30 @@ std::shared_ptr<CsgNode> CsgOpNode::Transform(const glm::mat4x3 &m) const {
   auto node = std::make_shared<CsgOpNode>();
   node->impl_ = impl_;
   node->transform_ = m * glm::mat4(transform_);
+  node->op_ = op_;
   return node;
 }
 
 std::shared_ptr<CsgLeafNode> CsgOpNode::ToLeafNode() const {
   if (cache_ != nullptr) return cache_;
-  if (impl_->children_.empty()) return nullptr;
   // turn the children into leaf nodes
   GetChildren();
-  auto &children_ = impl_->children_;
+  auto impl = impl_.GetGuard();
+  auto &children_ = impl->children_;
   if (children_.size() > 1) {
-    switch (impl_->op_) {
+    switch (op_) {
       case CsgNodeType::Union:
         BatchUnion();
         break;
       case CsgNodeType::Intersection: {
         std::vector<std::shared_ptr<const Manifold::Impl>> impls;
         for (auto &child : children_) {
-          impls.push_back(std::dynamic_pointer_cast<CsgLeafNode>(child)->GetImpl());
+          impls.push_back(
+              std::dynamic_pointer_cast<CsgLeafNode>(child)->GetImpl());
         }
         children_.clear();
-        children_.push_back(std::make_shared<CsgLeafNode>(BatchBoolean(OpType::Intersect, impls)));
+        children_.push_back(std::make_shared<CsgLeafNode>(
+            BatchBoolean(OpType::Intersect, impls)));
         break;
       };
       case CsgNodeType::Difference: {
@@ -283,6 +288,8 @@ std::shared_ptr<CsgLeafNode> CsgOpNode::ToLeafNode() const {
         // unreachable
         break;
     }
+  } else if (children_.size() == 0) {
+    return nullptr;
   }
   // children_ must contain only one CsgLeafNode now, and its Transform will
   // give CsgLeafNode as well
@@ -345,7 +352,8 @@ void CsgOpNode::BatchUnion() const {
   // If the number of children exceeded this limit, we will operate on chunks
   // with size kMaxUnionSize.
   constexpr int kMaxUnionSize = 1000;
-  auto &children_ = impl_->children_;
+  auto impl = impl_.GetGuard();
+  auto &children_ = impl->children_;
   while (children_.size() > 1) {
     const int start = (children_.size() > kMaxUnionSize)
                           ? (children_.size() - kMaxUnionSize)
@@ -392,7 +400,8 @@ void CsgOpNode::BatchUnion() const {
       }
     }
     children_.erase(children_.begin() + start, children_.end());
-    children_.push_back(std::make_shared<CsgLeafNode>(BatchBoolean(OpType::Add, impls)));
+    children_.push_back(
+        std::make_shared<CsgLeafNode>(BatchBoolean(OpType::Add, impls)));
     // move it to the front as we process from the back, and the newly added
     // child should be quite complicated
     std::swap(children_.front(), children_.back());
@@ -408,18 +417,18 @@ void CsgOpNode::BatchUnion() const {
  */
 std::vector<std::shared_ptr<CsgNode>> &CsgOpNode::GetChildren(
     bool finalize) const {
-  auto &children_ = impl_->children_;
-  if (children_.empty() || (impl_->simplified_ && !finalize) ||
-      impl_->flattened_)
+  auto impl = impl_.GetGuard();
+  auto &children_ = impl->children_;
+  if (children_.empty() || (impl->simplified_ && !finalize) || impl->flattened_)
     return children_;
-  impl_->simplified_ = true;
-  impl_->flattened_ = finalize;
+  impl->simplified_ = true;
+  impl->flattened_ = finalize;
   std::vector<std::shared_ptr<CsgNode>> newChildren;
 
-  CsgNodeType op = impl_->op_;
+  CsgNodeType op = op_;
   for (auto &child : children_) {
     if (child->GetNodeType() == op && child.use_count() == 1 &&
-        std::dynamic_pointer_cast<CsgOpNode>(child)->impl_.use_count() == 1) {
+        std::dynamic_pointer_cast<CsgOpNode>(child)->impl_.UseCount() == 1) {
       auto grandchildren =
           std::dynamic_pointer_cast<CsgOpNode>(child)->GetChildren(finalize);
       int start = children_.size();
@@ -444,13 +453,13 @@ std::vector<std::shared_ptr<CsgNode>> &CsgOpNode::GetChildren(
 void CsgOpNode::SetOp(OpType op) {
   switch (op) {
     case OpType::Add:
-      impl_->op_ = CsgNodeType::Union;
+      op_ = CsgNodeType::Union;
       break;
     case OpType::Subtract:
-      impl_->op_ = CsgNodeType::Difference;
+      op_ = CsgNodeType::Difference;
       break;
     case OpType::Intersect:
-      impl_->op_ = CsgNodeType::Intersection;
+      op_ = CsgNodeType::Intersection;
       break;
   }
 }

src/manifold/src/csg_tree.h

@@ -72,18 +72,18 @@ class CsgOpNode final : public CsgNode {
 
   std::shared_ptr<CsgLeafNode> ToLeafNode() const override;
 
-  CsgNodeType GetNodeType() const override { return impl_->op_; }
+  CsgNodeType GetNodeType() const override { return op_; }
 
   glm::mat4x3 GetTransform() const override;
 
  private:
   struct Impl {
-    CsgNodeType op_;
-    mutable std::vector<std::shared_ptr<CsgNode>> children_;
-    mutable bool simplified_ = false;
-    mutable bool flattened_ = false;
+    std::vector<std::shared_ptr<CsgNode>> children_;
+    bool simplified_ = false;
+    bool flattened_ = false;
   };
-  std::shared_ptr<Impl> impl_ = nullptr;
+  mutable ConcurrentSharedPtr<Impl> impl_ = ConcurrentSharedPtr<Impl>(Impl{});
+  CsgNodeType op_;
   glm::mat4x3 transform_ = glm::mat4x3(1.0f);
   // the following fields are for lazy evaluation, so they are mutable
   mutable std::shared_ptr<CsgLeafNode> cache_ = nullptr;

src/utilities/include/utils.h

@@ -194,11 +194,12 @@ class strided_range {
 template <typename T>
 class ConcurrentSharedPtr {
  public:
-  ConcurrentSharedPtr(T value) : impl(std::make_shared<T>(value)), mutex(std::make_shared<std::mutex>()) {}
-  ConcurrentSharedPtr(const ConcurrentSharedPtr<T>& other) : impl(other.impl), mutex(other.mutex) {}
+  ConcurrentSharedPtr(T value) : impl(std::make_shared<T>(value)) {}
+  ConcurrentSharedPtr(const ConcurrentSharedPtr<T>& other)
+      : impl(other.impl), mutex(other.mutex) {}
   class SharedPtrGuard {
    public:
-    SharedPtrGuard(std::mutex* mutex, T* content)
+    SharedPtrGuard(std::recursive_mutex* mutex, T* content)
         : mutex(mutex), content(content) {
       mutex->lock();
     }
@@ -208,14 +209,16 @@ class ConcurrentSharedPtr {
     T* operator->() { return content; }
 
    private:
-    std::mutex* mutex;
+    std::recursive_mutex* mutex;
     T* content;
   };
   SharedPtrGuard GetGuard() { return SharedPtrGuard(mutex.get(), impl.get()); };
+  unsigned int UseCount() { return impl.use_count(); };
 
  private:
   std::shared_ptr<T> impl;
-  std::shared_ptr<std::mutex> mutex;
+  std::shared_ptr<std::recursive_mutex> mutex =
+      std::make_shared<std::recursive_mutex>();
 };
 /** @} */
 }  // namespace manifold