Mark restore_array const

[dschwen]

Closes #4155

[moosebuild]

Job Coverage, step Generate coverage on 944850d wanted to post the following:

Coverage

	fa7d93	#4156 944850
	Total	Total	+/-	New
Rate	63.42%	63.42%	-	0.00%
Hits	74830	74830	-	0
Misses	43165	43165	-	1

Diff coverage report

Full coverage report

Warnings

• New new line coverage rate 0.00% is less than the suggested 90.0%

This comment will be updated on new commits.

[dschwen]

Ok, let's pump the brakes on this. @lindsayad pointed out that there may be issues with race conditions in threaded regions. I'll check if we need to add locking. Maybe he want's to chime in here.

[lindsayad]

A few thoughts:

1. I wish that we had some record about why we use VecGetArray(Read) and VecRestoreArray(Read) in the first place as opposed to just using things like VecGetValues and VecSetValues. But when I do a git log -S VecGetArray --reverse I see the first libMesh git commit haha. The natural hypothesis would be performance, but how much performance do we gain? I suppose having the raw array may also be helpful for doing larger global vector operations compared to element assembly.
2. Let's presume it's a good idea to have these raw array getter APIs. My vote would be to split _get_array and _restore_array into methods that either call the non-const VecGetArray/VecRestoreArray or the const VecGetArrayRead/VecRestoreArrayRead, but not both. Another option would be to eliminate these helper methods and put the code into the APIs. For APIs I think we should have void get_array_read() const, void get_array(), void restore_array_read() const (new), and void restore_array()

[lindsayad]

Let's workout the discussion a bit

[dschwen]

Could we maybe use a sentinel pattern for this? Two versions, one Read/Write one Read-only, the constructor calls the appropriate get method. The sentinel has a method to access the pointer, the destructor calls the appropriate restore method.

I'd love to keep raw access available, if only for experimental code that allows for fast copies between compute devices.

[lindsayad]

I think that's a nice idea

[roystgnr]

IIRC Derek's changes were purely for performance. I'd agree that figuring out exactly how much performance would be nice.

I like the idea of splitting our helper methods into const and non-const versions rather than letting read vs write be a parameter.

I hate the idea of exposing these methods to the user. That would require users to add Petsc-specific calls for non-Petsc-specific code, and to manually manage more complication.

I love the sentinel pattern idea in theory. It's kind of embarrassing that we still have pairs of C functions like this that don't yet have an RAII wrapper, now that I think about it. I guess the names are "Set/Restore" instead of the usual "Create/Destroy" and that was enough for it to slip past us?

[lindsayad]

I hate the idea of exposing these methods to the user. That would require users to add Petsc-specific calls for non-Petsc-specific code, and to manually manage more complication.

What are you referring to? The sentinel pattern idea?

[dschwen]

I'm sure Roy dislikes the idea of special treatment for PETSc vectors. I do understand that from his library developer standpoint, but from my side I wouldn't mind having an API for my 99-100% use-case.

[lindsayad]

IIRC Derek's changes were purely for performance

_get_array and _restore_array were added in 6fa3a3b, which was well before @friedmud added the public get_array, get_array_read, restore_array in #1056. But as the message in 6fa3a3b says, it was for performance

[roystgnr]

I'm referring to "eliminate these helper methods and put the code into the APIs". Or maybe I misunderstood you? We've got exposed get_array(), restore_array() already, I thought you were talking about requiring users to manually do the work of _get_array() rather than relying on the helper methods to be called correctly from the more general APIs.

[lindsayad]

I thought you were talking about requiring users to manually do the work of _get_array()

Definitely not. I was suggesting that the non-const branch of _get_array() be moved into get_array and the const branch of _get_array be moved into get_array_read. Other methods on PetscVector can just as easily call get_array/restore_array (or even better get_array_read/restore_array_read for const methods!) as _get_array/_restore_array, just remove a character 😄