ConsensusAlgorithms interface

[bangerth]

I'm playing with the consensus algorithms and I'm finding the interface a bit clunky. Specifically, the intent is that one creates a class that has a bunch of virtual functions that encode the various operations. But, in practice, these functions will typically access variables that live in the environment of the place where one calls the consensus algorithm and that would have to be passed to the object of the class one has to implement. That's what is most easily done using lambda functions -- which is actually already modeled by the ConsensusAlgorithms::AnonymousProcess class. Consequently the places I can find where consensus algorithms are used do things with lambda functions.

My proposal would be to get rid of the whole Process class and instead just let the ConsensusAlgorithms::Interface::run() have an interface of the form

  std::vector<unsigned int>
  run (const std::vector<unsigned int> &destinations,
         const std::function<...> &create_request,
         const std::function<....> &prepare_buffer_for_answer,
         const std::function<....> &answer_request,
         const std::function<....> &read_answer);

I can do this in a backward compatible way if desired, though I doubt that that functionality is used anywhere outside deal.II. (But then, I've been surprised before...)

Any thoughts about this?

[peterrum]

Historically, ConsensusAlgorithms::AnonymousProcess is the newest class in the ConsensusAlgorithms namespace. I have added it because my observation was that in many cases a Process is only used once and in such cases it is quite convenient to use lambda functions and capture the variables.

However, there are cases where we want to reuse processes, most notably Utilities::MPI::internal::ComputeIndexOwner::ConsensusAlgorithmsPayload, which is used all over the place in the library. That's why I am not in favor of getting rid of the Process infrastructure. If you want you could add an additional run function that takes the lambda functions and creates a ConsensusAlgorithms::AnonymousProcess instance and registers it.

[kronbichler]

Just to give some more background: Indeed the function Utilities::MPI::compute_index_owner

dealii/include/deal.II/base/mpi.h

Lines 1245 to 1258 in d5995ec

	* @param[in] owned_indices Index set with indices locally owned by this
	* process.
	* @param[in] indices_to_look_up Index set containing indices of which the
	* user is interested the rank of the owning process.
	* @param[in] comm MPI communicator.
	*
	* @return List containing the MPI process rank for each entry in the index
	* set @p indices_to_look_up. The order coincides with the order
	* within the ElementIterator.
	*/
	std::vector<unsigned int>
	compute_index_owner(const IndexSet &owned_indices,
	const IndexSet &indices_to_look_up,
	const MPI_Comm &comm);

was the first use of the function. That is a performance-critical function that has received quite some TLC in terms of data structures, interfaces, and internal helper functions. Funneling through those via lambda functions can be done, but I would not consider it good coding style when we can actually do object-oriented programming.

On the other hand, I would also agree that providing a possibility to run the algorithm via lambda functions would be a good solution to cover all those additional use cases of the consensus algorithms that we keep gathering recently.

[bangerth]

@peterrum Ah, I had not known about ConsensusAlgorithmsPayload.

How about the following proposal:

• We have two run() functions, one of which takes a Process object and the other one a set of function objects
• The former is implemented in the base class, and calls the second one by creating lambda functions that just call the virtual functions.
• The latter is abstract virtual in the base class and implemented in derived classes. Derived classes therefore do not need to have anything to do with Process objects.

[bangerth]

@kronbichler I would suspect that the overhead of whether we call a virtual function or a function object is negligible compared to the cost of data transfer. I'm ok with leaving the existing interfaces around, but it would be nice to reduce the set up cost (in terms of lines of code) if one just wants to call the algorithm once.

[peterrum]

Sounds OK. But how do you want to make this backwards-compatible - the process is taken currently by the constructor.

[bangerth]

I would eventually break the interface, but for the moment can retain the existing constructor and run(void) function pair -- both of which I can then deprecate.

[marcfehling]

We should add an (extensive?) changelog entry on how the ConsensusAlgorithm interface has been changed.

Possibly by expanding the current entry https://github.com/dealii/dealii/blob/master/doc/news/changes/incompatibilities/20220118Bangerth, which is the only one I could find in the folder.

[bangerth]

I intend to write a section about this in the release paper.