Rewrite dash::copy #659

devreal · 2019-07-13T09:52:32Z

This PR addresses #657 and #656 by making dash::copy adhere to the iteration space of the iterators passed. This requires splitting up the copy into transfers to/from contiguous global memory regions.

Copying should now be correct for both global-to-local and local-to-global. Larger (>1 cache line size, maybe better be a pagesize?) local copies are deferred until all remote transfers are kicked off in an attempt to overlap remote and local copies.

For GlobIter and GlobPtr the detection of contiguous memory regions is fairly straightforward, for GlobViewIter (or any view-based iterator) finding boundaries of contiguous regions is in O(n). Ideas on how to improve that are welcome (@fuchsto @dhinf @rkowalewski can the viewspec be used for that?)
All iterators (except for GlobPtr) make use of views to find contiguous memory boundaries,

…are kicked off

bertwesarg

copy_async local-to-global is missing an empty-range check like global-to-local:

  DASH_LOG_TRACE("dash::copy_async()", "async, global to local");
  if (in_first == in_last) {
    DASH_LOG_TRACE("dash::copy_async", "input range empty");
    return dash::Future<ValueType *>(out_first);
  }

But global-to-local has TRACE_LOGs with copy_async_impl in it.

bertwesarg · 2019-07-13T19:44:46Z

dash/include/dash/algorithm/Copy.h

-                                         dest_first,
-                                         handles);
-  }
+  // All elements in input range are remote


does this comment still holds?

bertwesarg · 2019-07-13T19:45:06Z

dash/include/dash/algorithm/Copy.h

-                 out_first,
-                 handles);
-  }
+  // All elements in output range are remote


does this comment still holds? the LOG_TRACE below too

bertwesarg · 2019-07-13T19:48:02Z

dash/include/dash/algorithm/Copy.h

+};
+
+template<typename ValueType>
+void


all other functions have their return type in the same line as the function name

bertwesarg · 2019-07-13T19:50:34Z

dash/include/dash/algorithm/Copy.h

+    if (cur_in.is_local()) {
+      // if the chunk is less than a cache line or if it is the only transfer
+      // don't bother post-poning it
+      if (num_copy_elem == num_elem_total ||


shouldn't this case use std::copy than instead of std::memcpy?

bertwesarg · 2019-07-13T20:01:58Z

dash/include/dash/algorithm/Copy.h

@@ -534,169 +409,18 @@ ValueType * copy(



team and use_memcpy not used anymore

bertwesarg · 2019-07-13T20:04:50Z

dash/include/dash/algorithm/Copy.h

-  ValueType * dest_first = out_first;
-  // Return value, initialize with begin of output range, indicating no
-  // values have been copied:
-  ValueType * out_last   = out_first;
  // Check if part of the input range is local:


comment outdated?

bertwesarg · 2019-07-13T20:06:22Z

dash/include/dash/algorithm/Copy.h

-  typedef typename decltype(pattern)::index_type index_type;
-  typedef typename decltype(pattern)::size_type  size_type;
-  size_type num_elem_total = dash::distance(in_first, in_last);
+  typedef typename GlobInputIt::pattern_type::size_type  size_type;


still using GlobInputIt::pattern_type::size_type instead of just 'GlobInputIt::size_type`

bertwesarg · 2019-07-13T20:09:09Z

dash/include/dash/algorithm/Copy.h

+    auto num_copy_elem = range.second;
+
+    DASH_ASSERT_GT(num_copy_elem, 0,
+                    "Number of element to copy is 0");


Number of elements to copy is 0

bertwesarg · 2019-07-13T20:09:57Z

dash/include/dash/algorithm/Copy.h

+                        const_cast<nonconst_value_type*>(cur_out_first.local());
+      // if the chunk is less than a cache line or if it is the only transfer
+      // don't bother post-poning it
+      if (num_elem_total == num_copy_elem ||


keep the order the same as above: num_copy_elem == num_elem_total

bertwesarg · 2019-07-13T20:21:13Z

dash/include/dash/algorithm/Copy.h

-    out_last = out_first + total_copy_elem;
-  }
-  DASH_LOG_TRACE("dash::copy_async", "preparing future");
+  auto out_last = internal::copy_impl(in_first, in_last, out_first, *handles);


all other calls are dash::internal::copy_impl, please harmonize it

bertwesarg · 2019-07-14T19:57:38Z

This should also fix #657.

codecov · 2019-07-15T07:49:39Z

Codecov Report

Merging #659 into development will increase coverage by 0.24%.
The diff coverage is 96.72%.

@@               Coverage Diff               @@
##           development     #659      +/-   ##
===============================================
+ Coverage        84.83%   85.08%   +0.24%     
===============================================
  Files              335      336       +1     
  Lines            24829    24888      +59     
  Branches         11262    11276      +14     
===============================================
+ Hits             21064    21175     +111     
+ Misses            3741     3692      -49     
+ Partials            24       21       -3

Impacted Files	Coverage Δ
dash/include/dash/iterator/GlobIter.h	`99% <ø> (+2.97%)`	⬆️
dash/test/pattern/CSRPatternTest.cc	`100% <ø> (ø)`	⬆️
dash/include/dash/iterator/GlobViewIter.h	`80.7% <ø> (+0.11%)`	⬆️
dash/include/dash/iterator/internal/GlobPtrBase.h	`90.32% <66.66%> (-0.99%)`	⬇️
dash/include/dash/algorithm/Copy.h	`91.53% <93.75%> (+14.22%)`	⬆️
dash/test/algorithm/CopyTest.cc	`97.64% <98.48%> (+0.18%)`	⬆️
...h/include/dash/iterator/internal/ContiguousRange.h	`98.76% <98.76%> (ø)`
dash/test/TestBase.h	`68.18% <0%> (-22.73%)`	⬇️
dash/include/dash/algorithm/LocalRange.h	`70.58% <0%> (-1.18%)`	⬇️
dash/include/dash/Team.h	`84.53% <0%> (-0.73%)`	⬇️
... and 13 more

bertwesarg · 2019-07-15T08:49:12Z

dash/include/dash/algorithm/Copy.h

+ * to complete. If set to 0, dash::copy will use flush instead to ensure
+ * completion.
+ */
+#if !defined(DASH_COPY_USE_HANDLES)


Where does this came from? How does one control this? Why should someone do not want to use handles?

Using handles is a trade-off: it allows you to wait for specific transfers instead of all transfers (which may be handy if multiple threads call dash::copy or if other transfers are already in flight). As a draw-back, they require memory allocation and might cause noticeable overhead if there many transfers due to short contiguous ranges. I have not done any comparisons, just leaving the option in here to disable the use of handles. You could compile your application using -DDASH_COPY_USE_HANDLES=0 to disable them.

But than, one should be able to make this decision on a call-by-call basis, not all-or-nothing. I'm not convinced, that this is needed right now, so why introduce it with this PR?

Why not? Yes, in general controlling that on a call-by-call basis is preferable. Executing policies have been in the ether for so long, I can only guess when they will [vapor|material]ize. Until then, I don't see any harm in having at least a chance to control it globally. If we find that handle-less std::copy is more efficient (benchmarks welcome), we can easily change the default. The infrastructure is in place.

benchmarks welcome

Why should I argue with benchmarks, for something which you want to introduce without showing benchmarks in the first place?

That was mainly meant as a hint that I only have finite resources at hand to work on this issue (it's gotten out of control already, really there is no such thing as a "quick fix"). I will make it a template parameter and add a benchmark example though

---------------------------------------------------------------------------------- -- Runtime arguments -- -r 20 repetitions per round -- -n 10 rounds -- -s 4096 matrix size (number of double elements per dimension) -- -t 512 tile size (number of double elements per dimension) ---------------------------------------------------------------------------------- units, mpi.impl, impl, total [s], bandwidth [MB/s] 16, craympi, copy_with_handle, 0.05789930, 2318.12350063 16, craympi, copy_without_handle, 0.03827390, 3506.76905149 16, craympi, copy_async, 0.05626080, 2385.63490032 16, craympi, copy_with_handle, 0.05594790, 2398.97704829 16, craympi, copy_without_handle, 0.03835725, 3499.14887016 16, craympi, copy_async, 0.05629005, 2384.39525280 16, craympi, copy_with_handle, 0.05596170, 2398.38546720 16, craympi, copy_without_handle, 0.03835130, 3499.69174448 16, craympi, copy_async, 0.05625060, 2386.06749084 16, craympi, copy_with_handle, 0.05594790, 2398.97704829 16, craympi, copy_without_handle, 0.03837160, 3497.84027771 16, craympi, copy_async, 0.05622025, 2387.35558807 16, craympi, copy_with_handle, 0.05598375, 2397.44082881 16, craympi, copy_without_handle, 0.03835040, 3499.77387459 16, craympi, copy_async, 0.05624890, 2386.13960451 16, craympi, copy_with_handle, 0.05601960, 2395.90657556 16, craympi, copy_without_handle, 0.03834610, 3500.16632721 16, craympi, copy_async, 0.05623290, 2386.81853506 16, craympi, copy_with_handle, 0.05596895, 2398.07478968 16, craympi, copy_without_handle, 0.03835305, 3499.53205808 16, craympi, copy_async, 0.05626840, 2385.31267994 16, craympi, copy_with_handle, 0.05596455, 2398.26332920 16, craympi, copy_without_handle, 0.03836975, 3498.00892630 16, craympi, copy_async, 0.05626480, 2385.46529980 16, craympi, copy_with_handle, 0.05601120, 2396.26588968 16, craympi, copy_without_handle, 0.03837580, 3497.45746017 16, craympi, copy_async, 0.05623485, 2386.73576972 16, craympi, copy_with_handle, 0.05579010, 2405.76245606 16, craympi, copy_without_handle, 0.03836095, 3498.81136937 16, craympi, copy_async, 0.05623960, 2386.53418588

---------------------------------------------------------------------------------- -- Runtime arguments -- -r 20 repetitions per round -- -n 10 rounds -- -s 4096 matrix size (number of double elements per dimension) -- -t 64 tile size (number of double elements per dimension) ---------------------------------------------------------------------------------- units, mpi.impl, impl, total [s], bandwidth [MB/s] 16, craympi, copy_with_handle, 0.46144025, 290.86697140 16, craympi, copy_without_handle, 0.28127320, 477.17922646 16, craympi, copy_async, 0.45664310, 293.92260170 16, craympi, copy_with_handle, 0.45475685, 295.14174003 16, craympi, copy_without_handle, 0.28064115, 478.25391251 16, craympi, copy_async, 0.45654145, 293.98804424 16, craympi, copy_with_handle, 0.45498160, 294.99594709 16, craympi, copy_without_handle, 0.28066755, 478.20892725 16, craympi, copy_async, 0.45660895, 293.94458431

Not using handles clearly has an advantage so I made not using them the default.

dash/include/dash/algorithm/Copy.h

bertwesarg · 2019-07-16T06:50:04Z

dash/include/dash/algorithm/Copy.h

+
+  ContiguousRangeSet<GlobOutputIt> range_set{out_first, out_last};
+
+  std::vector<local_copy_chunk<value_type>> local_chunks;


You are now inconsistent in the use of ValueType and value_type. The former is used below in sizeof

Good point, should be consistent now

bertwesarg · 2019-07-16T06:52:13Z

dash/include/dash/algorithm/Copy.h

+          handles->push_back(handle);
+        }
+      } else {
+        dash::internal::put(dst_gptr, src_ptr, num_copy_elem);


As we cannot overlap memcpy and comm anymore in this case, I think it does not make sense to postpone the memcpy above at all. Thus that condition should include || handles == nullptr.

The overlap is still possible, dash::internal::put is non-blocking

devreal · 2019-07-16T15:10:03Z

CI fails: global-to-local copy breaks under MPICH. I don't see any error for Open MPI, not sure why that fails. I suspect MPICH to have a broken flush_local, needs investigation...

devreal · 2019-07-16T16:25:58Z

I suspect MPICH to have a broken flush_local, needs investigation...

My fault, all checks clear now.

devreal · 2019-07-18T13:05:02Z

@bertwesarg Please check my latest changes and approve if there are not other things to change.

bertwesarg

Fine with me.

devreal added 10 commits July 12, 2019 15:37

Add has_view member to GlobPtr

4c13bf1

Add size_type member to Glob*Iter, forwarded from pattern

9235f03

Fix erroneous nullptr guard in dash::internal::increment

6c97ae3

Revamp dash::copy to respect iteration space, fix local-to-global copy

4538066

Fix GlobViewIter operator<<

f0d5939

Add copy tests for full global <-> local

663183c

Small fixes to ContiguousRange

d14b260

dash::copy: defer copying of local chunks until all remote transfers …

6bf4a92

…are kicked off

Directly copy a chunk if it is the only chunk to process

b7be5be

Add scale-down test-case provided by @bertwesarg

eca3d8f

devreal added bug module:algorithms labels Jul 13, 2019

devreal added this to the dash-0.4.0 milestone Jul 13, 2019

devreal requested review from fuchsto, bertwesarg, rkowalewski and dhinf July 13, 2019 09:52

devreal added 2 commits July 13, 2019 12:49

Use viewspec on all patterns, no need for overload using has_view

cd7e5f1

Remove has_view label from GlobPtr

67a3636

bertwesarg requested changes Jul 13, 2019

View reviewed changes

devreal added 3 commits July 15, 2019 09:44

Copy: use std::copy instead of std::memcpy

50f03b9

Copy: Postpone handling of local transfers if they exceed pagesize

181e524

Copy: cleanup to address review comments

21bc103

devreal added 3 commits July 15, 2019 10:07

Copy: Allow the use of handle-less put/get in dash::copy

732380c

Copy: constify local_copy_chunk

b8a7497

Copy: Remove unnecessary const casts

62b2353

bertwesarg reviewed Jul 15, 2019

View reviewed changes

bertwesarg requested changes Jul 15, 2019

View reviewed changes

dash/include/dash/algorithm/Copy.h Show resolved Hide resolved

bertwesarg requested changes Jul 16, 2019

View reviewed changes

devreal added 6 commits July 16, 2019 11:31

Correctly handle input and output types, make sure they are compatible

d91bcae

Copy: Control selection of handles through template arg

57f51e4

Add copy benchmark

7dfabdc

Fixes to copy benchmark

e20557e

Add async copy benchmark

cc66a80

Make handle-less copy the default

6bac80d

Copy: Use dart_flush_[local_]all

b434901

devreal added 2 commits July 18, 2019 09:14

Copy: Swap order of arguments for DASH_ASSERT_RETURNS

445d598

Merge branch 'development' into bug-656-copy

909438c

bertwesarg approved these changes Jul 18, 2019

View reviewed changes

devreal merged commit d41cb35 into development Jul 18, 2019

This was referenced Jul 18, 2019

dash::copy copies the wrong values in local to global #657

Closed

Infinite loop when using dash::copy #653

Closed

devreal mentioned this pull request Oct 1, 2019

DASH error when using dash::copy with 3D arrays #669

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite dash::copy #659

Rewrite dash::copy #659

devreal commented Jul 13, 2019 •

edited

Loading

bertwesarg left a comment

bertwesarg Jul 13, 2019

bertwesarg Jul 13, 2019

bertwesarg Jul 13, 2019

bertwesarg Jul 13, 2019

bertwesarg Jul 13, 2019

bertwesarg Jul 13, 2019

bertwesarg Jul 13, 2019

bertwesarg Jul 13, 2019

bertwesarg Jul 13, 2019

bertwesarg Jul 13, 2019

bertwesarg commented Jul 14, 2019

codecov bot commented Jul 15, 2019 •

edited

Loading

bertwesarg Jul 15, 2019

devreal Jul 15, 2019

bertwesarg Jul 16, 2019

devreal Jul 16, 2019

bertwesarg Jul 16, 2019

devreal Jul 16, 2019 •

edited

Loading

devreal Jul 16, 2019

bertwesarg Jul 16, 2019

devreal Jul 16, 2019

bertwesarg Jul 16, 2019

devreal Jul 16, 2019

devreal commented Jul 16, 2019

devreal commented Jul 16, 2019

devreal commented Jul 18, 2019

bertwesarg left a comment


		ContiguousRangeSet<GlobOutputIt> range_set{out_first, out_last};

		std::vector<local_copy_chunk<value_type>> local_chunks;

Rewrite dash::copy #659

Rewrite dash::copy #659

Conversation

devreal commented Jul 13, 2019 • edited Loading

bertwesarg 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

bertwesarg commented Jul 14, 2019

codecov bot commented Jul 15, 2019 • edited Loading

Codecov Report

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

devreal Jul 16, 2019 • edited Loading

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

devreal commented Jul 16, 2019

devreal commented Jul 16, 2019

devreal commented Jul 18, 2019

bertwesarg left a comment

Choose a reason for hiding this comment

devreal commented Jul 13, 2019 •

edited

Loading

codecov bot commented Jul 15, 2019 •

edited

Loading

devreal Jul 16, 2019 •

edited

Loading