From: Roland Schulz Date: Thu, 16 Aug 2018 01:40:20 +0000 (-0700) Subject: Remove pin/unpin from HostAllocator X-Git-Url: http://biod.pnpi.spb.ru/gitweb/?a=commitdiff_plain;h=42dae673fb68c9c08836494361a687531e0bf4bf;p=alexxy%2Fgromacs.git Remove pin/unpin from HostAllocator 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 --- diff --git a/cmake/gmxCFlags.cmake b/cmake/gmxCFlags.cmake index 29623e2bef..fec8adf422 100644 --- a/cmake/gmxCFlags.cmake +++ b/cmake/gmxCFlags.cmake @@ -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() diff --git a/src/gromacs/gpu_utils/hostallocator.cpp b/src/gromacs/gpu_utils/hostallocator.cpp index d177456a7f..32e342a54f 100644 --- a/src/gromacs/gpu_utils/hostallocator.cpp +++ b/src/gromacs/gpu_utils/hostallocator.cpp @@ -42,8 +42,6 @@ #include "hostallocator.h" -#include "config.h" - #include #include @@ -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 diff --git a/src/gromacs/gpu_utils/hostallocator.h b/src/gromacs/gpu_utils/hostallocator.h index 7b0c0cd53b..2806770f6e 100644 --- a/src/gromacs/gpu_utils/hostallocator.h +++ b/src/gromacs/gpu_utils/hostallocator.h @@ -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 +bool operator==(const Allocator &a, + const Allocator &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 void changePinningPolicy(HostVector *v, PinningPolicy pinningPolicy) { - if (v->get_allocator().pinningPolicy() == pinningPolicy) - { - return; - } - //Force reallocation by creating copy - *v = HostVector(*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(std::move(*v), {pinningPolicy}); } } // namespace gmx diff --git a/src/gromacs/gpu_utils/pinning.cu b/src/gromacs/gpu_utils/pinning.cu index 0660af72d5..bf140b0647 100644 --- a/src/gromacs/gpu_utils/pinning.cu +++ b/src/gromacs/gpu_utils/pinning.cu @@ -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 +#include + #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(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); diff --git a/src/gromacs/gpu_utils/tests/hostallocator.cpp b/src/gromacs/gpu_utils/tests/hostallocator.cpp index 1c73fbaeda..629b4549d8 100644 --- a/src/gromacs/gpu_utils/tests/hostallocator.cpp +++ b/src/gromacs/gpu_utils/tests/hostallocator.cpp @@ -161,8 +161,18 @@ void HostMemoryTest::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 TestTypes; +typedef ::testing::Types TestTypes; //! Typed test fixture template @@ -180,6 +190,13 @@ struct HostAllocatorTestNoMem : ::testing::Test }; TYPED_TEST_CASE(HostAllocatorTestNoMem, TestTypes); +//! Typed test fixture for tests requiring a copyable type +template +struct HostAllocatorTestNoMemCopyable : HostAllocatorTestNoMem {}; +//! The types used in testing minus move only types +using TestTypesCopyable = ::testing::Types; +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{}, HostAllocator{}); +} + //! Declare allocator types to test. using AllocatorTypesToTest = ::testing::Types, HostAllocator, - HostAllocator + HostAllocator, + HostAllocator >; TYPED_TEST_CASE(AllocatorTest, AllocatorTypesToTest); diff --git a/src/gromacs/utility/allocator.h b/src/gromacs/utility/allocator.h index f67a0e9e21..65f1339afa 100644 --- a/src/gromacs/utility/allocator.h +++ b/src/gromacs/utility/allocator.h @@ -222,15 +222,15 @@ class Allocator : public AllocationPolicy * by size_type. */ std::size_t - max_size() const { return (static_cast(0) - static_cast(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 - bool - operator==(const Allocator & /*unused*/) const { return std::is_same::value; } + template::value>::type> + bool operator==(const Allocator & /*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 diff --git a/src/gromacs/utility/tests/alignedallocator-impl.h b/src/gromacs/utility/tests/alignedallocator-impl.h index aa77d1f29d..8a49e0172d 100644 --- a/src/gromacs/utility/tests/alignedallocator-impl.h +++ b/src/gromacs/utility/tests/alignedallocator-impl.h @@ -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 v1(1); + value_type* data = v1.data(); + EXPECT_NE(data, nullptr); + std::vector v2(std::move(v1)); + EXPECT_EQ(data, v2.data()); +} + } // namespace test } // namespace gmx diff --git a/src/gromacs/utility/tests/alignedallocator.cpp b/src/gromacs/utility/tests/alignedallocator.cpp index e188dcb8d0..d046a24c6b 100644 --- a/src/gromacs/utility/tests/alignedallocator.cpp +++ b/src/gromacs/utility/tests/alignedallocator.cpp @@ -84,5 +84,12 @@ TYPED_TEST(AllocatorTest, StatelessAllocatorUsesNoMemory) sizeof(std::vector)); } +TEST(AllocatorUntypedTest, Comparison) +{ + //Should always be true for the same policy, indpendent of value_type + EXPECT_EQ(AlignedAllocator{}, AlignedAllocator{}); + EXPECT_EQ(PageAlignedAllocator{}, PageAlignedAllocator{}); +} + } // namespace test } // namespace gmx