Don't Panic! 50% plus performance improvement #4192
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… flowcontrol - 50% performance improvement
- Prior to this change, a recognition error was tracked by performing a panic(),
which the generated code for rules would then use recover() to discover. However,
recover() is not like catch(){} in Java and is expensive to run even if there is
no panic() to find on the execution stack. Eliminating this and doing a simple check
at the end of rule execution brings with it a massive performance improvement up to
50% of CPU execution time. Now that collections and context caching is working correctly
this is a significant improvement in execution time.
Signed-off-by: Jim.Idle <jimi@idle.ws>
Prior to this change, the runtime and generated code was using panic() and recover() to perform error checking and reporting. This is extremely expensive and just not the way to do it. This change now uses goto, and explicit checking for error state after selected calls into the runtime. This has greatly improved parser performance. Using the test code provided by a recent performance issue report, the parse is now twoice as fast as the issue raised was hoping for. Signed-off-by: Jim.Idle <jimi@idle.ws>
|
Seems OK to me. It looks like this would only improve performance when there's recognition Errors however right? |
|
No mate. The generated code was using a deferred function, well, two
actually. Try catch is free in c++ for instance, but the deferred funcs in
go cost on their own, and performance of `recover()` has, rightly, got more
expensive in deference to `defer()`. I basically reset to 17 years ago when
I used goto with C :)
This is >50% for all grammars. I was going to test against the Java target
today, as I now think go is at least on a par. Idly, perhaps on a Parr. But
I think I’ll let @kaby76 Ken have a mosey and look at his results.
…On Sun, Mar 19, 2023 at 02:02 Terence Parr ***@***.***> wrote:
Seems OK to me. It looks like this would only improve performance when
there's recognition Errors however right?
—
Reply to this email directly, view it on GitHub
<#4192 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJ7TMFLIK27Z52QSREUW7TW4X2EXANCNFSM6AAAAAAV7LMU6A>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
I'll be testing this today. K |
|
I guess the tool has changed: This is going to take a little time to work out. The scripts not only need to use a specific repo/branch of the runtime, but it will also need to generate a tool specific branch of Antlr because the template was updated. Currently, the perf scripts work against what is published, which limits things a bit. |
|
yay! back to the 80s with goto. love it. |
|
Yes - you need to rebuild the tool as the codegen has changed quite a bit.
It will probably change more before the next release
…On Sun, Mar 19, 2023 at 9:51 PM Ken Domino ***@***.***> wrote:
I guess the tool has changed:
$ make
bash build.sh
go: added github.com/antlr/antlr4/runtime/Go/antlr/v4 v4.0.0-20230318180256-adbe946416f5
go: added golang.org/x/exp v0.0.0-20220722155223-a9213eeb770e
# example.com/myparser/parser
parser\java_parser.go:1495:67: not enough arguments in call to p.GetInterpreter().AdaptivePredict
have (antlr.TokenStream, number, antlr.ParserRuleContext)
want (*antlr.BaseParser, antlr.TokenStream, int, antlr.ParserRuleContext)
This is going to take a little time to work out. The scripts not only need
to use a specific repo/branch, but it will also need to generate a tool
specific branch of Antlr because the template was updated. Currently, the
perf scripts work against what is published, which limits things a bit.
—
Reply to this email directly, view it on GitHub
<#4192 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJ7TMHCURJH72PQTJCUXZTW44FMLANCNFSM6AAAAAAV7LMU6A>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
before.tar.gz -- Generated Go parser driver with "master" reference. after.tar.gz -- Generated Go parser driver with "dev" reference. I've been adjust my scripts to work with any repo/branch as source in my driver generating code. In the meanwhile, I am just gathering some data for a sanity check, to make sure I am referencing the right version of the code. Unfortunately, I do not see any performance increase with the "dev" branch Go runtime over version 4.12. In fact, I now see some grammars/input no longer terminating (killed after 5 minutes vs. before completed in 55s). grammars-v4/sql/mysql/Positive-Technologies, examples/bitrix_queries_cut.sql. I am using the published 4.12 version vs. the "dev" version. Please check. The two .tar.gz files are generated parsers for Windows Msys. (I never use ridiculous cmd or Powershell if I can help it.) I see similar behavior over on Ubuntu. I will check to make sure there are no diffs in the git clone images between using a git on the command line versus what go thinks it did over in its cache area. |
|
It's possible that the grammars expose a hidden bug and don't terminate.
But if that is the case, the other grammars should be showing a a big
improvement. That's the case in my local system. BUt I will of course work
out why the bitrix queries parser is hanging.
…On Mon, Mar 20, 2023 at 7:36 AM Ken Domino ***@***.***> wrote:
I've been adjust my scripts but just looking over the
grammars-v4/java/java grammar, In the meanwhile, I am just gathering some
data for a sanity check. Unfortunately, I do not see and performance
increase with the "dev" branch Go runtime. In fact, I now see some
grammars/input no longer terminating (killed after 5 minutes vs. before
completed in 55s). grammars-v4/sql/mysql/Positive-Technologies
<https://github.com/antlr/grammars-v4/tree/a244fda17cfdbd7ae1443ce2db87092f69f59e69/sql/mysql/Positive-Technologies>,
examples/bitrix_queries_cut.sql
<https://github.com/antlr/grammars-v4/blob/a244fda17cfdbd7ae1443ce2db87092f69f59e69/sql/mysql/Positive-Technologies/examples/bitrix_queries_cut.sql>.
I am using the published 4.12 version vs. the "dev" version. Please check.
—
Reply to this email directly, view it on GitHub
<#4192 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJ7TMAHU7F7RUPRZ5LGMLTW46J7JANCNFSM6AAAAAAV7LMU6A>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
I tried grammars-v4/java/java first, and saw a slight perf slowdown for the dev-branch, too. That's when I tried the MySQL grammar. I'll try some other grammars. |
|
OK - I may still have work to do then with these. Every grammar that I have
tried has been very much including the recently reported one that supplied
the grammar and test inputs. That parser was taking 13 seconds, and now
takes 70-90ms on my system.
It could always be the case that there are other problems that I did not
hit with the grammars I used of course, but I can't see how/why the
performance would slow down at the moment.
…On Mon, Mar 20, 2023 at 9:44 AM Ken Domino ***@***.***> wrote:
I tried grammars-v4/java/java first, and saw a slight perf slowdown for
the dev-branch, too. That's when I tried the MySQL grammar. I'll try some
other grammars.
—
Reply to this email directly, view it on GitHub
<#4192 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJ7TMHXZ2PXNB2QJIQ2T3LW46ZBVANCNFSM6AAAAAAV7LMU6A>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
feat: Another 50%+ performance improvement
Prior to this change, the runtime and generated code was using panic() and recover()
to perform error checking and reporting. This is extremely expensive and just not the
way to do it.
This change now uses goto, and explicit checking for error state after selected calls
into the runtime. This has greatly improved parser performance. Using the test code
provided by a recent performance issue report, the parse is now twice as fast as the
issue raised was hoping for.
This change may require code changes by users if they have added Go code in to their
parsers and have declared variables at random points in actions. They merely have to
declare the variables at the start of the rule. This is a minor price to pay for the massive
performance improvement.
@parrt If you can merge this when you see it, I have many more performance improvements
to send your way.
@kaby76 If you have time to run any of your performance tests, I would appreciate your feedback sir.