perf: replace std::pow(x, 0.25) with std::sqrt(std::sqrt(x))

[paulgessinger]

powf64 is relatively slow. This change improves the performance of ActsBenchmarkEigenStepper by about 10%, the performance impact on the more real-world propagation with navigation (as in the propagation example with the generic detector), seems negligible.

Overall I think it's probably still worth adding.

[codecov]

Codecov Report

Merging #1150 (c716eee) into main (f9dbc02) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1150   +/-   ##
=======================================
  Coverage   47.89%   47.89%           
=======================================
  Files         359      359           
  Lines       18502    18504    +2     
  Branches     8730     8730           
=======================================
+ Hits         8861     8863    +2     
  Misses       3605     3605           
  Partials     6036     6036           

Impacted Files	Coverage Δ
Core/include/Acts/Propagator/EigenStepper.ipp	50.00% <100.00%> (+0.75%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9dbc02...c716eee. Read the comment docs.

[HadrienG2]

It's a bit puzzling to me that we have two almost but not quite identical versions of the step size adaptation formula. But I'm happy to see this optimization land.

[paulgessinger]

Copy paste error?

Also, I'm a bit surprised that the root file output changes. I'd have thought that a slight numerical change in the step size scaling would have a negligible effect on the output...

[asalzburger]

It's a bit puzzling to me that we have two almost but not quite identical versions of the step size adaptation formula. But I'm happy to see this optimization land.

This doesn't surprise me, I think this is really a numerical difference from slight change of step size.

[benjaminhuth]

Out of interest I've played a bit around with different versions of this, and I found that

std::sqrt(std::sqrt(static_cast<float>(x)));

is about twice as fast as using the double version. As I understand we do not require large precision here, so that could speed this even up a bit?

(The values I measured by just computing the result out of many sqrts(sqrts(...)) in a row, but without vectorization and with -O2)

[HadrienG2]

@benjaminhuth This throughput difference indeed expected, https://www.agner.org/optimize/instruction_tables.pdf states that on modern CPUs the underlying SQRTSS and SQRTSD hardware instructions exhibit a 2x throughput difference in the worst-case scenario. I also agree that some computations here could likely be safely moved to single precision.

[paulgessinger]

I'd probably say using this static cast to float is a good idea. I don't think we care about numerical precision in these cases.

[paulgessinger]

Switched to sqrt(sqrt(static_cast<float>(x))). The CI should fail still because of the output hashes, I'll fix those from the CI values.

[stephenswat]

Depending on the frequency at which the clamping condition between 0.25 and 4.0 occurs, it might also be beneficial to rewrite the clamp explicitly:

float r = state.options.tolerance / std::abs(2. * error_estimate);

if (r <= 0.00390625f) { // 0.25^4
    stepSizeScaling = 0.25;
else if (r >= 256.f) { // 4.0^4
    stepSizeScaling = 4.0;
} else {
    stepSizeScaling = std::sqrt(std::sqrt(r));
}

Hard to say if you'll benefit without knowing how often this fires, though. 🤷‍♂️

[paulgessinger]

@stephenswat that could be an improvement, but might indeed be overkill.

[asalzburger]

This has conflicts now because of the updated file I suppose ...

[paulgessinger]

Updated the hashes again. Let's see.

[paulgessinger]

This is green now. Do we merge (/ can you approve)? @benjaminhuth @HadrienG2 @stephenswat ?