Optimizations

[cbirkhold]

1/ Avoid copies and temporaries.
2/ Faster access to atmosphere values when multiple altitude dependent values are queried.

[codecov]

Codecov Report

Attention: Patch coverage is 71.87500% with 9 lines in your changes missing coverage. Please review.

Project coverage is 25.01%. Comparing base (d04edb8) to head (e8f1255).

Files with missing lines	Patch %	Lines
src/input_output/FGGroundCallback.h	0.00%	5 Missing ⚠️
src/math/FGLocation.h	20.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1194      +/-   ##
==========================================
+ Coverage   24.96%   25.01%   +0.04%     
==========================================
  Files         170      170              
  Lines       19283    19307      +24     
==========================================
+ Hits         4814     4829      +15     
- Misses      14469    14478       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

---

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests

[bcoconni]

It seems to me this PR should be split into 2 PRs because it is addressing 2 unrelated topics. Could you please do that ?

Regarding the atmosphere optimization, it seems to me that the speed improvement of your PR is only for those who use FGDefaultGroundCallback. Indeed your change adds a level of indirection to existing implementations of FGGroundCallback which complexifies the code and most certainly does not give any speed improvement in return. AFAIC FGDefaultGroundCallback exists to avoid implementing boiler plate code for POC or basic usage of JSBSim so this is not the path I'd spontaneously optimize. I'd rather do the opposite i.e. optimize the complex path for custom implementations of FGDefaultGroundCallback.

However I'm a bit skeptical that your PR brings any measurable improvement to the speed of execution. In which case, the additional complexity would not be payed back. So could you please provide some data to substantiate your claim ?

Thanks.

[cbirkhold]

It seems to me this PR should be split into 2 PRs because it is addressing 2 unrelated topics. Could you please do that ?

Certainly!

Regarding the atmosphere optimization, it seems to me that the speed improvement of your PR is only for those who use FGDefaultGroundCallback. Indeed your change adds a level of indirection to existing implementations of FGGroundCallback which complexifies the code and most certainly does not give any speed improvement in return. AFAIC FGDefaultGroundCallback exists to avoid implementing boiler plate code for POC or basic usage of JSBSim so this is not the path I'd spontaneously optimize. I'd rather do the opposite i.e. optimize the complex path for custom implementations of FGDefaultGroundCallback.

In the case where a custom FGGroundCallback implementation is used but does not provide an overload for GetAGLevel(const FGLocation&) the extra indirection would indeed be incurred, however this only replaces the indirection already present in the code path without the change so there actually is no delta in cost or complexity of the code path.

I intended to follow the pattern of the other non-pure overload just above the proposed change which has the same effect in terms of indirections and is the default method called on the interface (FGInertial).

If I'm not mistaken there is an automatic benefit to clients using the default implementation. When customized and only the pure-virtual method is overridden the cost is the same as before. At the same time, clients that care about performance would now have a simple way to support the optimized path by adding the overload to their custom implementation. The only negative effect would be for a custom implementation that provides an override for the non-pure-virtual method above the proposed change (which is the one used internally by FGInertial for instance) but not the new proposed overload.

However I'm a bit skeptical that your PR brings any measurable improvement to the speed of execution. In which case, the additional complexity would not be payed back. So could you please provide some data to substantiate your claim ?

You will probably see a fairly small effect in the test cases/samples of this project. In applications that mix high-fidelity JSBSim simulated entities with a large number of simpler entities stepped outside JSBSim but operating in the same world and querying the JSBSim environment for certain properties the effect can become quite relevant.

Totally understand though if this is not a change that makes sense to integrate from your perspective, it certainly is easy enough to maintain as a patch on the client side. Much larger gains are certainly possible when touching/limiting features, though I haven't been able to wrap those changes in a way that makes sense for the project repository yet. With this I've been trying to push back some changes that have come out of profile guided optimization and are more localized in case they make sense for the community.

I will split up the PR to make it easier for you to integrate (or not).

As always thanks for your work on this valuable project!

[bcoconni]

In the case where a custom FGGroundCallback implementation is used but does not provide an overload for GetAGLevel(const FGLocation&) the extra indirection would indeed be incurred, however this only replaces the indirection already present in the code path without the change so there actually is no delta in cost or complexity of the code path.

I'm afraid not. Currently, for all classes inheriting from FGGroundCallabck, the call graph is the following:

flowchart LR;
A[FGInertial::GetAltitudeAGL];
B["FGGroundCallback::GetAGLevel(FGLocation&, FGLocation&, FGColumnVector3&, FGColumnVector3&, FGColumnVector3&)"];
A-->B;

Loading

With your PR, for classes inheriting from FGGroundCallabck and which do not implement the new GetAGLevel method, the call graph would be:

flowchart LR;
A[FGInertial::GetAltitudeAGL];
B(["FGGroundCallback::GetAGLevel(FGLocation&)"]);
C["FGGroundCallback::GetAGLevel(FGLocation&, FGLocation&, FGColumnVector3&, FGColumnVector3&, FGColumnVector3&)"];
A-->B-->C;

Loading

So there is indeed an additional level of indirection and therefore a delta in cost and complexity.

If I'm not mistaken there is an automatic benefit to clients using the default implementation.

Not sure what you mean, here.

When customized and only the pure-virtual method is overridden the cost is the same as before.

That's only the case for classes that implement the new method GetAGLevel(FGLocation&).

At the same time, clients that care about performance would now have a simple way to support the optimized path by adding the overload to their custom implementation. The only negative effect would be for a custom implementation that provides an override for the non-pure-virtual method above the proposed change (which is the one used internally by FGInertial for instance) but not the new proposed overload.

There are 2 strong assumptions behind these statements:

1. Clients are willing to modify the code of their implementation of FGGroundCallback i.e. have the time, workforce and budget of doing so.
2. Your change brings some performance improvements which could be an incentive if the assumption above is met.

However, none of these assumptions can be taken for granted.

You will probably see a fairly small effect in the test cases/samples of this project. In applications that mix high-fidelity JSBSim simulated entities with a large number of simpler entities stepped outside JSBSim but operating in the same world and querying the JSBSim environment for certain properties the effect can become quite relevant.

I'd certainly merge your change if you could prove that it brings some improvements. From what I'm seeing from your PR, the changes in performance - if any - will be negligible so there is not much point (performance-wise) in modifying the code. I'd be happy to be proven wrong but at the moment there are no facts to substantiate your claims so I'm staying on the conservative side.