-
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
[Go] Thread-safe ANTLR codegen #2816
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTm
@pboyer would you be able to review and approve this change? Thanks! |
Thanks @TristonianJones! I approve of this change! Like you said, this will cause significant performance regressions in clients that naively upgrade. We should mention this in the release notes. IMHO, the benefits clearly outweigh the drawbacks. |
Excellent @pboyer! Thanks for the review. I've been using ANTLR since v2, in one capacity or another, and I'm happy to be contributing back. I agree this is release notes worthy. I debated adding a generated What are the next steps here? Should I add @parrt in order to merge the changes? |
@pboyer I think you are in charge of this target, right? Should I merge? |
Yup, merge. |
The ANTLR Go codegen allocates mutable global variables which can result in data races as evidenced in google/cel-go#177 and reported in #2040. As a local workaround, global variables in the generated source were shifted into local variables within the
NewLexer
andNewParser
functions.While the change made the generated code thread-safe, it came at a significant CPU and memory overhead in parse-heavy call paths. However, exporting the
SetInputStream
method on thelexer.go
makes it possible to usesync.Pool
objects with the modified codegen artifacts and reduces the parsing overhead to within striking distance of the prior non-thread-safe code.To take advantage of such thread-safety and object reuse within ANTLR Go applications, developers would need to regen their grammars and incorporate the following pattern of use within their own code base:
All Go-related tests have been run and executed successfully following this change: