Break the Manifold interface for performance. #4723

drwells · 2017-08-08T16:42:55Z

This is the branch which I have mentioned in passing a few times: we spend nearly a third of our time allocating and freeing memory inside of the Manifold functions called when creating a grid.

I am not sure if we want to proceed with this and there are still a few things that need to be fixed:

The minimum boost version needs to be increased to ~~1.58~~ 1.59 (update: 1.58 is not compatible with deal.II)
The dimension-dependent functions for returning the number of default points in each direction are pretty ugly.
The Manifolds::get_default_points_and_weights function could also be improved.

In addition, I included a commit that adds a new constructor to ArrayView which creates an ArrayView from a std::vector: this allows all tests but one to pass in their current form.

In reference to #4704.

tjhei · 2017-08-08T16:56:05Z

/no-longer-run-tests (I am trying out if the tester gcc-serial is running on demand now)

bangerth · 2017-08-08T17:43:47Z

Please submit the ArrayView work with a testcase in a separate PR -- this could be merged independently.

What new functionality of BOOST do you need for the current work?

drwells · 2017-08-08T18:04:25Z

I only added the new ArrayView constructor so that it is possible to call the modified functions with their old arguments. I think it is worth discussing whether or not we should permit that (or make the break even bigger) before moving it to a separate PR.

Regardless of this patch: are people OK with adding a new conversion constructor like this one? I am a bit on the fence as to whether or not we should automatically convert std::vectors into ArrayViews.

The only new boost functionality that I require it boost::container::small_vector which is new in 1.58.

bangerth · 2017-08-08T18:06:20Z

I think we found ArrayView useful, so making more uses possible seems like the right direction. I would advocate making conversion operators explicit to avoid the automatic conversion.

Updating boost requirements is a different issue, of course...

kronbichler

I'm very much in favor of changing this because we really do way too many memory allocations in those places. I have a few comments regarding one of the functions and would suggest to choose larger sizes of around 200 for the small vectors.

By the way, it is not very difficult to work around the boost vector: We use a variable of size [200] on the stack unconditionally. If it fits, we put the array view on that data type. If not, we put it into an std::vector. That would maybe be 8x5 more lines of code. I don't know about the usage in manifold.h, though.

kronbichler · 2017-08-08T18:16:28Z

include/deal.II/grid/manifold.h

+  constexpr int n_default_points_per_cell<3>()
+  {
+    return GeometryInfo<3>::vertices_per_cell + GeometryInfo<3>::lines_per_cell
+           + GeometryInfo<3>::faces_per_cell;


Why not simply returning 3^dim-1?

I.e., use Utilities::fixed_int_power<3,dim>::value-1

I would prefer to write it out this way so that it is clear what the number signifies.

kronbichler · 2017-08-08T18:51:01Z

source/grid/manifold.cc

+  const std::size_t n_points = surrounding_points.size();
+  // TODO find a better dimension-dependent estimate for the size of this
+  // vector
+  boost::container::small_vector<double, 20> local_weights(n_points);


I'm not sure we know that. It really depends on the degree of interpolation (or whatever comes in here). In the end, the allocation is a one-time cost that must be paid off by enough operations. I would have chosen a much larger value here, maybe 200 or 500.

kronbichler · 2017-08-08T18:54:35Z

source/grid/manifold.cc

+      // The older version used surrounding_points
+      new_points[row] = project_to_manifold(make_array_view(surrounding_points.begin(),
+                                                            surrounding_points.end()),
+                                            new_point);


I would use surrounding_points_start but it might be that project_to_manifold does the necessary adjustments internally. Do we have tests that trigger on one or the other?

I am not sure; it has been long enough since I wrote this that I don't even really remember the issue. I will look at this with @luca-heltai at some point during the next few days.

I believe that the answer is 'yes' and our current implementation is wrong. At the same time, we do not use the surrounding points in FlatManifold::project_to_manifold (which is another bug in itself; this function should not be the identity function for structdim != spacedim) so this is really a separate problem.

drwells · 2017-08-11T04:59:11Z

@kronbichler I have made some of the preliminary changes (and I adopted the little optimization you added in a commit to #2418). I will address things more fully once #2418 is merged.

bangerth · 2017-08-28T22:34:27Z

Just following up -- what's the status of this patch right now?

drwells · 2017-08-28T23:56:01Z

@bangerth What we have now works but there are still some clean-ups I would like to do before merging (see the TODO list at the top of the PR). These are closely related to @kronbichler's criticisms.

I haven't spent much time of this recently; I'd like to finish #4798 before returning to this.

drwells · 2017-09-05T00:14:25Z

The first part of this patch has been moved to #5024.

drwells · 2017-09-06T15:50:52Z

I would prefer to merge #4798 before we get to this but I will write more patches for this PR this week.

drwells · 2017-09-07T01:46:03Z

I addressed most of the review comments and made a few other cosmetic changes. I believe that this is ready to be reviewed again. Here are the main points:

I kept, for now, the implicit conversion constructor from a std::vector to an ArrayView. I added this purely for compatibility: users can still call the old functions with std::vectors and things will work. We can make the break even bigger, if we want, by removing this (I would prefer to do that; I don't like implicit conversion constructors).
I changed the interface to TransfiniteInterpolationManifold::compute_chart_points to take a mutable ArrayView as an argument and return a cell iterator. This is not as pretty as the old version (which returns return values instead of populating a provided array) but this approach works better with ArrayView. This change aligns with my philosophy 'small_vector is a memory management detail and should never be in an API'.
I think that the implementation of get_default_points_and_weights is improved but I would like to hear feedback on it. Perhaps this should be an internal function? How to do this correctly came up in the last review.

After reviewing this and #4798 I don't think it matters which is merged first; this one is probably easier to understand.

kronbichler

I had a look through the patch and it looks great. I only have a minor comment. I'm looking forward to using this.

kronbichler · 2017-09-07T12:51:44Z

include/deal.II/grid/manifold.h

-                 const std::vector<double>           &weights) const;
+  get_new_point (const ArrayView<const Point<spacedim>> &surrounding_points,
+                 const ArrayView<const double>          &weights) const;
+


Please remove one empty line (we use one line distance between function declarations).

kronbichler · 2017-09-07T15:09:35Z

I think the get_default_points_and_weights function makes sense. It is a bit longish to read but the names are really descriptive so I like it.

kronbichler · 2017-09-07T18:07:14Z

/run-tests

kronbichler · 2017-09-08T08:38:59Z

Are there any further comments? If nobody objects, I will merge this PR in a day or two.

drwells · 2017-09-08T15:36:08Z

If no one feels strongly about preserving the compatibility shim then I will remove the implicit conversion constructor from std::vector to ArrayView. In addition to deleting 511f57e I will also have to go back and fix a few dozen tests (which expect the old interface).

drwells · 2017-09-11T14:11:34Z

I have made the requested changes (including the changelog entry).

bangerth

Just two minor comments about the changelog. please fix and merge yourself!

Nice work!

bangerth · 2017-09-11T14:18:41Z

doc/news/changes/incompatibilities/20170910DavidWells

+  <li>Manifold::project_to_manifold</li>
+</ol>
+have had their declarations changed in an incompatible manner: these three
+methods now take arguments that are ArrayViews instead of


well, it's actually only two methods :-)

If you write it as "ArrayView objects", then doxygen will link to the class correctly.

bangerth · 2017-09-11T14:19:48Z

doc/news/changes/incompatibilities/20170910DavidWells

+eliminates this allocation cost.
+
+The method <code>Manifold::add_new_points</code> has been removed in favor of
+<code>Manifold::get_new_points</code>, which also uses ArrayViews instead of


same here

you don't need to use <code> for `Manifold::get_new_points -- doxygen will automatically print it with tt font because it knows about it.

... when using Manifold::get_new_points() (opening and closing parenthesis).

tamiko · 2017-09-11T14:30:40Z

@drwells Please merge after updating the changelog.

Looks good!

drwells · 2017-09-11T16:15:16Z

Clang is not happy with the internal function: perhaps it does not like the = delete syntax.

bangerth · 2017-09-11T16:25:16Z

Ah, what a shame.

I think you can generally define the function as

  template <int dim>
  uint ()
  {
  return
     GeometryInfo<dim>::vertices_per_cell + 
     GeometryInfo<dim>::lines_per_cell +
     GeometryInfo<dim>::quads_per_cell  +
     GeometryInfo<dim>::hexes_per_cell 
     - 1;   // don't count the cell itself, just the bounding objects

Things like quads_per_cell should be zero in dim == 1.

drwells · 2017-09-11T16:36:49Z

I cannot locally reproduce the clang problem; I suspect that they have since fixed this bug. I'll upload the fixed version in a moment.

drwells · 2017-09-11T16:59:51Z

I have made the requested changes.

@bangerth's proposal is a lot nicer than the list of internal functions, so I am glad we went back and changed this.

drwells · 2017-09-11T17:02:46Z

Blah, I screwed something up when rebasing on different machines: hold on.

This commit changes the interfaces of Manifolds::get_default_points_and_weights Manifold::project_to_manifold Manifold::get_new_point Manifold::add_new_points to use ArrayView instead of std::vector. In addition, the interface of add_new_points has been changed to populate the ArrayView argument instead of appending to the end of the array. This breaks the public interface of Manifold for the sake of improving performance by about 30%: profiling indicates that, when creating a grid with a Manifold, we spend about 30% of our time purely calling new and delete since we must create and destroy so many std::vectors.

This function no longer appends new points to a vector so a name change is in order.

drwells · 2017-09-11T18:23:04Z

All right; this should be good to go now.

bangerth · 2017-09-11T18:37:17Z

OK to merge once the tester is happy.

drwells added Discussion Enhancement ready for review WIP labels Aug 8, 2017

kronbichler requested changes Aug 8, 2017

View reviewed changes

drwells force-pushed the manifold-arrayview-compat branch from c00334d to 1810257 Compare August 11, 2017 04:54

drwells mentioned this pull request Aug 17, 2017

The new general container type #4883

Closed

drwells mentioned this pull request Sep 4, 2017

Require boost 1.59 #5024

Merged

drwells force-pushed the manifold-arrayview-compat branch from 12209b6 to b8a8df3 Compare September 6, 2017 17:56

drwells removed the WIP label Sep 7, 2017

drwells mentioned this pull request Sep 7, 2017

Remove boundary usage #4798

Merged

kronbichler approved these changes Sep 7, 2017

View reviewed changes

drwells force-pushed the manifold-arrayview-compat branch 4 times, most recently from 954ff14 to d82a525 Compare September 8, 2017 21:32

drwells force-pushed the manifold-arrayview-compat branch from fff95e1 to 688c50d Compare September 11, 2017 14:05

bangerth reviewed Sep 11, 2017

View reviewed changes

tamiko removed Discussion Enhancement ready for review labels Sep 11, 2017

tamiko approved these changes Sep 11, 2017

View reviewed changes

tamiko added the Reviewed and ready to merge label Sep 11, 2017

drwells force-pushed the manifold-arrayview-compat branch from 688c50d to d55fd91 Compare September 11, 2017 16:58

drwells force-pushed the manifold-arrayview-compat branch from d55fd91 to 0431503 Compare September 11, 2017 17:00

drwells removed the Reviewed and ready to merge label Sep 11, 2017

drwells force-pushed the manifold-arrayview-compat branch from 0431503 to edb442e Compare September 11, 2017 18:16

drwells added 5 commits September 11, 2017 14:17

Mark methods as 'override'.

2dc1dda

Directly create an ArrayView from a table.

df56518

Rename add_new_points as get_new_points.

2893c02

This function no longer appends new points to a vector so a name change is in order.

Add a changelog entry for the Manifold incompatibities.

8bc04f9

drwells force-pushed the manifold-arrayview-compat branch from edb442e to 8bc04f9 Compare September 11, 2017 18:17

drwells mentioned this pull request Sep 11, 2017

FlatManifold::project_to_manifold should not always be the identity function #5056

Open

bangerth added the Reviewed and ready to merge label Sep 11, 2017

bangerth merged commit 853114d into dealii:master Sep 11, 2017

kronbichler mentioned this pull request Sep 14, 2017

Vectorized tensor product matrix #5074

Merged

Break the Manifold interface for performance. #4723

Break the Manifold interface for performance. #4723

Conversation

drwells commented Aug 8, 2017 • edited

tjhei commented Aug 8, 2017 • edited

bangerth commented Aug 8, 2017

drwells commented Aug 8, 2017

bangerth commented Aug 8, 2017

kronbichler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kronbichler Aug 8, 2017 • 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

drwells commented Aug 11, 2017

bangerth commented Aug 28, 2017

drwells commented Aug 28, 2017

drwells commented Sep 5, 2017

drwells commented Sep 6, 2017

drwells commented Sep 7, 2017

kronbichler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kronbichler commented Sep 7, 2017

kronbichler commented Sep 7, 2017

kronbichler commented Sep 8, 2017

drwells commented Sep 8, 2017

drwells commented Sep 11, 2017

bangerth 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

tamiko commented Sep 11, 2017

drwells commented Sep 11, 2017

bangerth commented Sep 11, 2017

drwells commented Sep 11, 2017

drwells commented Sep 11, 2017

drwells commented Sep 11, 2017

drwells commented Sep 11, 2017

bangerth commented Sep 11, 2017

drwells commented Aug 8, 2017 •

edited

tjhei commented Aug 8, 2017 •

edited

kronbichler Aug 8, 2017 •

edited