FEPointEvaluation: Remove signal to keep track of reinit() state

[kronbichler]

In the analysis for the particle benchmark done for #16854, I observed a high cost for the boost::signals2::connection (for my analysis around 10% of the cost in FEPointEvaluation::reinit(). We introduced this in #14982 in order to support the case where several FEPointEvaluation objects share the same MappingInfo object (typically because we want to query the expensive mapping only once). I now implement a simpler strategy that only keeps track of whether the object itself called reinit() or whether the data comes from another MappingInfo object, which then decides whether we must call the respective call in FEPointEvaluation::evaluate() or FEPointEvaluation::integrate(). There are some cases where my simple setup fails, e.g. when we call reinit() on the derived object on some cells only, but I don't think they are relevant to how we typically use them. We might want to add some basic assertions regarding the sizes (I would have to check how to do it). Furthermore, we evaluate the polynomials twice if we call evaluate() and integrate() on such a derived object; this could be avoided by calling a suitable reinit(numbers::invalid_unsigned_int) function I believe (again not checked). @bergbauer opinions?

Related to #15650.

[tjhei]

This looks much cleaner than before (including the variable names).👍

[bergbauer]

It is good to remove the signal from a performance perspective 👍🏽 @kronbichler

I still think it would be the cleanest option to shift this responsibility to call reinit to user code, see #14982 (comment) . This is how all our evaluator classes work, you have to call reinit() for FEValues/FEEvaluation/FEPointEvaluation before you can use it.

Making the "automatic" reinitalization more efficient (what this PR does) is a step in the right direction.

One question: We can also set the new bool must_reinitalize_pointers to false here

dealii/include/deal.II/matrix_free/fe_point_evaluation.h

Lines 2299 to 2310 in e9eb5ab

	template <int n_components_, int dim, int spacedim, typename Number>
	inline void
	FEPointEvaluation<n_components_, dim, spacedim, Number>::reinit()
	{
	this->current_cell_index = numbers::invalid_unsigned_int;
	this->current_face_number = numbers::invalid_unsigned_int;
	if (this->use_linear_path)
	this->template do_reinit<false, true>();
	else
	this->template do_reinit<false, false>();
	}

? Otherwise we would reinitialize several times when calling this reinit function and evaluate/integrate afterwards.

I would like to have @gassmoeller s opinion on this one more time since he is the one with the use case.

[kronbichler]

Otherwise we would reinitialize several times when calling this reinit function and evaluate/integrate afterwards.

I did think about it, but the question is when we would turn the value, once it is false, back to true again for the next time we come around with a new batch of points or a new element. So in a sense, that choice would, if I do not miss something, require the use of the signals again. I guess this is the price one needs to pay in terms of efficiency. As you suggest, it is better to call reinit() on the evaluator again (without passing the points), because then one can more explicitly control what happens. I realize that my code would not make the evaluate/integrate optimization with the code you suggested in the other PR, so we might want to think about which interface we would use for this case, and if we would need to discuss this in the documentation of recommended use (here or in a follow-up PR).

[bergbauer]

I did think about it, but the question is when we would turn the value, once it is false, back to true again for the next time we come around with a new batch of points or a new element. So in a sense, that choice would, if I do not miss something, require the use of the signals again. I guess this is the price one needs to pay in terms of efficiency.

You are right, this exactly what the bool can't do but the signal can. I am in favour of the current PR. But it will probably slow down @gassmoeller s use case because there reinit will be called twice with the change from this PR. In my opinion we should give the user full control (and responsibility) to call reinit to enable full performance for all use cases.

[drwells]

@gassmoeller any comments or should we merge?

[blaisb]

Very good catch @kronbichler . I do not foresee any issue with this.

[gassmoeller]

Sorry for the slow response, I am in the middle of a move right now. Do I understand correctly that the only negative consequence of this PR could be that the performance degrades somewhat for expensive mappings if the MappingInfo object is shared between multiple FEPointEvaluation objects? I would want to do some benchmarks to evaluate that impact once I have some time, but as long as this change doesnt break existing user code we can merge this for now.

As background: ASPECT has the use case where 3 or 4 FEPointEvaluation objects (one each for the different variables of a FESystem) share the MappingInfo object, with a MappingQ(4) mapping including derivatives.

[bergbauer]

Do I understand correctly that the only negative consequence of this PR could be that the performance degrades somewhat for expensive mappings if the MappingInfo object is shared between multiple FEPointEvaluation objects?

Yes, the only negative consequence will be a minor performance degradation for your use case. It comes from multiple reinit() calls in which pointers are set (additional integer work). I expect it to be visible for low dimension and linear finite elements as base element in your FESystem, when there is not enough floating point work. The cost of the mapping doesn't play a role here.

[kronbichler]

Let me add a little on the comment by @bergbauer: There is some integer work that gets done more often. We could work around this (and I would be willing to create a follow-up patch or addition to this patch) by letting the reinit() function without arguments still set the pointers, but use a different code path for the automatic refresh of information that gets called inside evaluate() and integrate() without prior call to reinit() as required by this setup in a test:

dealii/tests/matrix_free/point_evaluation_19.cc

Lines 110 to 113 in 36dc1a7

	mapping_info.reinit(cell, unit_points);
	evaluator1.evaluate(solution_values,
	EvaluationFlags::values | EvaluationFlags::gradients);

I guess this could be clearer from a use perspective, in that reinit() always makes sure information is not called again.

[gassmoeller]

The cost of the mapping doesn't play a role here.

There is some integer work that gets done more often.

If we are only talking about setting some pointers I do not think this is a problem. I was concerned the mapping information may have to be computed several times, but as long as that is not the case feel free to merge this PR.