Fix the prediction cache and the comparison algorithms from the legacy code #4187
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
- I missed this on my first evaluation of all the collections used by the code I inherited. The PredictionCache was implemented as a map using PredictionContext pointers as the key. This meant that there would never be a cache hit and the cache woudl just accumulate massive numbers of PredictionContext 'objects'! Bloody hell. Sometimes it is difficult to spot such glaring errors. This change vastly reduces memory usage. The next commit will fix the visited context look up which also suffers from the same problem. Fixes: antlr#3934 It probably also fixes a ton of performance and memory usage problems, which I have yet to test. Signed-off-by: Jim.Idle <jimi@idle.ws>
- Note, I think that this still needs work, but it is a lot better memory wise. Signed-off-by: Jim.Idle <jimi@idle.ws>
Some of the legacy code that tried to implement comparisons and hashing like Java was just plain wrong. That stuff is buried deep in there and took a massive effort to work out what was going wrong. In the end, while the hash codes were OK for DFAState, the comparisons inherited from the old code were just plain wrong for certain objects. With those corrected, there is a massive improvement in performance all around. I will push a PR for this stuff now, but I already see some massive potential gains by getting rid of interfaces where we don't need them stopping the use of pointers everywhere indiscrimiately. Next task is going to be pure performance improvements until the Go runtime takes its rightful place as the fastest implementation ;) Signed-off-by: Jim.Idle <jimi@idle.ws>
Signed-off-by: Jim.Idle <jimi@idle.ws>
- I will of course fix this in a follow up PR, but the test DropLoopEntryBranchInLRRule_4 currently hangs. It is also disabled for many other runtimes, so I think it is fine to get this PR in and then follow up with a fix for this test. Signed-off-by: Jim.Idle <jimi@idle.ws>
|
@parrt I could also do with this PR merging. I will have quite a few coming fast and furious new. Cheers. |
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.
Fixes: #3934
Closes: #3934
While this is not the end of performance improvements and probably not the end of bug fixes for some of the structs that we are collecting, this PR massively (orders of magnitude) improves the performance characteristics of the Go runtime and also fixes the ATN prediction context cache, that just wasn't working.
This PR disables one test that is also disabled for many other targets. I will fix that in a follow-up PR.
This is ready to merge, with my other PRs (except my testing improvements - I must fix some minor issues for Ivan.