#4092: DynamicView: rewrite without UVM #4129

lifflander · 2021-06-28T19:00:50Z

lifflander · 2021-06-28T19:01:47Z

containers/src/Kokkos_DynamicView.hpp

+    // Destroy the array of chunk pointers.
+    // Two entries beyond the max chunks are allocation counters.
+    for (unsigned i = 0; i < m_chunk_max; i++) {
+      MemorySpace().deallocate(


I'm not sure what the preferred allocation/deallocation style is. I went with this style throughout but could change it to use kokkos_malloc if preferable.

@crtrott Are you ok with the MemorySpace().allocate/deallocate style used throughout?

lifflander · 2021-06-28T19:02:45Z

containers/src/Kokkos_DynamicView.hpp

@@ -93,6 +259,12 @@ class DynamicView : public Kokkos::ViewTraits<DataType, P...> {
 public:
  using traits = Kokkos::ViewTraits<DataType, P...>;

+  using value_type      = typename traits::value_type;
+  using device_space    = typename traits::memory_space;
+  using host_space      = typename Kokkos::Impl::HostMirror<device_space>::Space::memory_space;


Is this the right trait to use for getting the HostMirror memory space?

containers/src/Kokkos_DynamicView.hpp

lifflander · 2021-06-29T22:17:44Z

retest this please

crtrott

This is pretty good. Other than adding two comments or so and maybe renaming some stuff as pointed out in the comments this is good to go.

containers/src/Kokkos_DynamicView.hpp

crtrott · 2021-06-30T00:35:17Z

containers/src/Kokkos_DynamicView.hpp

+  using host_space =
+      typename Kokkos::Impl::HostMirror<device_space>::Space::memory_space;
+  using device_accessor = Impl::SmartMemoryAccessor<device_space, value_type>;
+  using host_accessor   = Impl::SmartMemoryAccessor<host_space, value_type>;


might be worthwhile to think about a better name. Since this is just the outer array, it doesn't actually give you access to the primary data on the host after all or?

Changed the name to ChunkedArrayManager. Let me know if you can think of something better!

containers/src/Kokkos_DynamicView.hpp

crtrott · 2021-06-30T00:45:13Z

Also this crashed NVCC 9.2 with an internal error, and GCC 5.3 doesn't like it either:

/var/jenkins/workspace/Kokkos/containers/src/Kokkos_DynamicView.hpp:537:5: error: 'is_accessible_from' is not a member template function

     if (m_chunks.template is_accessible_from<host_space>) {

So those need to be fixed too (in addition to the MSVC issue which doesn't like "not" as a keyword.

…anager

containers/src/Kokkos_DynamicView.hpp

lifflander · 2021-07-01T22:02:13Z

retest this please

lifflander · 2021-07-06T22:08:56Z

retest this please

lifflander · 2021-07-07T17:37:50Z

@crtrott I have everything passing. Can you take a final look at the changes I made?

e10harvey

Thanks, @lifflander ! Please see the questions and suggestions below.

e10harvey · 2021-07-29T20:31:04Z

containers/src/Kokkos_DynamicView.hpp

+          nullptr) {
+    using tag_type =
+        typename ChunkedArrayManager<Space, ValueType>::INACCESSIBLE_TAG;
+    return ChunkedArrayManager<Space, ValueType>{tag_type{}, other.m_chunk_max,


Why not use INACCESSIBLE_TAG{} instead?

e10harvey · 2021-07-29T20:36:30Z

containers/src/Kokkos_DynamicView.hpp

+  using value_type   = ValueType;
+  using pointer_type = ValueType*;
+  using track_type   = Kokkos::Impl::SharedAllocationTracker;
+


Suggested change

// 2 extra array elements (per allocation) to keep track of metadata for every allocation

constexpr unsigned n_allocation_counters = 2;

e10harvey · 2021-07-29T20:37:03Z

containers/src/Kokkos_DynamicView.hpp

+  void allocate_device(const std::string& label) {
+    if (m_chunks == nullptr) {
+      m_chunks = reinterpret_cast<pointer_type*>(MemorySpace().allocate(
+          label.c_str(), (sizeof(pointer_type) * (m_chunk_max + 2))));


Suggested change

label.c_str(), (sizeof(pointer_type) * (m_chunk_max + 2))));

label.c_str(), (sizeof(pointer_type) * (m_chunk_max + n_allocation_counters))));

e10harvey · 2021-07-29T20:37:14Z

containers/src/Kokkos_DynamicView.hpp

+  }
+
+  void initialize() {
+    for (unsigned i = 0; i < m_chunk_max + 2; i++) {


Suggested change

for (unsigned i = 0; i < m_chunk_max + 2; i++) {

for (unsigned i = 0; i < m_chunk_max + n_allocation_counters; i++) {

e10harvey · 2021-07-29T20:37:47Z

containers/src/Kokkos_DynamicView.hpp

+      // Destroy the linked allocation if we have one.
+      if (m_linked != nullptr) {
+        Space().deallocate(m_label.c_str(), m_linked,
+                           (sizeof(value_type*) * (m_chunk_max + 2)));


Suggested change

(sizeof(value_type*) * (m_chunk_max + 2)));

(sizeof(value_type*) * (m_chunk_max + n_allocation_counters)));

Should this be (sizeof(value_type*) * (m_chunk_max))?

e10harvey · 2021-07-29T20:40:27Z

containers/src/Kokkos_DynamicView.hpp

+  typename std::enable_if<!IsAccessibleFrom<Space>::value>::type deep_copy_to(
+      ChunkedArrayManager<Space, ValueType> const& other) {
+    Kokkos::Impl::DeepCopy<Space, MemorySpace>(
+        other.m_chunks, m_chunks, sizeof(pointer_type) * (m_chunk_max + 2));


Suggested change

other.m_chunks, m_chunks, sizeof(pointer_type) * (m_chunk_max + 2));

other.m_chunks, m_chunks, sizeof(pointer_type) * (m_chunk_max + n_allocation_counters));

e10harvey · 2021-07-29T20:42:53Z

containers/src/Kokkos_DynamicView.hpp

+  bool m_valid           = false;
+  unsigned m_chunk_max   = 0;
+  pointer_type* m_chunks = nullptr;
+  track_type m_track;


What's this member for?

e10harvey · 2021-07-29T20:44:36Z

containers/src/Kokkos_DynamicView.hpp

@@ -183,7 +355,7 @@ class DynamicView : public Kokkos::ViewTraits<DataType, P...> {
  KOKKOS_INLINE_FUNCTION
  size_t size() const noexcept {
    size_t extent_0 =
-        *reinterpret_cast<const size_t*>(m_chunks + m_chunk_max + 1);
+        *reinterpret_cast<const size_t*>(m_chunks_host + m_chunk_max + 1);


m_chunks_host -> m_chunks?

that was actually a request from me adding the host since this is a host side pointer.

e10harvey · 2021-07-29T20:44:55Z

containers/src/Kokkos_DynamicView.hpp

@@ -173,7 +344,8 @@ class DynamicView : public Kokkos::ViewTraits<DataType, P...> {

  KOKKOS_INLINE_FUNCTION
  size_t allocation_extent() const noexcept {
-    uintptr_t n = *reinterpret_cast<const uintptr_t*>(m_chunks + m_chunk_max);
+    uintptr_t n =
+        *reinterpret_cast<const uintptr_t*>(m_chunks_host + m_chunk_max);


m_chunks_host -> m_chunks?

e10harvey · 2021-07-29T20:48:20Z

containers/src/Kokkos_DynamicView.hpp

-#ifdef KOKKOS_ENABLE_CUDA
-template <>
-struct ChunkArraySpace<Kokkos::CudaSpace> {
-  using memory_space = typename Kokkos::CudaUVMSpace;


This is the removal of UVM?

kokkos#4092: DynamicView: rewrite without UVM

41361c6

lifflander requested a review from crtrott June 28, 2021 19:00

lifflander commented Jun 28, 2021

View reviewed changes

containers/src/Kokkos_DynamicView.hpp Outdated Show resolved Hide resolved

lifflander added 4 commits June 28, 2021 13:37

kokkos#4092: DynamicView: remove unused argument

a460f3b

kokkos#4092: DynamicView: remove unused variable

e1b0a05

kokkos#4092: DynamicView: run clang-format on it

c08bbaf

kokkos#4092: DynamicView: fix some initialize order warnings

ff8bfc9

crtrott requested changes Jun 30, 2021

View reviewed changes

crtrott reviewed Jun 30, 2021

View reviewed changes

containers/src/Kokkos_DynamicView.hpp Outdated Show resolved Hide resolved

lifflander added 7 commits June 30, 2021 11:19

kokkos#4092: DynamicView: change not to ! for mvsc

0859032

kokkos#4092: DynamicView: make Destory an inner class

4311344

kokkos#4092: DynamicView: fix typo in comment

34d50fa

kokkos#4092: DynamicView: add comment for destroy functor

022c451

kokkos#4092: DynamicView: rename SmartMemoryAccessor to ChunkedArrayM…

7da914f

…anager

kokkos#4092: DynamicView: write comment about memory manager

8c076f1

kokkos#4092: DynamicView: re-run formating check and apply changes

d29b327

lifflander requested a review from crtrott June 30, 2021 18:31

lifflander marked this pull request as ready for review June 30, 2021 18:31

crtrott reviewed Jul 1, 2021

View reviewed changes

containers/src/Kokkos_DynamicView.hpp Outdated Show resolved Hide resolved

kokkos#4092: DynamicView: rewrite is accessible trait for nvcc/msvc

9377c65

lifflander requested a review from crtrott July 7, 2021 17:37

crtrott approved these changes Jul 8, 2021

View reviewed changes

lifflander mentioned this pull request Jul 8, 2021

Add test for DynamicView with and without UVM #4149

Open

crtrott added this to In progress in Kokkos Release 3.5 Jul 14, 2021

crtrott moved this from In progress to Awaiting Feedback in Kokkos Release 3.5 Jul 14, 2021

lifflander added this to In progress in Developer: Jonathan Lifflander Jul 14, 2021

e10harvey self-requested a review July 14, 2021 21:29

e10harvey added this to To do in Developer: E10HARVEY Jul 15, 2021

crtrott moved this from Awaiting Feedback to Done in Kokkos Release 3.5 Jul 17, 2021

crtrott moved this from Done to Awaiting Feedback in Kokkos Release 3.5 Jul 17, 2021

lifflander requested a review from dalg24 July 21, 2021 18:11

e10harvey approved these changes Jul 29, 2021

View reviewed changes

Developer: E10HARVEY automation moved this from To do to Reviewer approved Jul 29, 2021

crtrott merged commit d721229 into kokkos:develop Aug 2, 2021

Kokkos Release 3.5 automation moved this from Awaiting Feedback to Done Aug 2, 2021

Developer: E10HARVEY automation moved this from Reviewer approved to Done Aug 2, 2021

lifflander mentioned this pull request Nov 10, 2021

Slow deallocation of DynamicView #4517

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#4092: DynamicView: rewrite without UVM #4129

#4092: DynamicView: rewrite without UVM #4129

lifflander commented Jun 28, 2021

lifflander Jun 28, 2021 •

edited

lifflander Jul 1, 2021

lifflander Jun 28, 2021

lifflander commented Jun 29, 2021

crtrott left a comment

crtrott Jun 30, 2021

lifflander Jun 30, 2021

crtrott commented Jun 30, 2021

lifflander commented Jul 1, 2021

lifflander commented Jul 6, 2021

lifflander commented Jul 7, 2021

e10harvey left a comment

e10harvey Jul 29, 2021

e10harvey Jul 29, 2021

e10harvey Jul 29, 2021

e10harvey Jul 29, 2021

e10harvey Jul 29, 2021

e10harvey Jul 29, 2021

e10harvey Jul 29, 2021

e10harvey Jul 29, 2021

e10harvey Jul 29, 2021

crtrott Jul 30, 2021

e10harvey Jul 29, 2021

e10harvey Jul 29, 2021


	// 2 extra array elements (per allocation) to keep track of metadata for every allocation
	constexpr unsigned n_allocation_counters = 2;

	label.c_str(), (sizeof(pointer_type) * (m_chunk_max + 2))));
	label.c_str(), (sizeof(pointer_type) * (m_chunk_max + n_allocation_counters))));

	for (unsigned i = 0; i < m_chunk_max + 2; i++) {
	for (unsigned i = 0; i < m_chunk_max + n_allocation_counters; i++) {

	(sizeof(value_type) (m_chunk_max + 2)));
	(sizeof(value_type) (m_chunk_max + n_allocation_counters)));

	other.m_chunks, m_chunks, sizeof(pointer_type) * (m_chunk_max + 2));
	other.m_chunks, m_chunks, sizeof(pointer_type) * (m_chunk_max + n_allocation_counters));

#4092: DynamicView: rewrite without UVM #4129

#4092: DynamicView: rewrite without UVM #4129

Conversation

lifflander commented Jun 28, 2021

lifflander Jun 28, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lifflander commented Jun 29, 2021

crtrott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crtrott commented Jun 30, 2021

lifflander commented Jul 1, 2021

lifflander commented Jul 6, 2021

lifflander commented Jul 7, 2021

e10harvey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lifflander Jun 28, 2021 •

edited