Remove pin/unpin from HostAllocator
authorRoland Schulz <roland.schulz@intel.com>
Thu, 16 Aug 2018 01:40:20 +0000 (18:40 -0700)
committerMark Abraham <mark.j.abraham@gmail.com>
Thu, 6 Sep 2018 14:51:37 +0000 (16:51 +0200)
Simplifies HostAllocator a lot and there doesn't seem to
be a good usage of those functions. Also allows to
call unpin without first checking whether the memory
was pinned.

Also fix comparison operator for HostAllocator and
make it properly work with move-only types.

Change-Id: I3be397a26c10ef68334c3753666802e46129a0ec

cmake/gmxCFlags.cmake
src/gromacs/gpu_utils/hostallocator.cpp
src/gromacs/gpu_utils/hostallocator.h
src/gromacs/gpu_utils/pinning.cu
src/gromacs/gpu_utils/tests/hostallocator.cpp
src/gromacs/utility/allocator.h
src/gromacs/utility/tests/alignedallocator-impl.h
src/gromacs/utility/tests/alignedallocator.cpp

index 29623e2befbd227bce417f52a46c380eec022530..fec8adf4223fcd6ad90a77352b1edd462e99f5e7 100644 (file)
@@ -210,8 +210,9 @@ GMX_TEST_CFLAG(CFLAGS_WARN "/W3 /wd177 /wd411 /wd593 /wd981 /wd1418 /wd1419 /wd1
 # 304: access control not specified
 # 383: value copied to temporary, reference to temporary used
 # 444: destructor for base class ".." is not virtual
+# 869: was never referenced (false positives)
 #2282: unrecognized GCC pragma
-                GMX_TEST_CXXFLAG(CXXFLAGS_WARN "-w3 -wd177 -wd304 -wd383 -wd411 -wd444 -wd981 -wd1418 -wd1572 -wd1599 -wd2259 -wd2547 -wd3280 -wd11074 -wd11076 -wd2282" GMXC_CXXFLAGS)
+                GMX_TEST_CXXFLAG(CXXFLAGS_WARN "-w3 -wd177 -wd304 -wd383 -wd411 -wd444 -wd869 -wd981 -wd1418 -wd1572 -wd1599 -wd2259 -wd2547 -wd3280 -wd11074 -wd11076 -wd2282" GMXC_CXXFLAGS)
             endif()
             GMX_TEST_CXXFLAG(CXXFLAGS_OPT "-ip -funroll-all-loops -alias-const -ansi-alias -no-prec-div -fimf-domain-exclusion=14 -qoverride-limits" GMXC_CXXFLAGS_RELEASE)
             GMX_TEST_CXXFLAG(CXXFLAGS_DEBUG "-O0" GMXC_CXXFLAGS_DEBUG)
@@ -222,7 +223,7 @@ GMX_TEST_CFLAG(CFLAGS_WARN "/W3 /wd177 /wd411 /wd593 /wd981 /wd1418 /wd1419 /wd1
             endif()
             if (GMX_COMPILER_WARNINGS)
 #809: exception specification for virtual function X is incompatible with that of overridden function
-                GMX_TEST_CXXFLAG(CXXFLAGS_WARN "/W3 /wd177 /wd304 /wd383 /wd411 /wd444 /wd809 /wd981 /wd1418 /wd1572 /wd1599 /wd1786 /wd2259 /wd2547 /wd3280 /wd11074 /wd11076 /wd2282" GMXC_CXXFLAGS)
+                GMX_TEST_CXXFLAG(CXXFLAGS_WARN "/W3 /wd177 /wd304 /wd383 /wd411 /wd444 /wd809 /wd869 /wd981 /wd1418 /wd1572 /wd1599 /wd1786 /wd2259 /wd2547 /wd3280 /wd11074 /wd11076 /wd2282" GMXC_CXXFLAGS)
             endif()
             GMX_TEST_CXXFLAG(CXXFLAGS_OPT "/Qip" GMXC_CXXFLAGS_RELEASE)
         endif()
index d177456a7f33416138c272dd0ebe9a57495ed930..32e342a54f2395b8a821b06750e42d89a79be2da 100644 (file)
@@ -42,8 +42,6 @@
 
 #include "hostallocator.h"
 
-#include "config.h"
-
 #include <cstddef>
 
 #include <memory>
@@ -74,7 +72,22 @@ void *HostAllocationPolicy::malloc(std::size_t bytes) const noexcept
     if (pinningPolicy_ == PinningPolicy::PinnedIfSupported)
     {
         void *p = PageAlignedAllocationPolicy::malloc(bytes);
-        pin(p, bytes);
+        if (p)
+        {
+            /* For every pin, unpin has to be called or resources will
+             * leak.  Doing this correctly is guaranteed because for
+             * every p!=null && pinningPolicy_ == PinnedIfSupported,
+             * the malloc and free calls handle pinning. For very
+             * standard-compliant containers, the allocator object
+             * can't be changed independently of the buffer (for move,
+             * it is propagated) and thus the allocator (and thus
+             * pinningPolicy_) can't change between malloc and
+             * free.
+             *
+             * Note that we always pin (even for size 0) so that we
+             * can always unpin without any checks. */
+            pinBuffer(p, bytes);
+        }
         return p;
     }
     else
@@ -90,9 +103,9 @@ void HostAllocationPolicy::free(void *buffer) const noexcept
         // Nothing to do
         return;
     }
-    unpin(buffer);
     if (pinningPolicy_ == PinningPolicy::PinnedIfSupported)
     {
+        unpinBuffer(buffer);
         PageAlignedAllocationPolicy::free(buffer);
     }
     else
@@ -101,28 +114,4 @@ void HostAllocationPolicy::free(void *buffer) const noexcept
     }
 }
 
-void HostAllocationPolicy::pin(void gmx_unused *p, size_t gmx_unused n) const noexcept
-{
-#if GMX_GPU == GMX_GPU_CUDA
-    // I believe this if statement isn't required for calls from malloc. But it
-    // is required for the unit tests. Which might not make much sense to test
-    // cases which can't actually happen for an allocator.
-    if (p == nullptr || n == 0 || isHostMemoryPinned(p))
-    {
-        return;
-    }
-    pinBuffer(p, n);
-#endif
-}
-
-void HostAllocationPolicy::unpin(void gmx_unused *p) const noexcept
-{
-#if GMX_GPU == GMX_GPU_CUDA
-    if (isHostMemoryPinned(p))
-    {
-        unpinBuffer(p);
-    }
-#endif
-}
-
 } // namespace gmx
index 7b0c0cd53bf4027fa9c3774bd7024478460c0ec3..2806770f6ee092d5a810cfdce61a431a8216cec7 100644 (file)
@@ -178,20 +178,6 @@ class HostAllocationPolicy
          *
          * Does not throw.
          */
-        void pin(void* p, size_t n) const noexcept;
-        /*! \brief Unpin the allocation, if appropriate.
-         *
-         * Regardless of the allocation policy, unpin the memory if
-         * previously pinned, otherwise do nothing.
-         *
-         * Does not throw.
-         */
-        void unpin(void* p) const noexcept;
-        /*! \brief Return the current pinning policy (which is semi-independent
-         * of whether the buffer is actually pinned).
-         *
-         * Does not throw.
-         */
         PinningPolicy pinningPolicy() const { return pinningPolicy_; }
         //! Don't propagate for copy
         using propagate_on_container_copy_assignment = std::false_type;
@@ -205,11 +191,23 @@ class HostAllocationPolicy
         {
             return {};
         }
+
     private:
         //! Pinning policy
         PinningPolicy pinningPolicy_;
 };
 
+/*! \brief Return true if two allocators are identical
+ *
+ * True if pinning policy is the same.
+ */
+template<class T1, class T2>
+bool operator==(const Allocator<T1, HostAllocationPolicy> &a,
+                const Allocator<T2, HostAllocationPolicy> &b)
+{
+    return a.pinningPolicy() == b.pinningPolicy();
+}
+
 /*! \brief Helper function for changing the pinning policy of a HostVector.
  *
  * If the vector has contents, then a full reallocation and buffer
@@ -221,12 +219,9 @@ class HostAllocationPolicy
 template <class T>
 void changePinningPolicy(HostVector<T> *v, PinningPolicy pinningPolicy)
 {
-    if (v->get_allocator().pinningPolicy() == pinningPolicy)
-    {
-        return;
-    }
-    //Force reallocation by creating copy
-    *v = HostVector<T>(*v, {pinningPolicy});
+    //Force reallocation by element-wise move (because policy is different
+    //container is forced to realloc). Does nothing if policy is the same.
+    *v = HostVector<T>(std::move(*v), {pinningPolicy});
 }
 }      // namespace gmx
 
index 0660af72d564a34ff0080faf5a3026c92f43de6c..bf140b064752dcceb66fe4367a0c0906621741d3 100644 (file)
@@ -1,7 +1,7 @@
 /*
  * This file is part of the GROMACS molecular simulation package.
  *
- * Copyright (c) 2017, by the GROMACS development team, led by
+ * Copyright (c) 2017,2018, by the GROMACS development team, led by
  * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl,
  * and including many others, as listed in the AUTHORS file in the
  * top-level source directory and at http://www.gromacs.org.
@@ -44,6 +44,8 @@
 
 #include <cstddef>
 
+#include <algorithm>
+
 #include "gromacs/gpu_utils/cudautils.cuh"
 #include "gromacs/utility/alignedallocator.h"
 #include "gromacs/utility/exceptions.h"
@@ -66,6 +68,8 @@ void pinBuffer(void *pointer, std::size_t numBytes) noexcept
     GMX_ASSERT(isAligned(pointer, PageAlignedAllocationPolicy::alignment()),
                formatString("%s Host memory needs to be page aligned.", errorMessage).c_str());
 
+    numBytes = std::max<size_t>(1, numBytes); //C++11 3.7.4.1 gurantees that every pointer is different thus at least 1 byte
+
     ensureNoPendingCudaError(errorMessage);
     cudaError_t stat = cudaHostRegister(pointer, numBytes, cudaHostRegisterDefault);
 
index 1c73fbaedaa5d04a85331832b9e9620913c0ce1e..629b4549d8d931b0f8e1ed7f26a0877cad713cac 100644 (file)
@@ -161,8 +161,18 @@ void HostMemoryTest<T>::runTest(ConstViewType input, ViewType output) const
     this->compareVectors(input, output);
 }
 
+struct MoveOnly {
+    MoveOnly(real x = 0) : x(x) {}
+    MoveOnly(const MoveOnly &)            = delete;
+    MoveOnly(MoveOnly &&)                 = default;
+    MoveOnly &operator=(const MoveOnly &) = delete;
+    MoveOnly &operator=(MoveOnly &&)      = default;
+    bool operator==(const MoveOnly &o) const { return x == o.x; }
+    real x;
+};
+
 //! The types used in testing.
-typedef ::testing::Types<int, real, RVec> TestTypes;
+typedef ::testing::Types<int, real, RVec, MoveOnly> TestTypes;
 
 //! Typed test fixture
 template <typename T>
@@ -180,6 +190,13 @@ struct HostAllocatorTestNoMem : ::testing::Test
 };
 TYPED_TEST_CASE(HostAllocatorTestNoMem, TestTypes);
 
+//! Typed test fixture for tests requiring a copyable type
+template <typename T>
+struct HostAllocatorTestNoMemCopyable : HostAllocatorTestNoMem<T> {};
+//! The types used in testing minus move only types
+using TestTypesCopyable = ::testing::Types<int, real, RVec>;
+TYPED_TEST_CASE(HostAllocatorTestNoMemCopyable, TestTypesCopyable);
+
 // Note that in GoogleTest typed tests, the use of TestFixture:: and
 // this-> is sometimes required to get access to things in the fixture
 // class (or its base classes).
@@ -194,7 +211,7 @@ TYPED_TEST(HostAllocatorTest, EmptyMemoryAlwaysWorks)
 
 TYPED_TEST(HostAllocatorTest, VectorsWithDefaultHostAllocatorAlwaysWorks)
 {
-    typename TestFixture::VectorType input = {{1, 2, 3}}, output;
+    typename TestFixture::VectorType input(3), output;
     output.resize(input.size());
 }
 
@@ -251,7 +268,7 @@ TYPED_TEST(HostAllocatorTestNoMem, MoveConstruction)
     EXPECT_TRUE(input4.get_allocator().pinningPolicy() == PinningPolicy::PinnedIfSupported);
 }
 
-TYPED_TEST(HostAllocatorTestNoMem, CopyAssignment)
+TYPED_TEST(HostAllocatorTestNoMemCopyable, CopyAssignment)
 {
     typename TestFixture::VectorType input1;
     typename TestFixture::VectorType input2({PinningPolicy::PinnedIfSupported});
@@ -263,7 +280,7 @@ TYPED_TEST(HostAllocatorTestNoMem, CopyAssignment)
     EXPECT_TRUE (input2.get_allocator().pinningPolicy() == PinningPolicy::PinnedIfSupported);
 }
 
-TYPED_TEST(HostAllocatorTestNoMem, CopyConstruction)
+TYPED_TEST(HostAllocatorTestNoMemCopyable, CopyConstruction)
 {
     typename TestFixture::VectorType input1;
     typename TestFixture::VectorType input2(input1); //NOLINT(performance-unnecessary-copy-initialization)
@@ -286,6 +303,14 @@ TYPED_TEST(HostAllocatorTestNoMem, Swap)
     EXPECT_TRUE (input2.get_allocator().pinningPolicy() == PinningPolicy::PinnedIfSupported);
 }
 
+TYPED_TEST(HostAllocatorTestNoMem, Comparison)
+{
+    using AllocatorType = typename TestFixture::VectorType::allocator_type;
+    EXPECT_EQ(AllocatorType {}, AllocatorType {});
+    //Should be false for different pinning policy
+    EXPECT_NE(AllocatorType {}, AllocatorType {PinningPolicy::PinnedIfSupported});
+}
+
 #if GMX_GPU == GMX_GPU_CUDA
 
 // Policy suitable for pinning is only supported for a CUDA build
@@ -327,34 +352,10 @@ TYPED_TEST(HostAllocatorTest, ManualPinningOperationsWorkWithCuda)
     EXPECT_TRUE(input.get_allocator().pinningPolicy() == PinningPolicy::PinnedIfSupported);
     EXPECT_FALSE(isPinned(input));
 
-    // Unpin before allocation is fine, but does nothing.
-    input.get_allocator().getPolicy().unpin(input.data());
-    EXPECT_FALSE(isPinned(input));
-
-    // Pin with no contents is fine, but does nothing.
-    input.get_allocator().getPolicy().pin(input.data(), 0);
-    EXPECT_FALSE(isPinned(input));
-
     // Fill some contents, which will be pinned because of the policy.
     this->fillInput(&input);
     EXPECT_TRUE(isPinned(input));
 
-    // Unpin after pin is fine.
-    input.get_allocator().getPolicy().unpin(input.data());
-    EXPECT_FALSE(isPinned(input));
-
-    // Repeated unpin should be a no-op.
-    input.get_allocator().getPolicy().unpin(input.data());
-
-    // Pin after unpin is fine.
-    const size_t size = input.size() * sizeof(typename TestFixture::VectorType::value_type);
-    input.get_allocator().getPolicy().pin(input.data(), size);
-    EXPECT_TRUE(isPinned(input));
-
-    // Repeated pin should be a no-op, and still pinned.
-    input.get_allocator().getPolicy().pin(input.data(), size);
-    EXPECT_TRUE(isPinned(input));
-
     // Switching policy to CannotBePinned must unpin the buffer (via
     // realloc and copy).
     auto oldInputData = input.data();
@@ -374,19 +375,6 @@ TYPED_TEST(HostAllocatorTest, ManualPinningOperationsWorkWithCuda)
     EXPECT_NE(oldInputData, input.data());
 }
 
-#else
-
-TYPED_TEST(HostAllocatorTest, ManualPinningOperationsWorkEvenWithoutCuda)
-{
-    typename TestFixture::VectorType input;
-
-    // Since the buffer can't be pinned and isn't pinned, and the
-    // calling code can't be unhappy about this, these are OK.
-    input.get_allocator().getPolicy().pin(input.data(),
-                                          input.size()*sizeof(typename TestFixture::VectorType::value_type));
-    input.get_allocator().getPolicy().unpin(input.data());
-}
-
 #endif
 
 TYPED_TEST(HostAllocatorTest, StatefulAllocatorUsesMemory)
@@ -398,10 +386,17 @@ TYPED_TEST(HostAllocatorTest, StatefulAllocatorUsesMemory)
               sizeof(typename TestFixture::VectorType));
 }
 
+TEST(HostAllocatorUntypedTest, Comparison)
+{
+    //Should always be true for the same policy, indpendent of value_type
+    EXPECT_EQ(HostAllocator<float>{}, HostAllocator<double>{});
+}
+
 //! Declare allocator types to test.
 using AllocatorTypesToTest = ::testing::Types<HostAllocator<real>,
                                               HostAllocator<int>,
-                                              HostAllocator<RVec>
+                                              HostAllocator<RVec>,
+                                              HostAllocator<MoveOnly>
                                               >;
 
 TYPED_TEST_CASE(AllocatorTest, AllocatorTypesToTest);
index f67a0e9e213248f0c4d822a3efd878d97ede0bd1..65f1339afaaa9d1b033aa86da79a94b2d5b98611 100644 (file)
@@ -222,15 +222,15 @@ class Allocator : public AllocationPolicy
          * by size_type.
          */
         std::size_t
-        max_size() const { return (static_cast<std::size_t>(0) - static_cast<std::size_t>(1)) / sizeof(T); }
+        max_size() const { return SIZE_MAX / sizeof(T); }
 
         /*! \brief Return true if two allocators are identical
          *
          * This is a member function of the left-hand-side allocator.
+         * Always true for stateless polcies. Has to be defined in the policy for stateful policies.
          */
-        template<class T2>
-        bool
-        operator==(const Allocator<T2, AllocationPolicy> & /*unused*/) const { return std::is_same<T, T2>::value; }
+        template<class T2, class A = AllocationPolicy, typename = typename std::enable_if<std::is_empty<A>::value>::type>
+        bool operator==(const Allocator<T2, AllocationPolicy> & /*unused*/) const { return true; }
 
         /*! \brief Return true if two allocators are different
          *
@@ -239,13 +239,7 @@ class Allocator : public AllocationPolicy
          * This is a member function of the left-hand-side allocator.
          */
         bool
-        operator!=(const Allocator &rhs) const { return !operator==(rhs); }
-
-        //! Obtain allocator for copy construction
-        Allocator select_on_container_copy_construction() const
-        {
-            return Allocator(AllocationPolicy::select_on_container_copy_construction());
-        }
+        operator!=(const Allocator &rhs) const { return !(*this == rhs); }
 };
 
 }      // namespace gmx
index aa77d1f29d4da33e4e7d2cbfe28f0af74236d83e..8a49e0172de2c8f4c53db9e3b6d91a5396585fcd 100644 (file)
@@ -123,6 +123,16 @@ TYPED_TEST(AllocatorTest, VectorAllocatesAndReservesWithAlignment) //NOLINT(misc
     }
 }
 
+TYPED_TEST(AllocatorTest, Move) //NOLINT(misc-definitions-in-headers)
+{
+    using value_type = typename TypeParam::value_type;
+    std::vector<value_type, TypeParam> v1(1);
+    value_type* data = v1.data();
+    EXPECT_NE(data, nullptr);
+    std::vector<value_type, TypeParam> v2(std::move(v1));
+    EXPECT_EQ(data, v2.data());
+}
+
 }  // namespace test
 }  // namespace gmx
 
index e188dcb8d08d661cae1e2e2a0de04b2ca3ed4700..d046a24c6b565642336d5a411a886a65301dab02 100644 (file)
@@ -84,5 +84,12 @@ TYPED_TEST(AllocatorTest, StatelessAllocatorUsesNoMemory)
               sizeof(std::vector<value_type, TypeParam>));
 }
 
+TEST(AllocatorUntypedTest, Comparison)
+{
+    //Should always be true for the same policy, indpendent of value_type
+    EXPECT_EQ(AlignedAllocator<float>{}, AlignedAllocator<double>{});
+    EXPECT_EQ(PageAlignedAllocator<float>{}, PageAlignedAllocator<double>{});
+}
+
 }  // namespace test
 }  // namespace gmx