Merge topic 'arrayhandlevirtual-deadlock'

75cb53d3a Reset ArrayPortalToken when Detach is called
53c17a687 Release locks in ArrayHandleVirtual control portals

Acked-by: Kitware Robot <kwrobot@kitware.com>
Acked-by: Matt Larsen <larsen30@llnl.gov>
Merge-request: !1982

vtkm/cont/StorageVirtual.cxx

@@ -22,16 +22,14 @@ namespace detail
 
 //--------------------------------------------------------------------
 StorageVirtual::StorageVirtual(const StorageVirtual& src)
-  : HostUpToDate(src.HostUpToDate)
-  , DeviceUpToDate(src.DeviceUpToDate)
+  : DeviceUpToDate(src.DeviceUpToDate)
   , DeviceTransferState(src.DeviceTransferState)
 {
 }
 
 //--------------------------------------------------------------------
 StorageVirtual::StorageVirtual(StorageVirtual&& src) noexcept
-  : HostUpToDate(src.HostUpToDate),
-    DeviceUpToDate(src.DeviceUpToDate),
+  : DeviceUpToDate(src.DeviceUpToDate),
     DeviceTransferState(std::move(src.DeviceTransferState))
 {
 }
@@ -39,7 +37,6 @@ StorageVirtual::StorageVirtual(StorageVirtual&& src) noexcept
 //--------------------------------------------------------------------
 StorageVirtual& StorageVirtual::operator=(const StorageVirtual& src)
 {
-  this->HostUpToDate = src.HostUpToDate;
   this->DeviceUpToDate = src.DeviceUpToDate;
   this->DeviceTransferState = src.DeviceTransferState;
   return *this;
@@ -48,7 +45,6 @@ StorageVirtual& StorageVirtual::operator=(const StorageVirtual& src)
 //--------------------------------------------------------------------
 StorageVirtual& StorageVirtual::operator=(StorageVirtual&& src) noexcept
 {
-  this->HostUpToDate = src.HostUpToDate;
   this->DeviceUpToDate = src.DeviceUpToDate;
   this->DeviceTransferState = std::move(src.DeviceTransferState);
   return *this;
@@ -70,7 +66,6 @@ void StorageVirtual::DropExecutionPortal()
 void StorageVirtual::DropAllPortals()
 {
   this->DeviceTransferState->releaseAll();
-  this->HostUpToDate = false;
   this->DeviceUpToDate = false;
 }
 
@@ -117,7 +112,6 @@ const vtkm::internal::PortalVirtualBase* StorageVirtual::PrepareForOutput(
   {
     this->TransferPortalForOutput(
       *(this->DeviceTransferState), OutputMode::WRITE, numberOfValues, devId);
-    this->HostUpToDate = false;
     this->DeviceUpToDate = true;
   }
   return this->DeviceTransferState->devicePtr();
@@ -138,37 +132,29 @@ const vtkm::internal::PortalVirtualBase* StorageVirtual::PrepareForInPlace(
     vtkm::Id numberOfValues = this->GetNumberOfValues();
     this->TransferPortalForOutput(
       *(this->DeviceTransferState), OutputMode::READ_WRITE, numberOfValues, devId);
-    this->HostUpToDate = false;
     this->DeviceUpToDate = true;
   }
   return this->DeviceTransferState->devicePtr();
 }
 
 //--------------------------------------------------------------------
-const vtkm::internal::PortalVirtualBase* StorageVirtual::WritePortal()
+std::unique_ptr<vtkm::internal::PortalVirtualBase>&& StorageVirtual::WritePortal()
 {
-  if (!this->HostUpToDate)
-  {
-    //we need to prepare for input and grab the host ptr
-    auto* payload = this->DeviceTransferState.get();
-    this->ControlPortalForOutput(*payload);
-  }
+  //we need to prepare for input and grab the host ptr
+  auto* payload = this->DeviceTransferState.get();
+  this->ControlPortalForOutput(*payload);
 
   this->DeviceUpToDate = false;
-  this->HostUpToDate = true;
   return this->DeviceTransferState->hostPtr();
 }
 
 //--------------------------------------------------------------------
-const vtkm::internal::PortalVirtualBase* StorageVirtual::ReadPortal() const
+std::unique_ptr<vtkm::internal::PortalVirtualBase>&& StorageVirtual::ReadPortal() const
 {
-  if (!this->HostUpToDate)
-  {
-    //we need to prepare for input and grab the host ptr
-    vtkm::cont::internal::TransferInfoArray* payload = this->DeviceTransferState.get();
-    this->ControlPortalForInput(*payload);
-  }
-  this->HostUpToDate = true;
+  //we need to prepare for input and grab the host ptr
+  vtkm::cont::internal::TransferInfoArray* payload = this->DeviceTransferState.get();
+  this->ControlPortalForInput(*payload);
+
   return this->DeviceTransferState->hostPtr();
 }
 

vtkm/cont/StorageVirtual.h

@@ -25,6 +25,40 @@ namespace vtkm
 namespace cont
 {
 
+// A control-side version of ArrayPortalRef that also manages the object created.
+template <typename T>
+class VTKM_ALWAYS_EXPORT ArrayPortalRef : public vtkm::ArrayPortalRef<T>
+{
+  std::shared_ptr<vtkm::ArrayPortalVirtual<T>> ManagedPortal;
+
+public:
+  ArrayPortalRef() = default;
+  ~ArrayPortalRef() = default;
+
+  ArrayPortalRef(std::shared_ptr<vtkm::ArrayPortalVirtual<T>> portal, vtkm::Id numValues) noexcept
+    : vtkm::ArrayPortalRef<T>(portal.get(), numValues),
+      ManagedPortal(portal)
+  {
+  }
+};
+
+} // namespace cont
+
+template <typename T>
+inline vtkm::cont::ArrayPortalRef<T> make_ArrayPortalRef(
+  std::shared_ptr<vtkm::ArrayPortalVirtual<T>> portal,
+  vtkm::Id numValues)
+{
+  return vtkm::cont::ArrayPortalRef<T>(portal, numValues);
+}
+
+} // namespace vtkm
+
+namespace vtkm
+{
+namespace cont
+{
+
 struct VTKM_ALWAYS_EXPORT StorageTagVirtual
 {
 };
@@ -122,11 +156,11 @@ public:
 
   //This needs to cause a host side sync!
   //This needs to work before we execute on a device
-  const vtkm::internal::PortalVirtualBase* WritePortal();
+  std::unique_ptr<vtkm::internal::PortalVirtualBase>&& WritePortal();
 
   //This needs to cause a host side sync!
   //This needs to work before we execute on a device
-  const vtkm::internal::PortalVirtualBase* ReadPortal() const;
+  std::unique_ptr<vtkm::internal::PortalVirtualBase>&& ReadPortal() const;
 
   /// Returns the DeviceAdapterId for the current device. If there is no device
   /// with an up-to-date copy of the data, VTKM_DEVICE_ADAPTER_UNDEFINED is
@@ -170,8 +204,7 @@ private:
                                        vtkm::Id numberOfValues,
                                        vtkm::cont::DeviceAdapterId devId);
 
-  //These might need to exist in TransferInfoArray
-  mutable bool HostUpToDate = false;
+  //This might need to exist in TransferInfoArray
   mutable bool DeviceUpToDate = false;
   std::shared_ptr<vtkm::cont::internal::TransferInfoArray> DeviceTransferState =
     std::make_shared<vtkm::cont::internal::TransferInfoArray>();
@@ -230,8 +263,8 @@ class VTKM_ALWAYS_EXPORT Storage<T, vtkm::cont::StorageTagVirtual>
 public:
   using ValueType = T;
 
-  using PortalType = vtkm::ArrayPortalRef<T>;
-  using PortalConstType = vtkm::ArrayPortalRef<T>;
+  using PortalType = vtkm::cont::ArrayPortalRef<T>;
+  using PortalConstType = vtkm::cont::ArrayPortalRef<T>;
 
   Storage() = default;
 
@@ -249,14 +282,16 @@ public:
   PortalType GetPortal()
   {
     return make_ArrayPortalRef(
-      static_cast<const vtkm::ArrayPortalVirtual<T>*>(this->VirtualStorage->WritePortal()),
+      std::shared_ptr<vtkm::ArrayPortalVirtual<T>>(reinterpret_cast<vtkm::ArrayPortalVirtual<T>*>(
+        this->VirtualStorage->WritePortal().release())),
       this->GetNumberOfValues());
   }
 
   PortalConstType GetPortalConst() const
   {
     return make_ArrayPortalRef(
-      static_cast<const vtkm::ArrayPortalVirtual<T>*>(this->VirtualStorage->ReadPortal()),
+      std::shared_ptr<vtkm::ArrayPortalVirtual<T>>(reinterpret_cast<vtkm::ArrayPortalVirtual<T>*>(
+        this->VirtualStorage->ReadPortal().release())),
       this->GetNumberOfValues());
   }
 

vtkm/cont/internal/ArrayPortalToken.h

@@ -91,7 +91,14 @@ public:
   ///
   /// This will open up the `ArrayHandle` for reading and/or writing.
   ///
-  VTKM_CONT void Detach() { this->Token->DetachFromAll(); }
+  VTKM_CONT void Detach()
+  {
+    this->Token->DetachFromAll();
+
+    // Reset this portal in case the superclass is holding other array portals with their own
+    // tokens. Detach is supposed to invalidate the array portal, so it is OK to do this.
+    *this = ArrayPortalToken<PortalType_>();
+  }
 
   /// \brief Get the `Token` of the `ArrayPortal`.
   ///

vtkm/cont/internal/TransferInfo.h

@@ -44,7 +44,10 @@ struct VTKM_CONT_EXPORT TransferInfoArray
   void releaseDevice();
   void releaseAll();
 
-  const vtkm::internal::PortalVirtualBase* hostPtr() const noexcept { return this->Host.get(); }
+  std::unique_ptr<vtkm::internal::PortalVirtualBase>&& hostPtr() noexcept
+  {
+    return std::move(this->Host);
+  }
   const vtkm::internal::PortalVirtualBase* devicePtr() const noexcept { return this->Device; }
   vtkm::cont::DeviceAdapterId deviceId() const noexcept { return this->DeviceId; }
 

vtkm/cont/testing/UnitTestArrayHandleVirtual.cxx

@@ -40,6 +40,8 @@ struct Test
 
   void TestConstructors()
   {
+    std::cout << "Constructors" << std::endl;
+
     VirtHandle nullStorage;
     VTKM_TEST_ASSERT(nullStorage.GetStorage().GetStorageVirtual() == nullptr,
                      "storage should be empty when using ArrayHandleVirtual().");
@@ -67,6 +69,8 @@ struct Test
 
   void TestMoveConstructors()
   {
+    std::cout << "Move constructors" << std::endl;
+
     //test ArrayHandle move constructor
     {
       ArrayHandle handle;
@@ -89,6 +93,8 @@ struct Test
 
   void TestAssignmentOps()
   {
+    std::cout << "Assignment operators" << std::endl;
+
     //test assignment operator from ArrayHandleVirtual
     {
       VirtHandle virt;
@@ -126,8 +132,10 @@ struct Test
 
   void TestPrepareForExecution()
   {
+    std::cout << "Prepare for execution" << std::endl;
+
     vtkm::cont::ArrayHandle<ValueType> handle;
-    handle.Allocate(50);
+    handle.Allocate(ARRAY_SIZE);
 
     VirtHandle virt(std::move(handle));
 
@@ -149,6 +157,8 @@ struct Test
 
   void TestIsType()
   {
+    std::cout << "IsType" << std::endl;
+
     vtkm::cont::ArrayHandle<ValueType> handle;
     VirtHandle virt(std::move(handle));
 
@@ -164,6 +174,7 @@ struct Test
 
   void TestCast()
   {
+    std::cout << "Cast" << std::endl;
 
     vtkm::cont::ArrayHandle<ValueType> handle;
     VirtHandle virt(handle);
@@ -185,14 +196,49 @@ struct Test
     }
   }
 
+  void TestControlPortalLocking()
+  {
+    std::cout << "Control portal locking" << std::endl;
+
+    // There was a bug where a control portal was not relinquished and it locked the
+    // ArrayHandle from further use.
+
+    ArrayHandle concreteArray;
+    concreteArray.Allocate(ARRAY_SIZE);
+
+    VirtHandle virtualArray(concreteArray);
+
+    // Make sure you can write to the virtualArray and then read the data from the concreteArray
+    // without the concreteArray getting locked up.
+    SetPortal(virtualArray.WritePortal());
+    CheckPortal(concreteArray.ReadPortal());
+
+    // Make sure you can read from the virtualArray and the write to the concreteArray without
+    // the concreteArray getting locked up.
+    CheckPortal(virtualArray.ReadPortal());
+    SetPortal(concreteArray.WritePortal());
+
+    // Make sure Detach also correctly unlocks the concrete array.
+    auto readPortal = virtualArray.ReadPortal();
+    readPortal.Detach();
+    concreteArray.WritePortal();
+
+    auto writePortal = virtualArray.WritePortal();
+    writePortal.Detach();
+    concreteArray.WritePortal();
+  }
+
   void operator()()
   {
+    std::cout << std::endl;
+    std::cout << "### Testing for " << vtkm::cont::TypeToString<ValueType>() << std::endl;
     TestConstructors();
     TestMoveConstructors();
     TestAssignmentOps();
     TestPrepareForExecution();
     TestIsType();
     TestCast();
+    TestControlPortalLocking();
   }
 };