Major duplicate code removal in SharedAllocationRecord specializations #3658

dhollman · 2020-12-08T19:47:39Z

While working on the View refactor for mdspan, it became clear that there is a massive amount of (relatively technical, hard to understand, hard to recognize as identical) duplicated code across backends with respect to SharedAllocationRecord partial specializations. Since SharedAllocationRecord and SharedAllocationTracker are part of the View refactor, I literally needed to rewrite a bunch of this code in a common place just to determine whether or not it's the same. I'd like to avoid touching individual backends in the actual View refactor, and this is a start towards being able to do this.

This pull request basically just introduces a CRTP base class named SharedAllocationRecordCommon that contains all of the default implementations that had been copy-pasted across backends over the past 10 years.

core/src/impl/Kokkos_SharedAlloc_timpl.hpp

dalg24

Looks good but I am confused how does this not fail at link time this you forgot the explicit instantiation of HostInaccessibleSharedAllocationRecordCommon<SYCLDeviceUSMSpace>?

core/src/impl/Kokkos_ViewTracker.hpp

core/src/impl/Kokkos_SharedAlloc.hpp

dalg24 · 2020-12-09T13:06:01Z

core/src/impl/Kokkos_SharedAlloc.hpp

+ protected:
+  using record_base_t::record_base_t;
+
+  void _fill_host_accessible_header_info(SharedAllocationHeader& arg_header,


I thought we had settled for impl_ prefix

This isn't public. impl_ is for things that need to be public for some reason (usually because friendship with some other library is difficult or impossible) but aren't part of the supported stable public API. Nothing in this class is part of the public API, but there are parts that are only used in this class hierarchy. A common convention for this is to prefix with an underscore, and since we don't really have a uniform convention for this in Kokkos, I just used that here. If you can point to somewhere else where we have a uniform convention for protected/private methods of implementation classes, I'll use that instead; otherwise, I don't see a reason to change this.

I don't buy your argument.

core/src/impl/Kokkos_SharedAlloc_timpl.hpp

core/src/Kokkos_HostSpace.hpp

core/src/SYCL/Kokkos_SYCL_Space.cpp

core/src/HIP/Kokkos_HIP_Space.cpp

core/src/Kokkos_HostSpace.hpp

masterleinad · 2020-12-09T16:48:00Z

core/src/impl/Kokkos_SharedAlloc.hpp

+  static auto allocate(MemorySpace const& arg_space,
+                       std::string const& arg_label, size_t arg_alloc_size)
+      -> derived_t*;


Suggested change

static auto allocate(MemorySpace const& arg_space,

std::string const& arg_label, size_t arg_alloc_size)

-> derived_t*;

static derived_t* allocate(MemorySpace const& arg_space,

std::string const& arg_label, size_t arg_alloc_size);

or is there a reason to defer the return type?

Yes, see my response to your suggestion elsewhere.

This is only true for the definition.

core/src/impl/Kokkos_SharedAlloc.hpp

core/src/impl/Kokkos_SharedAlloc_timpl.hpp

calewis

LGTM, except that I think the string formatting for printing pointers can be simplified by using %p. I might be wrong about that though, but at least in the simple example I checked %p was equivalent to the uintptr_t hex string that you wrote.

If I am missing something or %p is weird on some systems then you can ignore this.

core/src/Cuda/Kokkos_CudaSpace.cpp

calewis · 2020-12-09T18:27:31Z

core/src/Kokkos_SYCL_Space.hpp

+  friend class SharedAllocationRecordCommon<
+      Kokkos::Experimental::SYCLSharedUSMSpace>;
+  using base_t =
+      SharedAllocationRecordCommon<Kokkos::Experimental::SYCLSharedUSMSpace>;
  using RecordBase = SharedAllocationRecord<void, void>;


Should RecordBase be determined from base_t? As far as I can tell we have this because base_t derives from SharedAllocationRecord<void, void>. This is the same in other example, I just picked this one because it was the first one I found while scrolling.

I mostly just avoided doing that to minimize the size of the diff. That said, it's often useful to know the size of the root class of some hierarchy.

What do you mean by size? I just meant something like. using RecordBase = base_t::blahblah.

calewis · 2020-12-09T18:41:02Z

core/src/impl/Kokkos_SharedAlloc_timpl.hpp

+      // Formatting dependent on sizeof(uintptr_t)
+      const char* format_string;
+
+      if (sizeof(uintptr_t) == sizeof(unsigned long)) {


Why can't you just use %p https://godbolt.org/z/vszj3o

Better https://godbolt.org/z/PfG3Eo, also %#x means you don't have to write 0x in front of of the input. But I still think you just want %.12p

Probably, but this is old code and a different issue from the current pull request. I just moved it over from the existing code that was duplicated across several backends. Either way, if we want to change it, we shouldn't do it here.

I'm skeptical of that argument. You just touched the lines and moved them as part of a clean up. I'm not saying you have to fix it, but now blame will imply that you wrote it this way instead of whoever wrote the code you copied and pasted.

masterleinad

This is fine with me. The failing CI checks are unrelated.

nliber · 2020-12-15T23:14:52Z

core/src/HIP/Kokkos_HIP_Space.cpp

@@ -57,6 +51,13 @@
 #include <impl/Kokkos_Error.hpp>
 #include <impl/Kokkos_MemorySpace.hpp>

+#include <stdlib.h>


Any particular reason to not use cstdlib here? (I know it is copy + paste here, but since it is changing anyway...)

nliber · 2020-12-15T23:33:25Z

core/src/Kokkos_HIP_Space.hpp

+  friend class HostInaccessibleSharedAllocationRecordCommon<
+      Kokkos::Experimental::HIPSpace>;
+  using base_t = HostInaccessibleSharedAllocationRecordCommon<
+      Kokkos::Experimental::HIPSpace>;
  using RecordBase = SharedAllocationRecord<void, void>;

  SharedAllocationRecord(const SharedAllocationRecord&) = delete;


General comment: should we do the Rule of 5 (and should the delete functions be public)?

core/src/impl/Kokkos_SharedAlloc.hpp

nliber · 2020-12-15T23:48:22Z

core/src/impl/Kokkos_SharedAlloc.hpp

+  static auto get_record(void* alloc_ptr) -> derived_t*;
+  std::string get_label() const;
+};
+
 namespace {


Doesn't this give us separate deallocate in every translation unit? I'm not sure it matters...

nliber · 2020-12-15T23:55:09Z

core/src/impl/Kokkos_SharedAlloc_timpl.hpp

+  arg_header.m_record = &self();
+
+  strncpy(arg_header.m_label, arg_label.c_str(),
+          SharedAllocationHeader::maximum_label_length);


This can technically be SharedAllocationHeader::maximum_label_length-1, since you always write a \0 in the last byte anyway.

nliber · 2020-12-15T23:57:35Z

core/src/impl/Kokkos_SharedAlloc_timpl.hpp

+            "0x%.12llx + %.8ld ] count(%d) dealloc(0x%.12llx) %s\n";
+      }
+
+      snprintf(buffer, 256, format_string, MemorySpace::execution_space::name(),


Preference for sizeof(buffer) instead of 256, since the two are tied together.

@masterleinad

Based on @masterleinad suggestion. Co-authored-by: Daniel Arndt <arndtd@ornl.gov>

masterleinad · 2021-03-10T13:48:01Z

Retest this please.

dalg24

I am mostly OK with these changes. Please make sure you replay to Nevin's comment

@masterleinad

kokkos#3658) * started cleanup using a CRTP base * Cuda replicates removed * host space migration done * Hip changes for shared alloc cleanup * OpenMPTargetSpace shared alloc cleanup * SYCL shared alloc cleanup * Forgot explicit instantiations in a couple of places * redoing some formatting for a slightly different clang version * introduced HostInaccessibleSharedAllocationRecordCommon * fixed a couple of incorrect default constructors * a few more small bugs in SYCL and OmpTarget * Update to more modern use of (char)0 Based on @masterleinad suggestion. Co-authored-by: Daniel Arndt <arndtd@ornl.gov> * removed CudaSharedAllocationRecordCommon * revert change accidentally included in this changeset * added missing SYCL explicit template instantiation * reverted intel workaround removal * Post-rebase cleanup Co-authored-by: Daniel Arndt <arndtd@ornl.gov>

dhollman force-pushed the shared-alloc-cleanup branch 2 times, most recently from 2e3d69c to 21fb670 Compare December 8, 2020 20:05

masterleinad reviewed Dec 8, 2020

View reviewed changes

core/src/impl/Kokkos_SharedAlloc_timpl.hpp Outdated Show resolved Hide resolved

dhollman requested review from masterleinad, dalg24, calewis and nliber December 8, 2020 22:07

dalg24 reviewed Dec 9, 2020

View reviewed changes

masterleinad reviewed Dec 9, 2020

View reviewed changes

dhollman force-pushed the shared-alloc-cleanup branch 2 times, most recently from 68388cf to 4e4c681 Compare December 9, 2020 18:33

calewis reviewed Dec 9, 2020

View reviewed changes

dhollman requested review from dalg24, masterleinad and calewis December 9, 2020 19:09

masterleinad approved these changes Dec 9, 2020

View reviewed changes

calewis approved these changes Dec 9, 2020

View reviewed changes

nliber approved these changes Dec 16, 2020

View reviewed changes

D. S. Hollman and others added 12 commits March 3, 2021 09:44

started cleanup using a CRTP base

3b1ec60

Cuda replicates removed

f788f69

host space migration done

4d6c8ef

Hip changes for shared alloc cleanup

2c16c42

OpenMPTargetSpace shared alloc cleanup

69423c4

SYCL shared alloc cleanup

b8e4b4d

Forgot explicit instantiations in a couple of places

80a1c1f

redoing some formatting for a slightly different clang version

0df43c5

introduced HostInaccessibleSharedAllocationRecordCommon

5fdbbaf

fixed a couple of incorrect default constructors

6be55b0

a few more small bugs in SYCL and OmpTarget

3428dee

Update to more modern use of (char)0

22609e6

Based on @masterleinad suggestion. Co-authored-by: Daniel Arndt <arndtd@ornl.gov>

D. S. Hollman added 5 commits March 3, 2021 09:44

removed CudaSharedAllocationRecordCommon

997287e

revert change accidentally included in this changeset

6606f5b

added missing SYCL explicit template instantiation

2b404cc

reverted intel workaround removal

09e96de

Post-rebase cleanup

468c9b0

dhollman force-pushed the shared-alloc-cleanup branch from 4e4c681 to 468c9b0 Compare March 9, 2021 23:32

dalg24 reviewed Mar 10, 2021

View reviewed changes

crtrott merged commit 64c0518 into kokkos:develop Mar 10, 2021

ndellingwood mentioned this pull request Mar 23, 2021

Build failures with intel/17 (internal error) #3872

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Major duplicate code removal in SharedAllocationRecord specializations #3658

Major duplicate code removal in SharedAllocationRecord specializations #3658

dhollman commented Dec 8, 2020

dalg24 left a comment

dalg24 Dec 9, 2020

dhollman Dec 9, 2020

dalg24 Mar 10, 2021

masterleinad Dec 9, 2020

dhollman Dec 9, 2020

dalg24 Mar 10, 2021

calewis left a comment

calewis Dec 9, 2020

dhollman Dec 9, 2020

calewis Dec 9, 2020

dalg24 Mar 10, 2021

calewis Dec 9, 2020

calewis Dec 9, 2020 •

edited

dhollman Dec 9, 2020

calewis Dec 9, 2020

masterleinad left a comment

nliber Dec 15, 2020

nliber Dec 15, 2020

nliber Dec 15, 2020

nliber Dec 15, 2020

dalg24 Mar 10, 2021

nliber Dec 15, 2020

dalg24 Mar 10, 2021

masterleinad commented Mar 10, 2021

dalg24 left a comment

Major duplicate code removal in SharedAllocationRecord specializations #3658

Major duplicate code removal in SharedAllocationRecord specializations #3658

Conversation

dhollman commented Dec 8, 2020

dalg24 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

calewis 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

calewis Dec 9, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masterleinad 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

masterleinad commented Mar 10, 2021

dalg24 left a comment

Choose a reason for hiding this comment

calewis Dec 9, 2020 •

edited