BoundaryOp refactor performance regression fix?

[johnomotani]

@ZedThree @JosephThomasParker @d7919, I made an example to check the performance of the BoundaryOps. At least on my laptop, it seems that the refactored version was a lot slower than next. For 10000 calls to f.applyBoundary():
dirichlet 1.05137 s (next) -> 4.63342 s (BoundaryOp-refactor)
neumann 3.37127 s (next) -> 6.1994 s (BoundaryOp-refactor)
dirichlet_o3 7.34798 s (next) -> 8.08621 s (BoundaryOp-refactor)

I've tried to jig things around to get more functions to inline and get back to the same performance. Getting rid of calls to f.getNZ() and localmesh->GlobalZ(z) seemed to help with BoundaryDirichlet. But BoundaryDirichlet implements its own apply method, so was pretty much the same as the old implementation anyway. BoundaryNeumann and BoundaryDirichlet_O3 use the new base-class apply() and implement methods to apply the boundary condition at a single point that get called inside the loop. I thought using CRTP pattern to allow the BoundaryOpWithApply<typename Derived>::applyTemplate method to use static methods of the Derived class so that they could be inlined, which helps a bit.

The thing that's confusing me is: adding inline keywords to some of the methods; in particular making BoundaryNeumann::applyAtPoint(Field3D...) inline reduced the execution time for the neumann boundary condition from about 2.7 s to 0.9s. However, adding inline to all the methods slows down both dirichlet and neumann (even though those methods haven't been changed). Is there some kind of limit to the number of methods that can be inlined in a file, and the compiler just stops inlining when the limit is reached?

The current state of this branch makes examples/BoundaryOp_timing run as fast as I've managed: adding more inline keywords slows it down (once I add a certain number of them).

[ZedThree]

inline is (mostly) just a suggestion to the compiler: "I'm confident that this can be inlined, please do so". The compiler normally has some heuristic on whether or not to actually do so; these include a limit on the function call depth to inline (it might not inline a 30 deep stack of function calls for instance). Another is on the size of the function(s), meaning it might not inline large functions even if you ask. I don't think there's a limit on the number of inline functions in a file though.

I believe inlining might also interact with the instruction cache as well, so inlining everything might plausibly have negative effects.

[johnomotani]

@ZedThree thanks for the quick reply. What you say confirms my vague ideas about what inline should do (I was surprised it had any effect at all). The part that really gets me is that adding inline to different boundary conditions (that aren't in the timing test) slows down the ones being timed. As an example, the extra inlines in 29757e4 slow down 'dirichlet' and 'neumann' boundary conditions (on my desktop machine) from {0.67s, 0.69s} to {2.01s, 1.60s}. 'dirichlet_o3' isn't affected, it takes 2.16s before and after.

29757e4 only added inline to BoundaryDirichlet_O4::applyAtPoint, BoundaryDirichlet_smooth::applyAtPoint, BoundaryDirichlet_2ndOrder::applyAtPoint, BoundaryDirichlet_O5::applyAtPoint, BoundaryNeumann2::applyAtPoint, BoundaryNeumann_2ndOrder::applyAtPoint, BoundaryNeumann_O4::applyAtPoint, BoundaryNeumann_4thOrder::applyAtPoint, and BoundaryConstGradient::applyAtPoint.
😕

[d7919]

Could it be just timing jitter?

[d7919]

I should also note that there are a few examples of performance tests with helpful macros in the examples/performance/iterator* directories which might help write your tests here.

[d7919]

I think these would need to be removed should this go into the main code -- we really want to avoid this level of (compiler family specific) detail where possible. I think the simplest fix here would be to just explicitly say xnorm=ynorm=0 in the initial declaration of the variables.

[ZedThree]

Yes, absolutely agree. It's good practice to always initialise variables

[johnomotani]

👍 sorry, I think I must have been in caffeine withdrawal when I thought that was a good idea 😫

[johnomotani]

Could it be just timing jitter?

I don't think so, the timings vary by maybe +/-0.02s between repeated runs. The difference between the cases with/without more inlines is >1s.

[johnomotani]

The answer could also be that the way I've written the BoundaryOps is horrible... pointers/suggestions for a different design welcome 👍

[johnomotani]

Comparing to @dschwoerer's code in #1179, I realised that the main thing slowing down the boundary conditions was conditionals checking if the FieldGenerator is initialized inside the boundary condition loops. Moving the conditional outside the loops makes the new implementation significantly faster than the original. [Faster in most cases: oddly BoundaryDirichlet in the original implementation managed to be nearly as fast even with the ifs inside the loops. Not sure how the compiler managed to optimize that, but it seems to have been a bit fragile, since with the old pattern changes elsewhere in the file slowed down BoundaryDirichlet].

There's now one branch with a FieldGenerator and one branch without, so a bit of code duplication, but it's only in two methods now, so seems worth it for the speed-up.

[johnomotani]

Parsing expressions is much slower, so I updated the boundary conditions to apply a constant value (in without using the expression parser.

I think this PR is ready to merge into #1334 now. Should I keep the BoundaryOp_timing example in and if so, (i) should I move it into examples/performance; (ii) does it need tidying up in the style of examples/performance/iterator, etc.?

[dschwoerer]

I think the test is useful 👍
I have changed it in 73081b6 - so that other boundary operator can be tested without recompiling. That makes the tests slower, but imho still fast enough.