libMesh / libmesh

libMesh github repository
http://libmesh.github.io
GNU Lesser General Public License v2.1
654 stars 286 forks source link

Make `PetscVector::operator()` actually const #854

Open friedmud opened 8 years ago

friedmud commented 8 years ago

After doing some more thinking about https://github.com/libMesh/libmesh/pull/853 the problem is really that we shouldn't be doing any of the stuff that is happening in PetscVector::_get_array(). NumericVector::operator() is const for a reason... and we shouldn't be violating that by using mutable (like here: https://github.com/libMesh/libmesh/blob/master/include/numerics/petsc_vector.h#L510 ). That's what's gotten us into this ( https://github.com/idaholab/moose/issues/6436 ) mess.

I propose this:

  1. We remove all mutable out of PetscVector.
  2. We add a new virtual void NumericVector::cacheValues() method that developers can explicitly call in order to allow NumericVectors to cache local data (i.e. what PetscVector::_get_array() is doing). Developers can call this function to speed up subsequent operator() calls. This will allow developers the chance to do this explicitly before threaded sections.
  3. Add a new method: virtual void NumericVector::restoreCachedValues()
  4. All of the current methods that call PetscVector::_restore() should be made to error out in the case that cacheValues() has been called... and the data is still cached (i.e. that restoreCachedValues() has not been called).

Essentially: I think that we should get much more explicit about the memory movement and caching inside NumericVectors.

friedmud commented 8 years ago

For a while this can be a compile time switch that we turn on.

--enable-thread-safe-numeric-vectors

?

jwpeterson commented 8 years ago

:+1: for making operator() actually const. I have gone from regarding the mutable keyword as "interesting hack" to "actively evil".