[C++] Performance macros in header files

[jrsala-auguration]

In the C++ client, the DISABLE_BOUNDS_CHECKS macro, if defined, is intended to make AtomicBuffer operations faster at the cost of safety. It is defined by file aeron-client/src/main/cpp/CMakeLists.txt when performing a non-debug build.

Since that variable is tested for in inline functions in file AtomicBuffer.h and that file can be included (directly or indirectly) by users, users might not benefit from that speedup unless they too define DISABLE_BOUNDS_CHECKS in their release builds, but this is not documented anywhere.

Is this situation intentional? If so, what is the rationale? If not, I can think of a few solutions, but waiting to hear from maintainers first.

Thanks

[tmontgomery]

Sorry for the delay.

Yes. It is intentional. The reasoning here is so that the check is conditionally compiled out totally and thus enables as much optimization as possible. What solutions are you thinking of?

[jrsala-auguration]

Hi Todd, thanks for your response, and no worries re. the delay.

I'm afraid I did not explain the issue clearly. When Aeron code is compiled, the presence or absence of DISABLE_BOUNDS_CHECKS is correctly controlled by CMakeLists.txt depending on whether the build is a release build or a debug build.

However, when application code is compiled and it (directly or indirectly) includes AtomicBuffer.h, it most likely will not define DISABLE_BOUNDS_CHECKS even in the application's release builds because developers of application code (such as myself) are not aware that this macro exists and is used in inline functions in a header file. As a consequence, Aeron code benefits from disabling bounds checks but application code most likely will not.

Obviously this depends on whether the compiler decides to inline a given function at a given call site, but having had a look at the member functions of AtomicBuffer, for a lot of them it most likely will.

Solutions I can think of are:

• Tell application code developers to define DISABLE_BOUNDS_CHECKS in their own release builds so they benefit from the optimisations. One downside is that it pushes the effort to the users of Aeron. Another downside is that this instruction might be missed by users if it's buried in some documentation. One benefit is that there is no change to be made to Aeron code.
• Not a solution but a mitigation: have Aeron code define DISABLE_BOUNDS_CHECKS itself if the NDEBUG variable is defined. That variable being present is a very common convention for signifying that code is being compiled as part of a release build with optimisations turned on. One downside is that it's just a convention, and people don't have to follow it, and if they don't follow it, they still won't benefit from optimisations. The original purpose of NDEBUG is to disable assert statements.
• Change the default behaviour for application code from optimisations disabled by default (DISABLE_BOUNDS_CHECKS not defined) to enabled by default. This means replacing DISABLE_BOUNDS_CHECKS with an ENABLE_BOUNDS_CHECKS variable. One benefit is that application code developers don't have to do anything to benefit from the change. The change in Aeron code is also very straightforward. Application developers can still define ENABLE_BOUNDS_CHECKS in their own builds to benefit from the checks. One potential downside is that application developers might have gotten used to bounds checks being enabled by default and catching their mistakes, but I think that's unlikely.

I prefer the 3rd solution.

Another remark: I would rename DISABLE_BOUNDS_CHECKS to AERON_DISABLE_BOUNDS_CHECKS to avoid potential naming conflicts and to clearly indicate the purpose and "domain" of the variable.

Thanks

[tmontgomery]

I've given this some thought from the standpoint of existing users and the impact on supported users. This gets complicated.

While I agree with the variable rename, I think renaming the variable is an issue due to the impact on existing code that does set the variable and the difficulty in tracking down the performance impact. So, I don't think we can do that easily.

This is the same impact for the 3rd option but is the opposite for the users who don't set the macro as they desire the bounds to be checked.

The 2nd option is probably also not desirable.

Which leaves option 1. Which is a good idea. Maybe adding it to the wiki in the Performance Testing section. Would that make sense?

[jrsala-auguration]

Sure, happy with that. Thanks.

[mikeb01]

https://github.com/real-logic/aeron/wiki/Performance-Testing#native-code-bounds-checking