Buffer-pack-in-one #359

pgrete · 2020-11-11T20:36:44Z

PR Summary

WIP to have a central place to keep track of performance numbers.

PR Checklist

Code passes cpplint
New features are documented.
Adds a test for any bugs fixed. Adds tests for new features.
Code is formatted
Changes are summarized in CHANGELOG.md

Open questions

Are we still using vbvar->var_cc extensively or can than functionality go as it it effectively absorbed by VariablePacks`?

Yurlungur

Our local resources are down for maintenance at the moment. But when we're back up, we'll try this with riot.

src/bvals/cc/bvals_cc_in_one.cpp

src/parthenon_manager.cpp

AndrewGaspar · 2021-01-19T19:49:48Z

@pgrete I'm in the middle of reviewing this. will publish soon.

AndrewGaspar · 2021-01-19T16:47:22Z

src/reconstruct/dc_inline.hpp

-                 ScratchPad2D<Real> &ql, ScratchPad2D<Real> &qr) {
+template <typename T>
+KOKKOS_FORCEINLINE_FUNCTION void
+DonorCellX1(parthenon::team_mbr_t const &member, const int k, const int j, const int il,


It's not clear to me why this was made a template. If it's for an external code, that's fine, but I don't see any motivation in this PR myself. I think the only place this is called is in advection_package.cpp, and it doesn't look like the types used there changed at all.

If it's for an external code, just put a comment in the PR explaining why this was changed.

This was actually pulled in from #365 (this branch here has been active for so long I fixed a couple of things along the go to make things work downstream).
Would you prefer that I update #365 to just include those reconstruction changes, get that merged in, and then update this branch
or
add another line in the changelog saying this happenend here, merge this branch and close #365 ?

I now updated the changelog here to also reflect this change (as well as the "un-/pack" -> "pack and unpack" change you requested).
@AndrewGaspar If you're happy feel free to merge (or schedule for merge once the pipeline passes).

AndrewGaspar · 2021-01-19T16:48:30Z

src/reconstruct/plm_inline.hpp

-                       ScratchPad2D<Real> &dqm) {
+template <typename T>
+KOKKOS_INLINE_FUNCTION void
+PiecewiseLinearX1(parthenon::team_mbr_t const &member, const int k, const int j,


Same comment as on DonorCell

src/parthenon_manager.cpp

AndrewGaspar · 2021-01-19T21:54:58Z

CHANGELOG.md

 - [[PR 400]](https://github.com/lanl/parthenon/pull/400) Extend `StateDescriptor` for customizable output via user-customizable function pointers `PreStepDiagnosticsMesh` and `PostStepDiagnosticsMesh`
 - [[PR 391]](https://github.com/lanl/parthenon/pull/391) Add `VariablePack<T>::GetSparseId` and `VariablePack<T>::GetSparseIndex` to return global sparse ids and pack-local sparse index, repsectively.
 - [[PR 381]](https://github.com/lanl/parthenon/pull/381) Overload `DataCollection::Add` to build `MeshData` and `MeshBlockData` objects with a subset of variables.
 - [[PR 378]](https://github.com/lanl/parthenon/pull/378) Add Kokkos profiling regions throughout the code to allow the collection characteristic application profiles
+- [[PR 359]](https://github.com/lanl/parthenon/pull/359) MeshBlockPack support for buffer un-/pack of CellCentered Variables


"un-/pack" - took me a second to understand what this was trying to say... probably better as

Suggested change

- [[PR 359]](https://github.com/lanl/parthenon/pull/359) MeshBlockPack support for buffer un-/pack of CellCentered Variables

- [[PR 359]](https://github.com/lanl/parthenon/pull/359) MeshBlockPack support for buffer pack and unpack of CellCentered Variables

pgrete · 2021-01-21T18:16:48Z

With respect to getting this PR merged, would you @AndrewGaspar @Yurlungur like to keep the option to switch between MeshBlock and MeshBlockPack in the advection example or should I remove it?
I'm in favor of keeping it for now and update once the proxy app is ready so that we have one example using MeshBlock based version and another (separate) example using the MeshBlockPack based versions.

Also, @Yurlungur did you get the chance to test this downstream for more than one Variable in riot or shall we merge in good faith for now and fix if there's an issue later? I verified that it works in principle by just adding another independent variable, so MPI signaling seems to work, but I didn't test for correctness (but I fairly optimistic that it is) .
I'm in favor of merging now so that we can continue to work (on all the other open issues) on "clean" branches.

Yurlungur · 2021-01-21T18:53:37Z

With respect to getting this PR merged, would you @AndrewGaspar @Yurlungur like to keep the option to switch between MeshBlock and MeshBlockPack in the advection example or should I remove it?
I'm in favor of keeping it for now and update once the proxy app is ready so that we have one example using MeshBlock based version and another (separate) example using the MeshBlockPack based versions.

Agreed, let's keep the switch for now and remove it once @jlippuner has the proxy app working.

Also, @Yurlungur did you get the chance to test this downstream for more than one Variable in riot or shall we merge in good faith for now and fix if there's an issue later? I verified that it works in principle by just adding another independent variable, so MPI signaling seems to work, but I didn't test for correctness (but I fairly optimistic that it is) .
I'm in favor of merging now so that we can continue to work (on all the other open issues) on "clean" branches.

Apologies, @pgrete I was sick last week, so didn't get a chance to do this yet. Let's go ahead and merge and I will push on the riot version and engage with you if things don't work.

Yurlungur · 2021-01-21T18:54:47Z

src/bvals/cc/bvals_cc.cpp

-        ek -= pmb->block_size.nx3 / 2 - pmb->cnghost;
-    }
-  }
+  cell_centered_bvars::CalcIndicesLoadToFiner(si, ei, sj, ej, sk, ek, nb, pmb.get());


Yurlungur · 2021-01-21T18:55:05Z

src/bvals/cc/bvals_cc.cpp

-  auto CalcIndices = [](int ox, int &s, int &e, const IndexRange &bounds) {
-    if (ox == 0) {
-      s = bounds.s;
-      e = bounds.e;
-    } else if (ox > 0) {
-      s = bounds.e + 1;
-      e = bounds.e + NGHOST;
-    } else {
-      s = bounds.s - NGHOST;
-      e = bounds.s - 1;
-    }
-  };
-
  IndexDomain interior = IndexDomain::interior;
-  CalcIndices(nb.ni.ox1, si, ei, cellbounds.GetBoundsI(interior));
-  CalcIndices(nb.ni.ox2, sj, ej, cellbounds.GetBoundsJ(interior));
-  CalcIndices(nb.ni.ox3, sk, ek, cellbounds.GetBoundsK(interior));
+  cell_centered_bvars::CalcIndicesSetSame(nb.ni.ox1, si, ei,
+                                          cellbounds.GetBoundsI(interior));
+  cell_centered_bvars::CalcIndicesSetSame(nb.ni.ox2, sj, ej,
+                                          cellbounds.GetBoundsJ(interior));
+  cell_centered_bvars::CalcIndicesSetSame(nb.ni.ox3, sk, ek,
+                                          cellbounds.GetBoundsK(interior));


Yurlungur · 2021-01-21T18:58:06Z

src/bvals/cc/bvals_cc_in_one.cpp

+    for (auto &v : rc->GetCellVariableVector()) {
+      if (v->IsSet(parthenon::Metadata::FillGhost)) {
+        for (int n = 0; n < pmb->pbval->nneighbor; n++) {
+          parthenon::NeighborBlock &nb = pmb->pbval->neighbor[n];
+          auto *pbd_var_ = v->vbvar->GetPBdVar();
+
+          auto &si = boundary_info_h(b).si;
+          auto &ei = boundary_info_h(b).ei;
+          auto &sj = boundary_info_h(b).sj;
+          auto &ej = boundary_info_h(b).ej;
+          auto &sk = boundary_info_h(b).sk;
+          auto &ek = boundary_info_h(b).ek;
+          auto &Nv = boundary_info_h(b).Nv;
+          Nv = v->GetDim(4);
+
+          if (nb.snb.level == mylevel) {
+            const parthenon::IndexShape &cellbounds = pmb->cellbounds;
+            CalcIndicesSetSame(nb.ni.ox1, si, ei, cellbounds.GetBoundsI(interior));
+            CalcIndicesSetSame(nb.ni.ox2, sj, ej, cellbounds.GetBoundsJ(interior));
+            CalcIndicesSetSame(nb.ni.ox3, sk, ek, cellbounds.GetBoundsK(interior));
+            boundary_info_h(b).var = v->data.Get<4>();
+          } else if (nb.snb.level < mylevel) {
+            const IndexShape &c_cellbounds = pmb->c_cellbounds;
+            const auto &cng = pmb->cnghost;
+            CalcIndicesSetFromCoarser(nb.ni.ox1, si, ei,
+                                      c_cellbounds.GetBoundsI(interior), pmb->loc.lx1,
+                                      cng, true);
+            CalcIndicesSetFromCoarser(nb.ni.ox2, sj, ej,
+                                      c_cellbounds.GetBoundsJ(interior), pmb->loc.lx2,
+                                      cng, pmb->block_size.nx2 > 1);
+            CalcIndicesSetFromCoarser(nb.ni.ox3, sk, ek,
+                                      c_cellbounds.GetBoundsK(interior), pmb->loc.lx3,
+                                      cng, pmb->block_size.nx3 > 1);
+
+            boundary_info_h(b).var = v->vbvar->coarse_buf.Get<4>();
+          } else {
+            CalcIndicesSetFromFiner(si, ei, sj, ej, sk, ek, nb, pmb.get());
+            boundary_info_h(b).var = v->data.Get<4>();
+          }
+          boundary_info_h(b).buf = pbd_var_->recv[nb.bufid];
+          // safe to set completed here as the kernel updating all buffers is
+          // called immediately afterwards
+          pbd_var_->flag[nb.bufid] = parthenon::BoundaryStatus::completed;
+          b++;
+        }
+      }
+    }
+  }


I'm fairly confident this should work for multiple variables.

Reintroduce buffer pack in one fucs

c4dfc1e

pgrete added the performance label Nov 11, 2020

pgrete and others added 18 commits November 11, 2020 22:18

Add profiling regions to bvals

66512db

Perf fix

5f30250

Perf fix

1d14a2c

template inline reconstruction functions

1c2f922

Merge branch 'pgrete/fix-perf-regression' into pgrete/buffer-in-one

03d7108

remove noexcept again

0c20215

Merge branch 'pgrete/fix-perf-regression' into pgrete/buffer-in-one

7e14631

Update Changelog

1828a20

Merge branch 'develop' into pgrete/fix-perf-regression

33c84ab

Merge branch 'pgrete/fix-perf-regression' into pgrete/buffer-in-one

df494f3

Merge branch 'develop' into pgrete/buffer-in-one

60d99b9

Add flags to control buffer packing

74460ac

Fix pkg name

43a7880

Add assert statement for single var per MeshBlock

dc01d61

Add var view to bnd struct

663a4f1

BufferSet works for AMR

6799cef

Use reference for bnd info indices

be4c0fd

Use inner_loop_pattern in buffer set

c7d4692

Yurlungur mentioned this pull request Nov 20, 2020

Do the remaining work to thread the new boundary machinery through the MeshData and MeshBlockData machinery #376

Open

pgrete added 9 commits November 21, 2020 16:16

Add perf comment

8caceab

Merge branch 'develop' into pgrete/buffer-in-one

165aa09

Reuse index calculation in SetBoundary routines

0fe1811

Streamline region names

5e06847

Receive MeshBlockData Receive function for MeshData

6facf55

Separate idx calc in LoadBoundary calls

37a3891

AMR LoadBuffers with MeshData support

3713709

Merge branch 'develop' into pgrete/buffer-in-one

684401b

Only use buffers used for send buffers

626e040

pgrete added 6 commits December 30, 2020 11:41

Fix send boundary_info caching

5f873af

Disentable functions and document

0cde4e2

Add MPI check to buffer_in_one call

1365e16

Fix formatting

287468a

allow disabling of allocation tracking

c4dbbb9

Merge remote-tracking branch 'origin/develop' into pgrete/buffer-in-one

3775b6b

Yurlungur reviewed Jan 7, 2021

View reviewed changes

src/bvals/cc/bvals_cc_in_one.cpp Outdated Show resolved Hide resolved

src/bvals/cc/bvals_cc_in_one.cpp Show resolved Hide resolved

src/bvals/cc/bvals_cc_in_one.cpp Show resolved Hide resolved

src/parthenon_manager.cpp Show resolved Hide resolved

Move comm buffers to host pinned space

a6e941c

pgrete force-pushed the pgrete/buffer-in-one branch from 14423bf to dcb2f4f Compare January 12, 2021 20:17

Merge branch 'pgrete/host-pinned-buffers' into pgrete/buffer-in-one

9e5f3d0

pgrete force-pushed the pgrete/buffer-in-one branch from dcb2f4f to 9e5f3d0 Compare January 12, 2021 20:19

This was referenced Jan 18, 2021

AMR performance 3/N: Prolongation/restriction still uses a kernel per buffer approach #417

Open

AMR performance 4/N: Reorder buffer routine by "distance" #418

Closed

AMR performance 6/N: Optimal execution policy for buffer filling kernels #420

Open

pgrete added 3 commits January 18, 2021 18:41

Merge branch 'pgrete/host-pinned-buffers' into pgrete/buffer-in-one

4f5fd81

Rename pointer var

2bdd99c

Fix multi variable buffer in one

efa0516

AndrewGaspar approved these changes Jan 19, 2021

View reviewed changes

pgrete mentioned this pull request Jan 21, 2021

AMR performance 8/N: Combine "Variable" communication buffer #427

Open

Yurlungur approved these changes Jan 21, 2021

View reviewed changes

Yurlungur reviewed Jan 21, 2021

View reviewed changes

pgrete added 2 commits January 21, 2021 20:45

Merge remote-tracking branch 'origin/develop' into pgrete/buffer-in-one

3ba4f5f

Fix changelog

a676709

AndrewGaspar merged commit a8505cf into develop Jan 21, 2021

AndrewGaspar deleted the pgrete/buffer-in-one branch January 21, 2021 22:59

pgrete mentioned this pull request Jan 22, 2021

WIP: Fix performance regression #365

Closed

5 tasks

JoshuaSBrown mentioned this pull request Feb 1, 2021

Bug in Regression tests #433

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Buffer-pack-in-one #359

Buffer-pack-in-one #359

pgrete commented Nov 11, 2020 •

edited

Loading

Yurlungur left a comment

AndrewGaspar commented Jan 19, 2021

AndrewGaspar Jan 19, 2021

pgrete Jan 21, 2021

pgrete Jan 21, 2021

AndrewGaspar Jan 19, 2021

AndrewGaspar Jan 19, 2021

pgrete commented Jan 21, 2021 •

edited

Loading

Yurlungur commented Jan 21, 2021

Yurlungur Jan 21, 2021

Yurlungur Jan 21, 2021

Yurlungur Jan 21, 2021

	- [[PR 359]](https://github.com/lanl/parthenon/pull/359) MeshBlockPack support for buffer un-/pack of CellCentered Variables
	- [[PR 359]](https://github.com/lanl/parthenon/pull/359) MeshBlockPack support for buffer pack and unpack of CellCentered Variables

Buffer-pack-in-one #359

Buffer-pack-in-one #359

Conversation

pgrete commented Nov 11, 2020 • edited Loading

PR Summary

PR Checklist

Open questions

Yurlungur left a comment

Choose a reason for hiding this comment

AndrewGaspar commented Jan 19, 2021

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

pgrete commented Jan 21, 2021 • edited Loading

Yurlungur commented Jan 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgrete commented Nov 11, 2020 •

edited

Loading

pgrete commented Jan 21, 2021 •

edited

Loading