Added names of Views to the different View initialize/free kernels #3159

DavidPoliakoff · 2020-07-06T18:39:02Z

Addresses #3070 , in which people want to know which kernels they're spending time initializing and free'ing.

Honestly, I don't much know how Kokkos_ViewMapping.hpp works, I'd appreciate somebody reading over this and letting me know if there is a better implementation.

core/src/impl/Kokkos_ViewMapping.hpp

dalg24 · 2020-07-06T23:20:37Z

core/src/impl/Kokkos_ViewArray.hpp

@@ -395,13 +395,13 @@ class ViewMapping<Traits, Kokkos::Array<> > {
    const size_t alloc_size =
        (m_impl_offset.span() * Array_N * MemorySpanSize + MemorySpanMask) &
        ~size_t(MemorySpanMask);
-
+    const std::string &alloc_name =
+        ((Kokkos::Impl::ViewCtorProp<void, std::string> const &)arg_prop).value;


Avoid C-style cast.

~~I'm kind of confused about why this cast is necessary in the first place.~~ ViewCtorProp is weird, and the presence of this cast raises a red flag for many readers. There should at least be a comment explaining why the cast is here and is like it is.

On further reflection here, I'd probably prefer to have a get_as or get_argument (or something) member function template in ViewCtorProp which make it clear what's going on here rather than having the cast everywhere that it's used and having to explain why there's a cast. I'm pretty okay at C++, know a lot about C++ patterns and idioms, and know Kokkos pretty well, and I had to look this up to figure out what was going on, so I suspect others will have issues also. (I can make this change if you need me to).

also, please use auto here. std::string on the left-hand side of an assignment just screams accidental implicit conversion.

dalg24 · 2020-07-06T23:22:07Z

core/src/impl/Kokkos_ViewMapping.hpp

@@ -3243,13 +3250,13 @@ class ViewMapping<
    const size_t alloc_size =
        (m_impl_offset.span() * MemorySpanSize + MemorySpanMask) &
        ~size_t(MemorySpanMask);
-
+    const std::string& alloc_name =
+        ((Kokkos::Impl::ViewCtorProp<void, std::string> const&)arg_prop).value;


Same comment about C-style casts

How true to the old code do we want to be (I'm ripping from old code here)? I'd like to make it a static_cast, but it started as a C-cast, which is more of a reinterpret_cast. If I make the change, should I change the other C-casts in the function?

I would prefer a reinterpret_cast. You can change all occurrence of (Kokkos::Impl::ViewCtorProp<void, std::string> const&)arg_prop if you agree that this is an improvement.

It's not a reinterpret_cast, it's a static_cast. C spells both of those the same way. This is being used to disambiguate a base class; i.e., I believe the following would also work (though I doubt it would be much clearer for most users), but mostly see my comment above for what I think is a better solution

arg_prop.template ViewCtorProp<void, std::string>::value

Yeah, I've verified that the move to static_cast fixes my bugs

dalg24 · 2020-07-06T23:30:46Z

core/unit_test/CMakeLists.txt

@@ -419,7 +419,7 @@ KOKKOS_ADD_ADVANCED_TEST( UnitTest_PushFinalizeHook_terminate
    KOKKOS_ADD_TEST( NAME ProfilingTestLibraryLoad
      EXE  ProfilingAllCalls
      TOOL printer-tool
-      PASS_REGULAR_EXPRESSION "kokkosp_init_library::kokkosp_allocate_data:${MEMSPACE_REGEX}:source:${ADDRESS_REGEX}:40::kokkosp_begin_parallel_for:Kokkos::View::initialization:0:0::kokkosp_end_parallel_for:0::kokkosp_allocate_data:${MEMSPACE_REGEX}:destination:${ADDRESS_REGEX}:40::kokkosp_begin_parallel_for:Kokkos::View::initialization:0:0::kokkosp_end_parallel_for:0::kokkosp_begin_deep_copy:${MEMSPACE_REGEX}:destination:${ADDRESS_REGEX}:${MEMSPACE_REGEX}:source:${ADDRESS_REGEX}:40::kokkosp_end_deep_copy::kokkosp_begin_parallel_for:parallel_for:${SIZE_REGEX}:0::kokkosp_end_parallel_for:0::kokkosp_begin_parallel_reduce:parallel_reduce:0:1${SKIP_SCRATCH_INITIALIZATION_REGEX}::kokkosp_end_parallel_reduce:1::kokkosp_begin_parallel_scan:parallel_scan:${SIZE_REGEX}:2::kokkosp_end_parallel_scan:2::kokkosp_push_profile_region:push_region::kokkosp_pop_profile_region::kokkosp_create_profile_section:created_section:3::kokkosp_start_profile_section:3::kokkosp_stop_profile_section:3::kokkosp_destroy_profile_section:3::kokkosp_profile_event:profiling_event::kokkosp_deallocate_data:${MEMSPACE_REGEX}:destination:${ADDRESS_REGEX}:40::kokkosp_deallocate_data:${MEMSPACE_REGEX}:source:${ADDRESS_REGEX}:40::kokkosp_finalize_library::"
+      PASS_REGULAR_EXPRESSION "kokkosp_init_library::kokkosp_allocate_data:${MEMSPACE_REGEX}:source:${ADDRESS_REGEX}:40::kokkosp_begin_parallel_for:Kokkos::View::initialization [[]source]:0:0::kokkosp_end_parallel_for:0::kokkosp_allocate_data:${MEMSPACE_REGEX}:destination:${ADDRESS_REGEX}:40::kokkosp_begin_parallel_for:Kokkos::View::initialization [[]destination]:0:0::kokkosp_end_parallel_for:0::kokkosp_begin_deep_copy:${MEMSPACE_REGEX}:destination:${ADDRESS_REGEX}:${MEMSPACE_REGEX}:source:${ADDRESS_REGEX}:40::kokkosp_end_deep_copy::kokkosp_begin_parallel_for:parallel_for:${SIZE_REGEX}:0::kokkosp_end_parallel_for:0::kokkosp_begin_parallel_reduce:parallel_reduce:0:1${SKIP_SCRATCH_INITIALIZATION_REGEX}::kokkosp_end_parallel_reduce:1::kokkosp_begin_parallel_scan:parallel_scan:${SIZE_REGEX}:2::kokkosp_end_parallel_scan:2::kokkosp_push_profile_region:push_region::kokkosp_pop_profile_region::kokkosp_create_profile_section:created_section:3::kokkosp_start_profile_section:3::kokkosp_stop_profile_section:3::kokkosp_destroy_profile_section:3::kokkosp_profile_event:profiling_event::kokkosp_deallocate_data:${MEMSPACE_REGEX}:destination:${ADDRESS_REGEX}:40::kokkosp_deallocate_data:${MEMSPACE_REGEX}:source:${ADDRESS_REGEX}:40::kokkosp_finalize_library::"


Why don't we see the view destruction here?
Also I don't get where the extra square brackets come from in :Kokkos::View::initialization [[]destination]

I'll check on the destruction, that's weird. As to the [[]destination], that's because [abcd] in a regex means "one of a,b,c, or d", so [destination] means one of those letters. The way I know to ask for this character [ is to make it the only character in a set, [[].

I think we don't see View destruction because the type isn't complex, perhaps? I think simple things get freed. @dhollman , is that true?

@dalg24

(@dalg24 's suggestion, thanks) Co-authored-by: Damien L-G <dalg24+github@gmail.com>

…e if they manifest in CI

dhollman

This needs a little attention, and there are definitely some things to think about with respect to consistency with existing code versus changing things we shouldn't have done in the first place, but the general idea is sound.

dhollman · 2020-07-08T17:12:44Z

core/src/impl/Kokkos_ViewArray.hpp

@@ -395,13 +395,13 @@ class ViewMapping<Traits, Kokkos::Array<> > {
    const size_t alloc_size =
        (m_impl_offset.span() * Array_N * MemorySpanSize + MemorySpanMask) &
        ~size_t(MemorySpanMask);
-
+    const std::string &alloc_name =
+        ((Kokkos::Impl::ViewCtorProp<void, std::string> const &)arg_prop).value;


~~I'm kind of confused about why this cast is necessary in the first place.~~ ViewCtorProp is weird, and the presence of this cast raises a red flag for many readers. There should at least be a comment explaining why the cast is here and is like it is.

On further reflection here, I'd probably prefer to have a get_as or get_argument (or something) member function template in ViewCtorProp which make it clear what's going on here rather than having the cast everywhere that it's used and having to explain why there's a cast. I'm pretty okay at C++, know a lot about C++ patterns and idioms, and know Kokkos pretty well, and I had to look this up to figure out what was going on, so I suspect others will have issues also. (I can make this change if you need me to).

dhollman · 2020-07-08T17:14:34Z

core/src/impl/Kokkos_ViewArray.hpp

@@ -395,13 +395,13 @@ class ViewMapping<Traits, Kokkos::Array<> > {
    const size_t alloc_size =
        (m_impl_offset.span() * Array_N * MemorySpanSize + MemorySpanMask) &
        ~size_t(MemorySpanMask);
-
+    const std::string &alloc_name =
+        ((Kokkos::Impl::ViewCtorProp<void, std::string> const &)arg_prop).value;


also, please use auto here. std::string on the left-hand side of an assignment just screams accidental implicit conversion.

dhollman · 2020-07-08T17:42:44Z

core/src/impl/Kokkos_ViewMapping.hpp

@@ -2835,6 +2835,7 @@ struct ViewValueFunctor<ExecSpace, ValueType, false /* is_scalar */> {
  ValueType* ptr;
  size_t n;
  bool destroy;
+  std::string name;


Remember that this gets copied on to the device (well, as bits anyway, not as the copy constructor, so all hell would break loose if we copied ViewValueFunctor on the device at any point or even tried to access the name member). I'm not sure how I feel about having this as a member. Do we have this in other places in Kokkos?

@dhollman, I'll see what testing shows. To be upfront about it, I don't understand the overall structure of how this file works, this is very much a "what mechanisms get testing to pass" kind of PR for me. If you've got any other answers than this one I'd be happy to try it out.

I addressed the other comments, I think, though

Well at the very least, there should be a comment with something like "don't use this on the device because it got bit-copied" or something like that.

This is actually problematic for the SYCL backend because it makes ViewValueFunctor non trivially copyable.

The amount of non-trivially copyable Functors in real code is virtually endless. And Views are still non-trivially copyable aren't they? I am not sure how worried I am about this. But the name of a View is anyway limited in size. We could just stick the same constant size array of char here.

dhollman · 2020-07-08T17:45:16Z

core/src/impl/Kokkos_ViewMapping.hpp

+      std::string functor_name = (destroy ? "Kokkos::View::destruction"
+                                          : "Kokkos::View::initialization");


A bit of a nitpick: I don't like it when people put different types in the second and third positions of the ternary operator. Use auto on the LHS and construct the std::string inside of the ternary operator.

I don't like it when people put different types in the second and third positions of the ternary operator

But that wouldn't compile would it?

It will if they both have implicit decay to the same type; here it's const char[N] and const char[M] decaying to const char*.

Ohhhh, that's fascinating, I didn't pick up on that. Would explicitly constructing a string on each side of the ternary be more comfortable?

dhollman · 2020-07-08T17:47:32Z

core/src/impl/Kokkos_ViewMapping.hpp


  void execute(bool arg) {
    destroy = arg;
    if (!space.in_parallel()) {
+      std::string functor_name = (destroy ? "Kokkos::View::destruction"
+                                          : "Kokkos::View::initialization");
+      functor_name += " [" + name + "]";


Why isn't this just integrated in to the string literals above? Also, besides the fact that it seems like a waste of literal entries in the binary to put it here, this potentially does an extra two allocations, which seems bad.

dhollman · 2020-07-08T17:49:16Z

core/src/impl/Kokkos_ViewMapping.hpp

+      std::string functor_name = (destroy ? "Kokkos::View::destruction"
+                                          : "Kokkos::View::initialization");
+      functor_name += " [" + name + "]";
+


If functor_name is only used inside of the if statement, all of this should be in there as well.

dhollman · 2020-07-08T17:52:32Z

core/src/impl/Kokkos_ViewMapping.hpp

@@ -3243,13 +3250,13 @@ class ViewMapping<
    const size_t alloc_size =
        (m_impl_offset.span() * MemorySpanSize + MemorySpanMask) &
        ~size_t(MemorySpanMask);
-
+    const std::string& alloc_name =
+        ((Kokkos::Impl::ViewCtorProp<void, std::string> const&)arg_prop).value;


It's not a reinterpret_cast, it's a static_cast. C spells both of those the same way. This is being used to disambiguate a base class; i.e., I believe the following would also work (though I doubt it would be much clearer for most users), but mostly see my comment above for what I think is a better solution

arg_prop.template ViewCtorProp<void, std::string>::value

dhollman · 2020-07-08T17:55:11Z

core/src/impl/Kokkos_ViewMapping.hpp


  void construct_shared_allocation() {
    if (!space.in_parallel()) {
-      uint64_t kpID = 0;
+      std::string functor_name = "Kokkos::View::initialization [" + name + "]";


Same comment as above about scoping, only this time I don't see the reason to even declare the variable.

…/kokkos into feature/named-init-kernel

dhollman

LGTM

crtrott

I am putting a hold on it for a second. Does this mean Kokkos::Array() now gets a name?

DavidPoliakoff-backup · 2020-07-09T15:26:10Z

@crtrott if they don't have names we can just revert the changes in ViewArray.hpp, I didn't realize they didn't have one

crtrott

Ok: I figured out what ViewArray is for: views of Kokkos::array so this is correct.

ViewValueFunctor ctor requires a string 4th argument This was added as part of the changes to add names of Views to View initialize/finalize kernels for perf analysis PR kokkos/kokkos#3159

DavidPoliakoff added 4 commits July 6, 2020 11:23

Added names for the different view initialize/free kernels

43cd6cc

...actually saved the name this time

2244afc

Fixed up test to match new naming scheme

b63621b

Fixed up regex

c8e86f4

dalg24 reviewed Jul 6, 2020

View reviewed changes

dalg24 requested a review from dhollman July 6, 2020 23:31

DavidPoliakoff and others added 2 commits July 6, 2020 17:38

Refactor the string data management

c7836c1

(@dalg24 's suggestion, thanks) Co-authored-by: Damien L-G <dalg24+github@gmail.com>

First crack at changing the casts. Getting errors locally, want to se…

06c7a3a

…e if they manifest in CI

dhollman reviewed Jul 8, 2020

View reviewed changes

crtrott added the Blocks Promotion Overview issue for release-blocking bugs label Jul 8, 2020

DavidPoliakoff added 3 commits July 8, 2020 12:00

Fixed up the reinterpret_cast's (thanks @dhollman)

1f52f91

Merge branch 'feature/named-init-kernel' of github.com:DavidPoliakoff…

4c7a85f

…/kokkos into feature/named-init-kernel

More cast fixes

9af681b

dhollman approved these changes Jul 9, 2020

View reviewed changes

crtrott requested changes Jul 9, 2020

View reviewed changes

crtrott approved these changes Jul 9, 2020

View reviewed changes

crtrott merged commit 752ffc8 into kokkos:develop Jul 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added names of Views to the different View initialize/free kernels #3159

Added names of Views to the different View initialize/free kernels #3159

DavidPoliakoff commented Jul 6, 2020

dalg24 Jul 6, 2020

dhollman Jul 8, 2020

dhollman Jul 8, 2020

dalg24 Jul 6, 2020

DavidPoliakoff Jul 6, 2020 •

edited

dalg24 Jul 6, 2020

dhollman Jul 8, 2020

DavidPoliakoff-backup Jul 8, 2020

dalg24 Jul 6, 2020

DavidPoliakoff Jul 6, 2020

DavidPoliakoff-backup Jul 8, 2020

dhollman left a comment

dhollman Jul 8, 2020

dhollman Jul 8, 2020

dhollman Jul 8, 2020

DavidPoliakoff-backup Jul 8, 2020

dhollman Jul 9, 2020

dalg24 Aug 4, 2020

crtrott Aug 4, 2020

dhollman Jul 8, 2020

dalg24 Jul 8, 2020

dhollman Jul 9, 2020

DavidPoliakoff-backup Jul 9, 2020

dhollman Jul 8, 2020

dhollman Jul 8, 2020

dhollman Jul 8, 2020

dhollman Jul 8, 2020

dhollman left a comment

crtrott left a comment

DavidPoliakoff-backup commented Jul 9, 2020

crtrott left a comment

		std::string functor_name = (destroy ? "Kokkos::View::destruction"
		: "Kokkos::View::initialization");

Added names of Views to the different View initialize/free kernels #3159

Added names of Views to the different View initialize/free kernels #3159

Conversation

DavidPoliakoff commented Jul 6, 2020

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

DavidPoliakoff Jul 6, 2020 • edited

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

dhollman 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhollman left a comment

Choose a reason for hiding this comment

crtrott left a comment

Choose a reason for hiding this comment

DavidPoliakoff-backup commented Jul 9, 2020

crtrott left a comment

Choose a reason for hiding this comment

DavidPoliakoff Jul 6, 2020 •

edited