[C++] Drastically improve multi-threaded performance

[jcking]

The original implementation uses two static std::shared_mutex that are shared among all ParserATNSimulator and LexerATNSimulator. This causes all generated lexers and parsers to share the same global locks. This is unnecessary as all data is rooted in antlr4::atn::ATN, meaning all data that needs synchronized access belongs to exactly one antlr4::atn::ATN. With this in mind, we can shift the locks from static to per antlr4::atn::ATN. This drastically improves performance as the number of threads go up, in some cases as much as 40%.

[jcking]

From CEL's parsing benchmarks using various thread configurations:

name                 old cpu/op  new cpu/op  delta
BM_Parse/threads:1   3.27ms ± 2%  3.33ms ± 6%   +1.86%  (p=0.000 n=49+44)
BM_Parse/threads:2   4.32ms ± 5%  4.00ms ± 5%   -7.37%  (p=0.000 n=51+54)
BM_Parse/threads:4   7.19ms ± 3%  5.39ms ± 5%  -25.07%  (p=0.000 n=49+51)
BM_Parse/threads:8   15.2ms ± 4%   9.6ms ± 6%  -36.50%  (p=0.000 n=33+55)
BM_Parse/threads:12  26.9ms ± 1%  16.5ms ± 4%  -38.88%  (p=0.000 n=13+18)

name                 old time/op             new time/op             delta
BM_Parse/threads:1   3.27ms ± 2%             3.33ms ± 6%   +1.86%        (p=0.000 n=49+44)
BM_Parse/threads:2   2.18ms ± 5%             2.02ms ± 6%   -7.29%        (p=0.000 n=51+56)
BM_Parse/threads:4   1.88ms ± 4%             1.39ms ± 6%  -26.32%        (p=0.000 n=45+48)
BM_Parse/threads:8   2.30ms ± 3%             1.47ms ± 6%  -36.14%        (p=0.000 n=31+56)
BM_Parse/threads:12  2.73ms ± 1%             1.62ms ± 2%  -40.75%        (p=0.000 n=13+20)

name                 old INSTRUCTIONS/op     new INSTRUCTIONS/op     delta
BM_Parse/threads:1    24.7M ± 0%              24.2M ± 0%   -1.99%        (p=0.000 n=50+47)
BM_Parse/threads:2    24.7M ± 0%              24.2M ± 0%   -2.02%        (p=0.000 n=54+57)
BM_Parse/threads:4    24.7M ± 0%              24.2M ± 0%   -2.10%        (p=0.000 n=50+52)
BM_Parse/threads:8    24.8M ± 0%              24.1M ± 0%   -2.45%        (p=0.000 n=31+57)
BM_Parse/threads:12   24.8M ± 0%              24.1M ± 0%   -2.67%        (p=0.000 n=12+19)

name                 old CYCLES/op           new CYCLES/op           delta
BM_Parse/threads:1    12.3M ± 2%              12.5M ± 6%   +1.75%        (p=0.000 n=49+42)
BM_Parse/threads:2    14.1M ± 2%              13.7M ± 5%   -2.71%        (p=0.000 n=51+47)
BM_Parse/threads:4    17.9M ± 2%              15.9M ± 8%  -11.64%        (p=0.000 n=47+51)
BM_Parse/threads:8    26.4M ± 5%              22.5M ± 8%  -14.73%        (p=0.000 n=31+57)
BM_Parse/threads:12   35.5M ± 2%              30.0M ± 3%  -15.42%        (p=0.000 n=13+20)

name                 old allocs/op           new allocs/op           delta
BM_Parse/threads:1    33.5k ± 0%              33.5k ± 0%     ~     (all samples are equal)
BM_Parse/threads:2    33.5k ± 0%              33.5k ± 0%     ~     (all samples are equal)
BM_Parse/threads:4    33.5k ± 0%              33.5k ± 0%     ~     (all samples are equal)
BM_Parse/threads:8    33.5k ± 0%              33.5k ± 0%     ~     (all samples are equal)
BM_Parse/threads:12   33.5k ± 0%              33.5k ± 0%     ~     (all samples are equal)

name                 old peak-mem(Bytes)/op  new peak-mem(Bytes)/op  delta
BM_Parse/threads:1    40.4k ± 0%              40.4k ± 0%     ~     (all samples are equal)
BM_Parse/threads:2    40.4k ± 0%              40.4k ± 0%     ~     (all samples are equal)
BM_Parse/threads:4    40.4k ± 0%              40.4k ± 0%     ~     (all samples are equal)
BM_Parse/threads:8    40.4k ± 0%              40.4k ± 0%     ~     (all samples are equal)
BM_Parse/threads:12   40.4k ± 0%              40.4k ± 0%     ~     (all samples are equal)

[jcking]

@mike-lischke

[parrt]

If this PR proves/looks useful, @mike-lischke , I'd like to wait to incorporate this into 4.10.

[mike-lischke]

@parrt How much time do we have to review this patch?

I'm hesitant to approve this patch, first, because it is pretty large again, second, it goes to the heart of the runtime access synchronisation and third, I'm not really sure it's correct.

@jcking The 2 mutexes you changed were static, right, but that is necessary because the ATN is shared between all parser instances of the same grammar type. I agree that there's some penalty if you use multiple different parser types at the same time, multithreaded (so, it would make some sense to make these mutexes instance variables). But how often does this really happen? Even if you need to parse different languages, you rarely do this all in parallel.

Your numbers are impressive (at least for time/op, while cycles/op is significantly lower), but I wonder how you took them. Additionally, I don't know what CEL is and what all those times/op, cycles/op etc. mean. What is the op here? One parse run? Did you test with multiple different grammars? How did you run the tests? Instantiate 12 parsers in 12 threads, all for the same grammar, parsing all the same input?

I have not had the time to go fully through the patch, but what certainly jumps out is the removal of the locks around the ATN access, which I think is wrong. Access is no longer serialized there. Can you explain your intention there?

[jcking]

@jcking The 2 mutexes you changed were static, right, but that is necessary because the ATN is shared between all parser instances of the same grammar type. I agree that there's some penalty if you use multiple different parser types at the same time, multithreaded (so, it would make some sense to make these mutexes instance variables). But how often does this really happen? Even if you need to parse different languages, you rarely do this all in parallel.

The mutexes were static and shared between all parser AND lexer instances of ALL grammar types. The scenario is very common. Think of API filtering expressions that you need to parse. You may be handling thousands simultaneously. Currently throughput is heavily restricted. At scale, handling different languages simultaneously also happens. When you are dealing in the millions of files you need to parse of different languages, doing that serially is infeasible.

Your numbers are impressive (at least for time/op, while cycles/op is significantly lower), but I wonder how you took them. Additionally, I don't know what CEL is and what all those times/op, cycles/op etc. mean. What is the op here? One parse run? Did you test with multiple different grammars? How did you run the tests? Instantiate 12 parsers in 12 threads, all for the same grammar, parsing all the same input?

Its based off of https://github.com/google/cel-cpp/blob/master/parser/parser_test.cc. Each iteration is over all the expressions. Multiple thread configurations are used. BM_Parse is the benchmark using google/benchmark. It runs the benchmark function simultaneously in multiple threads. The benchmark was run as the current head of the dev branch for ANTLRv4 vs the current head of the dev branch for ANTLRv4 plus this patch. https://pkg.go.dev/golang.org/x/perf/cmd/benchstat is the summarizer.

I have not had the time to go fully through the patch, but what certainly jumps out is the removal of the locks around the ATN access, which I think is wrong. Access is no longer serialized there. Can you explain your intention there?

No locking was removed. The locks were simply shifted from static to being per ATN. Since all DFAState and ATNState are owned by exactly one ATN, access only needs to be serialized on a per ATN basis. The other data is constant after creation or being added to DFA, DFAState, or ATNState.

I also ran the same benchmark with TSAN enabled and there was no error. I am happy to test other grammars.

[parrt]

@mike-lischke we have time. As you say let's proceed with caution. ATN stuff is scary to change. Running off to teach but around later today and weekend.

[mike-lischke]

After taking a long look at the patch I believe it's a completely fine one. My original impression that locks were removed was based on something I saw while roughly scanning the patch for obvious mistakes. It turned out that only one lock was removed (actually moved up to the caller of that function), so this is all correct.

The only small issue I still have is that comment I made about a large string used for lookup.

[mike-lischke]

This drastically improves performance as the number of threads go up, in some cases as much as 40%.

However, there's a slight speed decrease for single threaded use. Not a big deal, but I wonder where this comes from.

[parrt]

Hi Guys, I decided to look at the locking mechanism in Java and, fortunately, I was good to my future self and wrote a comment which I will paste here for easier retrieval by others. ALL(*) is a super complicated algorithm and I'm surprised I could hold it in my head earlier all at once. haha.

Thread safety in ATN/DFA construction

The ParserATNSimulator locks on the decisionToDFA field when it adds a new DFA object to that array. addDFAEdge() locks on the DFA for the current decision when setting the DFAState.edges field. addDFAState() locks on the DFA for the current decision when looking up a DFA state to see if it already exists. We must make sure that all requests to add DFA states that are equivalent result in the same shared DFA object. This is because lots of threads will be trying to update the DFA at once. The addDFAState() method also locks inside the DFA lock but this time on the shared context cache when it rebuilds the configurations' PredictionContext objects using cached subgraphs/nodes. No other locking occurs, even during DFA simulation. This is safe as long as we can guarantee that all threads referencing s.edge[t] get the same physical target DFAState, or null. Once into the DFA, the DFA simulation does not reference the DFA.states map. It follows the DFAState.edges field to new targets. The DFA simulator will either find DFAState#edges to be null, to be non-null and dfa.edges[t] null, or dfa.edges[t] to be non-null. The addDFAEdge() method could be racing to set the field but in either case the DFA simulator works; if null, and requests ATN simulation. It could also race trying to get dfa.edges[t], but either way it will work because it's not doing a test and set operation.

[parrt]

I'll leave this for further discussion until the coming work week.

[mike-lischke]

Thanks for the explanation @parrt. The task for the C++ target was to find a replacement for Java's synchronized block. There's no C++ equivalent for per-object locks. So we used the second best option: serialize access to those code paths, which use the object which is used in the Java synchronized block. This required some careful checks, but turned out to be pretty easy, as there are only very few code paths that need protection. As you know in C++ everything is a bit more explicit, so we had to define exactly where shared read or locked write access has to happen.

In the current solution the C++ target uses two mutexes, one for DFA state manipulation and the other for edge access control. These mutexes are defined as static members of the ATNSimulator class and hence there are only 2 mutexes in an application that uses any of the simulator classes. This in turn means all access to ATN and DFA goes through these 2 locks, even if there is actually no concurrent access (for example when using two different ATNs for different languages).

Justin now has changed it so that these mutexes became members of the ATN class (not the simulators, as we need to protect the ATN and DFA, not the simulators). So these locks became a per-instance thing and there's no interference between parsers working with different grammars (and hence ATNs). This improves performance for such scenarios.

What's not fully clear to me is where the speed increase in the tested scenarios comes from. I assume Justin only tested parsers all for the same grammar. However the code is constantly checked with TSAN (the thread sanitizer), which will point out (potentially) dangerous code paths in multi-threaded environments. And this patch brought up no warnings, it seems. So, I had no reason not to approve it.

Right after posting the comment I realized what the reason for the speed increase is, even for a single grammar type: the same mutexes are used by both the lexer ATN simulator and the parser ATN simulator. And can so hinder each other. With the per ATN instance locks this is a thing of the past. Both simulators have now own locks, as they don't share the ATN.