Rewrite LocalSplitter
#841
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
Debug names can cause different locals (with different local indices) to be given the same name. This is probably not correct, but the `LocalSplitter` has to support it anyway.
Timbals
force-pushed
the
localsplitter
branch
from
8083299 to
337de9c
Compare
- use `graph.getNodes()` instead of `builder.getStmts()` because the linearized ordering is not important for the `LocalSplitter` (it uses `successors`/`exceptionalSuccessors` to step through the statements) - exit early when there is only one assignment or when the local can't be split - use a stack instead of a queue (iteration order makes no difference) - only search through all statements for definitions once instead of repeating it for every local Potential speedups still remaining: - Calls to `successors` use `indexOf` which can be slow; this only affects cases where there are thousands of statements without any control flow and heavy re-use of locals - `getUses`, `getDefs` and `getUsesAndDefs` show up in the profiler; these could probably be cached - Most of the time is spent hashing. There are ways to improve this.
Previously used `getNodes` instead of `getStmts` for a small performance improvement. This unintentionally made the naming of locals non-deterministic.
Timbals
force-pushed
the
localsplitter
branch
from
da75d07 to
3cd6721
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #841 +/- ##
=============================================
+ Coverage 63.74% 64.04% +0.30%
- Complexity 3417 3421 +4
=============================================
Files 312 312
Lines 15086 15045 -41
Branches 2559 2542 -17
=============================================
+ Hits 9616 9636 +20
+ Misses 4557 4502 -55
+ Partials 913 907 -6 ☔ View full report in Codecov by Sentry. |
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.
This PR rewrites the
LocalSplitterto use a simpler algorithm.The algorithm uses a disjoint set to find any definitions for locals that can't be split because they have overlapping uses.
Then it splits the locals by modifying the statements in the graph.
I also did some easy performance optimization. Running the
LocalSplitterover the entire Java Runtime Library (roughly 200_000 methods of varying length and complexity) takes around 10 seconds on my machine. That seems adequate for now.There is definitely more room for optimization here (potential for caching results, reduce the amount of hashing done, reduce the amount of expensive
indexOfcalls).Closes #798
Fixes the problems with the LocalSplitter from #716