Add configurable capacity UniqueToken for #979. #3051

vbrunini · 2020-05-20T16:43:41Z

Limited to UniqueTokenScope::Instance specializations. Currently
implemented for Serial, OpenMP, and Cuda execution spaces.

@crtrott

dalg24-jenkins · 2020-05-20T16:43:43Z

Can one of the admins verify this patch?

crtrott

This is still failing on HPX. I will work on that, just a reminder not to merge.

core/src/Kokkos_Serial.hpp

core/src/Kokkos_UniqueToken.hpp

dalg24 · 2020-06-22T17:50:54Z

core/src/Kokkos_UniqueToken.hpp

+  using exec_space = ExecutionSpace;
+  using size_type  = typename exec_space::size_type;


I feel like you should deduce these from token_type.

core/src/OpenMP/Kokkos_OpenMP_Exec.hpp

dalg24 · 2020-06-22T20:08:24Z

core/unit_test/TestUniqueToken.hpp

+    bool ok = true;
+
+    ok = ok && 0 <= t;
+    ok = ok && t < tokens.size();


Do these need to be atomic_and?

No: this error check happens in each thread independently.

Limited to UniqueTokenScope::Instance specializations. Currently implemented for Serial, OpenMP, and Cuda execution spaces.

Both per-thread and per-team. Improve the testing of the configurable capacity version of UniqueToken to reflect the token per-team use case using the RAII helper.

Fixed bug in Test for backends with concurrency>1 but max_team_size==1 Also addressed review comments

crtrott · 2020-07-03T03:37:52Z

Ran spotcheck on apollo:

#######################################################
PASSED TESTS
#######################################################
clang-3.9.0-Pthread_Serial-release build_time=66 run_time=687
clang-6.0-Cuda_Pthread-release build_time=109 run_time=575
clang-6.0-OpenMP-release build_time=48 run_time=177
cuda-9.1-Cuda_OpenMP-release build_time=169 run_time=279
gcc-4.8.4-OpenMP-release build_time=50 run_time=366
gcc-4.8.4-Pthread-release build_time=39 run_time=1176
gcc-5.3.0-Serial-release build_time=53 run_time=261
hpx-1.3.0-HPX-release build_time=1362 run_time=543
intel-16.0.1-OpenMP-release build_time=126 run_time=259

crtrott · 2020-07-03T04:46:52Z

Retest this please!

crtrott · 2020-07-03T14:11:48Z

Ah the HIP version is still missing.

dalg24

Did you consider other names for your RAII-wrapper?

core/src/Cuda/Kokkos_Cuda_UniqueToken.hpp

core/src/Kokkos_AcquireUniqueTokenImpl.hpp

core/src/Kokkos_UniqueToken.hpp

core/src/Kokkos_Serial.hpp

dalg24 · 2020-07-06T17:01:04Z

Retest this please

core/src/Kokkos_Serial.hpp

dalg24 · 2020-07-08T12:47:35Z

core/src/Kokkos_HPX.hpp

-  void release(int) const noexcept {}
+  /// \brief create object size for concurrency on the given instance
+  ///
+  /// This object should not be shared between instances


I have trouble parsing this comment. Please improve here and at places it was copied from.

core/src/Kokkos_AcquireUniqueTokenImpl.hpp

vbrunini · 2020-07-08T14:13:20Z

Did you consider other names for your RAII-wrapper?

Just copied what we've been using in our app code, no objections to changing it if you've got an alternative you'd prefer.

nliber · 2020-07-15T03:03:48Z

Like @dalg24, I'm a bit uneasy with splitting the files, because it effectively means it isn't useable w/o #including Kokkos_AcquireUniqueTokenImpl.hpp (because they are functions in a templated type). Putting it in its own header would be much cleaner.

The name isn't all that clear either "Guard" tends to indicate an RAII class, although I'm not sure a name like UniqueTokenGuard quite fits (although I could be wrong).

Minor: I noticed that some of the namespaces are referenced as ::Kokkos::ns and others as Kokkos::ns. Any particular reason? It is unusual to reference them with a preceding ::.

vbrunini · 2020-07-15T05:24:02Z

As a user I would love for Kokkos as a whole to have more headers that can be individually included, but my experience is that almost everything requires just including Kokkos_Core.hpp, and in the past I've been told that is really all Kokkos supports. Given that the existing Kokkos_UniqueToken.hpp only defines the base template and the specializations per execution space are in different headers I think getting to the point where any of this is useable without including Kokkos_Core.hpp is a bigger task?

Personally I think ThreadUniqueToken and TeamUniqueToken might be the most clear names for the RAII helpers, and the existing UniqueToken might be better named UniqueTokenContainer? But there are probably backwards compatibility concerns there?

Some of the existing UniqueToken implementation had the preceding ::Kokkos, I was just trying to be consistent with that. I was unsure if there was a general Kokkos policy about including that to avoid ADL or something?

dalg24

Please

move the declaration/definition of the RAII-wrappers in a separate header instead of the current workaround
consider other names for the RAII-wrappers
fix the typos

…ot of thoughts in

I added comments about the current points of friction in the design

DavidPoliakoff · 2020-07-23T15:00:04Z

core/src/Threads/Kokkos_ThreadsExec.hpp

@@ -677,15 +728,28 @@ class UniqueToken<Threads, UniqueTokenScope::Global> {
  UniqueToken(execution_space const & = execution_space()) noexcept {}

  /// \brief upper bound for acquired values, i.e. 0 <= value < size()
-  inline int size() const noexcept { return Threads::impl_thread_pool_size(); }
+  KOKKOS_INLINE_FUNCTION
+  int size() const noexcept {


I don't like this pattern of KOKKOS_ACTIVE_EXECUTION_MEMORY_SPACE_HOST in the Threads backend, but think it's a problem with our macros, not this implementation. Capturing here #3209

DavidPoliakoff

Other than the comment (which I don't think is primarily about this PR) there are no obvious interface/implementation problems, and the functionality seems well tested

dalg24 · 2020-07-23T15:24:19Z

Retest this please

crtrott force-pushed the configurable_capacity_unique_token branch from 2c9e183 to 9b73f5c Compare June 19, 2020 04:02

crtrott requested changes Jun 22, 2020

View reviewed changes

dalg24 reviewed Jun 22, 2020

View reviewed changes

vbrunini and others added 3 commits July 2, 2020 17:17

Add configurable capacity UniqueToken for kokkos#979.

87bf179

Limited to UniqueTokenScope::Instance specializations. Currently implemented for Serial, OpenMP, and Cuda execution spaces.

Add RAII helpers for acquiring & releasing UniqueToken values.

0916403

Both per-thread and per-team. Improve the testing of the configurable capacity version of UniqueToken to reflect the token per-team use case using the RAII helper.

Fix sizeable UniqueToken for Threads backend and warnings.

9db155a

crtrott force-pushed the configurable_capacity_unique_token branch from 11559c9 to bd5f266 Compare July 2, 2020 23:21

Add HPX version of size limited unique token

97c8d1b

Fixed bug in Test for backends with concurrency>1 but max_team_size==1 Also addressed review comments

crtrott force-pushed the configurable_capacity_unique_token branch from bd5f266 to 97c8d1b Compare July 2, 2020 23:44

dalg24 reviewed Jul 6, 2020

View reviewed changes

core/src/Cuda/Kokkos_Cuda_UniqueToken.hpp Show resolved Hide resolved

core/src/Kokkos_AcquireUniqueTokenImpl.hpp Show resolved Hide resolved

core/src/Kokkos_UniqueToken.hpp Outdated Show resolved Hide resolved

core/src/Kokkos_Serial.hpp Outdated Show resolved Hide resolved

Update HIP UniqueToken specializations to enable configurable capacity

57e440a

crtrott approved these changes Jul 7, 2020

View reviewed changes

dalg24 reviewed Jul 8, 2020

View reviewed changes

core/src/Kokkos_Serial.hpp Outdated Show resolved Hide resolved

dalg24 reviewed Jul 8, 2020

View reviewed changes

masterleinad self-requested a review July 8, 2020 12:56

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

dalg24 requested a review from nliber July 14, 2020 04:32

dalg24 previously requested changes Jul 15, 2020

View reviewed changes

dalg24 self-assigned this Jul 22, 2020

dalg24 and others added 3 commits July 22, 2020 17:46

Apply suggestions from code review (fix typos)

e9b274e

Merge branch 'develop' into configurable_capacity_unique_token

839e329

Comment about the circular dependency issues in UniqueToken

4095ac0

Add a note about AcquireUniqueToken not being a name that was put a l…

7019e6d

…ot of thoughts in

DavidPoliakoff reviewed Jul 23, 2020

View reviewed changes

DavidPoliakoff approved these changes Jul 23, 2020

View reviewed changes

crtrott merged commit 43b7756 into kokkos:develop Jul 23, 2020

vbrunini deleted the configurable_capacity_unique_token branch July 23, 2020 18:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add configurable capacity UniqueToken for #979. #3051

Add configurable capacity UniqueToken for #979. #3051

vbrunini commented May 20, 2020

dalg24-jenkins commented May 20, 2020

crtrott left a comment

dalg24 Jun 22, 2020

crtrott Jul 2, 2020

dalg24 Jun 22, 2020

crtrott Jul 2, 2020

crtrott commented Jul 3, 2020

crtrott commented Jul 3, 2020

crtrott commented Jul 3, 2020

dalg24 left a comment

dalg24 commented Jul 6, 2020

dalg24 Jul 8, 2020

vbrunini commented Jul 8, 2020

nliber commented Jul 15, 2020

vbrunini commented Jul 15, 2020

dalg24 left a comment

DavidPoliakoff Jul 23, 2020

DavidPoliakoff left a comment

dalg24 commented Jul 23, 2020

		using exec_space = ExecutionSpace;
		using size_type = typename exec_space::size_type;

Add configurable capacity UniqueToken for #979. #3051

Add configurable capacity UniqueToken for #979. #3051

Conversation

vbrunini commented May 20, 2020

dalg24-jenkins commented May 20, 2020

crtrott left a comment

Choose a reason for hiding this comment

dalg24 Jun 22, 2020

Choose a reason for hiding this comment

crtrott Jul 2, 2020

Choose a reason for hiding this comment

dalg24 Jun 22, 2020

Choose a reason for hiding this comment

crtrott Jul 2, 2020

Choose a reason for hiding this comment

crtrott commented Jul 3, 2020

crtrott commented Jul 3, 2020

crtrott commented Jul 3, 2020

dalg24 left a comment

Choose a reason for hiding this comment

dalg24 commented Jul 6, 2020

dalg24 Jul 8, 2020

Choose a reason for hiding this comment

vbrunini commented Jul 8, 2020

nliber commented Jul 15, 2020

vbrunini commented Jul 15, 2020

dalg24 left a comment

Choose a reason for hiding this comment

DavidPoliakoff Jul 23, 2020

Choose a reason for hiding this comment

DavidPoliakoff left a comment

Choose a reason for hiding this comment

dalg24 commented Jul 23, 2020