[C++] Optimizations and cleanups and const correctness, oh my

[jcking]

Various changes related to optimizations, cleanup, and const correctness fixes. The optimizations are related to std::shared_ptr and getting rid of unnecessary copying when possible as we as some prerequisite work for future improvements. The cleanup is marking things as final or removing odd whitespace. The const correctness is self explanatory.

Additionally this makes LL1Analyzer final and removes its virtual methods. Its not possible for generated code to override the LL1Analyzer and having virtual in the hot path is not free, unlike Java. In C++ virtual methods effectively guarantee CPU cache misses.

Lastly it moves a few methods in the hot path to be inline, specifically Recognizer::set/getState.

[jcking]

@mike-lischke

[mike-lischke]

Passing a shared_ptr as const ref is not a good idea. I had a discussion on Stackoverflow 5 years ago, exactly because I was thinking about doing that. You are killing the shared_ptr promise to ensure a pointer is valid as long as at least one place in code holds the shared_ptr.

If you really want to improve that (what I had in mind since I wrote that code) is to pass through raw pointers and make sure there's "one source of truth" that owns the pointer (using a unique_ptr then).

[jcking]

Passing a shared_ptr as const ref is not a good idea. I had a discussion on Stackoverflow 5 years ago, exactly because I was thinking about doing that. You are killing the shared_ptr promise to ensure a pointer is valid as long as at least one place in code holds the shared_ptr.

If you really want to improve that (what I had in mind since I wrote that code) is to pass through raw pointers and make sure there's "one source of truth" that owns the pointer (using a unique_ptr then).

I think I switched most to be pass by value, which avoids incrementing and decrementing the reference count when paired with std::move, increasing throughput. The other changes where I made it const reference was to copying when possible with return values, avoids incrementing/decrementing reference counts.

[mike-lischke]

I just realised, it's the other way around: you removed passing by reference. This is totally fine then!

However, I wonder why you didn't apply that to all cases. For example: ATNConfig(Ref<ATNConfig> const& c, Ref<SemanticContext> semanticContext); you removed the const& from the second parameter. Why not from the first too?

[jcking]

I just realised, it's the other way around: you removed passing by reference. This is totally fine then!

However, I wonder why you didn't apply that to all cases. For example: ATNConfig(Ref<ATNConfig> const& c, Ref<SemanticContext> semanticContext); you removed the const& from the second parameter. Why not from the first too?

The implementation of the constructor does not actually save a reference to ATNConfig, it only accesses members. Realistically it should probably just accept const ATNConfig &c but that was a larger change that I was willing to do for this specific bit.

[mike-lischke]

@parrt This patch is ready to merge.

[parrt]

something is weird here with the unit test. Let me rerun them. Still looks messed up. Even Python3 says:

java.lang.RuntimeException: Missing system property:antlr-python3-python

Is it possible this PR is based on a previous version where we had the CI builds messed up? @ericvergnaud any ideas?

[jcking]

Rebased off of master. Let's see what happens.

[jcking]

All passed now, was due to being on an older base of master.

@parrt

[parrt]

Thanks again for all your help!