-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: use 32bit murmur3 same with java, implement array2dhashset which… #3243
Conversation
… copied from java and reduce hash problem
Can somebody in Go area review? @davesisson @easonlin404 @pboyer ? |
Is anyone focusing on it? |
Can any of the Go folks take a look @davesisson @easonlin404 @pboyer ? I'm afraid I'm not qualified |
Maybe somebody else can review, not necessary Go folks? @ericvergnaud ? It seems like the critical problem. |
I'm a bit skeptical re the inconsistent use of MurmurHash (why is not used by hashATNConfig ?). |
@ericvergnaud Thanks for the review. The main difference of murmur3 is that I changed it to 32bit like the java codes, and the antlr4/runtime/Java/src/org/antlr/v4/runtime/atn/ATNConfigSet.java Lines 40 to 63 in ce3c483
To be honest, I am not familiar with the core antlr implementation, most codes were translated from the Java runtime, try my best. Hope it can help those who are using the Go runtime. |
I concur with @ericvergnaud . The tests pass and improves performance; it also brings it in line with what ANTLR core does. Sam Harwell worked really hard to get that performance up, and using that hash function was important. merging. |
Hi All:
This PR fixes several serious performance problem in Go runtime:
murmur3
implementation in Go SDK has some problem, it's not consistent with Java.Set
implementation which based onmap[int][]interface{}
has poor performance, it is not consistent with Java, I implement theArray2DHashSet
instead, this implementation is same with Java.PredictionContext
.Many problems have been raised, and I think this PR will fix them, please check these issues:
#2888
antlr/grammars-v4#2272
#2152
The core problem in Golang runtime: it wastes too much time on hash code computing because of the incorrect
Murmur3
andSet
andhasher
implementation. Compare the Java and the original Go runtime, Go computes murmur3 ~80,000,000 times, but the Java runtime only ~600,000 times.Here's my benchmark in my MBP '16 2019, the g4 files are copied from https://github.com/antlr/grammars-v4/tree/master/sql/mysql/Positive-Technologies, the original Go runtime cost 4.8s, and this PR costs 171ms.
Benchmark: