IBAMR / IBSAMRAI2

SAMRAI 2.4.4 fork and associated patches.
Other
1 stars 1 forks source link

Skip overlapping box reset #3

Closed drwells closed 1 year ago

drwells commented 1 year ago

Fixes https://github.com/IBAMR/IBAMR/issues/1562, https://github.com/IBAMR/IBAMR/issues/1563, and https://github.com/IBAMR/IBAMR/issues/1564.

I did have to break the definition of CopyOperation - the original implementation doesn't make much sense (it should take a range, not single elements) so now we can use std::copy(), which assumes that inputs do not overlap.

AFAICT this is all the easy SAMRAI optimizations worth doing that don't significantly impact backwards compatibility. This compiles in C++98 mode and passes the IBAMR test suite so I think it's fine.

boyceg commented 1 year ago

@drwells it also is worth doing some parallel scaling tests to try to smoke out if there are any algorithms that scale poorly as a function of the number of cores. There also was some slowness related to side-centered data that Elijah D discovered when doing that many years ago.

drwells commented 1 year ago

Definitely. I think some of that is due to bad load balancing (which will hopefully get fixed this summer by someone who isn't us :))

b427885 fixes a problem with catastrophically bad performance with the 2D valve benchmark (there we were spending 50% of total runtime computing box overlaps). That algorithm is really expensive and it is run every time we do a linear algebra operation since SAMRAI calls resetLevels() all the time (so we were calling it dozens of times per NSE solve). I didn't look into it that carefully, but that algorithm is at least O(num_patches) and probably worse, so I would expect it to cause bad scaling problems.

boyceg commented 1 year ago

Hrmmm --- that sounds a lot like the algorithm that I thought that we had stripped out already ...

boyceg commented 1 year ago

Hrmmm --- that sounds a lot like the algorithm that I thought that we had stripped out already ...

Could some of the changes from older patches been dropped along the way?

boyceg commented 1 year ago

(Could we make a reference SAMRAI 2.4.4 branch so that we can easily see changes?)

drwells commented 1 year ago

We could, but it's the first commit in this repo so I've been using the hash instead. The script to generate changes is https://github.com/IBAMR/samrai-2.4.4/blob/master/generate-patch.sh which explicitly references that commit.

boyceg commented 1 year ago

Perfect.

drwells commented 1 year ago

I have no idea if changes were dropped at some point (in retrospect we should have set up this repo 5 years ago; oops). That's worth double-checking too.

boyceg commented 1 year ago

I have no idea if changes were dropped at some point (in retrospect we should have set up this repo 5 years ago; oops). That's worth double-checking too.

Perhaps we could make branches that correspond to the existing patches and make sure that they build on each other?

boyceg commented 1 year ago

Nonoverlapping boxes are computed in other places:

% grep nonoverlapping `find . -name "*.h"`
./include/HierarchySideDataOpsInteger.h:   tbox::Array< tbox::Array< hier::BoxList<DIM> > > d_nonoverlapping_side_boxes[DIM];
./include/HierarchySideDataOpsComplex.h:   tbox::Array< tbox::Array< hier::BoxList<DIM> > > d_nonoverlapping_side_boxes[DIM];
./include/HierarchyNodeDataOpsReal.h:   tbox::Array< tbox::Array< hier::BoxList<DIM> > > d_nonoverlapping_node_boxes;
./include/HierarchySideDataOpsReal.h:   tbox::Array< tbox::Array< hier::BoxList<DIM> > > d_nonoverlapping_side_boxes[DIM];
./include/HierarchyFaceDataOpsReal.h:   tbox::Array< tbox::Array< hier::BoxList<DIM> > > d_nonoverlapping_face_boxes[DIM];
./include/HierarchyEdgeDataOpsReal.h:   tbox::Array< tbox::Array< hier::BoxList<DIM> > > d_nonoverlapping_edge_boxes[DIM];
./include/HierarchyFaceDataOpsInteger.h:   tbox::Array< tbox::Array< hier::BoxList<DIM> > > d_nonoverlapping_face_boxes[DIM];
./include/HierarchyNodeDataOpsInteger.h:   tbox::Array< tbox::Array< hier::BoxList<DIM> > > d_nonoverlapping_node_boxes;
./include/HierarchyEdgeDataOpsComplex.h:   tbox::Array< tbox::Array< hier::BoxList<DIM> > > d_nonoverlapping_edge_boxes[DIM];
./include/HierarchyFaceDataOpsComplex.h:   tbox::Array< tbox::Array< hier::BoxList<DIM> > > d_nonoverlapping_face_boxes[DIM];
./include/HierarchyNodeDataOpsComplex.h:   tbox::Array< tbox::Array< hier::BoxList<DIM> > > d_nonoverlapping_node_boxes;
./include/HierarchyEdgeDataOpsInteger.h:   tbox::Array< tbox::Array< hier::BoxList<DIM> > > d_nonoverlapping_edge_boxes[DIM];
drwells commented 1 year ago

Perhaps we could make branches that correspond to the existing patches and make sure that they build on each other?

That was essentially what I did past the first commit (07a9e6c was your changeset to make things work on the M1). I'll double-check that what we have in the second commit in this repo vs. the patch included in https://github.com/IBAMR/IBAMR/releases/tag/v0.3.0 - that patch should be essentially what Elijah wrote plus a few C++ fixes I contributed.

drwells commented 1 year ago

It looks like you figured that out last night and already created the branches!

boyceg commented 1 year ago

I diffed the different patches and it seems like they all make sense.

drwells commented 1 year ago

I need to redo this whole PR now that I deleted every file I changed :)

boyceg commented 1 year ago

Oops.

drwells commented 1 year ago

Everything is now fixed in case you want to take a second look (and IBAMR's test suite passes).

boyceg commented 1 year ago

Looks good. Should we have makeNonOverlappingBoxLists print out a warning the first time it is called? (The current implementation probably should never be used, right?)

drwells commented 1 year ago

I don't think so - we only call it now right before we actually need the results. Anyone calling one of those RMS norm functions should get what they expect without extra messages.