-
Notifications
You must be signed in to change notification settings - Fork 28
Perf improvements #1096
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
base: master
Are you sure you want to change the base?
Perf improvements #1096
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.
Pull Request Overview
This PR introduces significant performance optimizations to the Wurst compiler, focusing on reducing compilation time through various algorithmic improvements and memory optimizations. The main goals are to improve scalability for large maps with many imports while maintaining correctness.
- Replaces recursive algorithms with iterative implementations to prevent stack overflows
- Introduces a two-phase validation approach to optimize control flow analysis
- Implements comprehensive caching and memoization strategies
- Switches to high-performance data structures (fastutil library)
Reviewed Changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
SccForwardExecution.java | New SCC-based dataflow analysis implementation for better performance on large functions |
ForwardMethod.java | Updates execution to use new SCC-based approach |
DataflowAnomalyAnalysis.java | Converts recursive local variable collection to iterative |
WurstValidator.java | Implements two-phase validation with light/heavy phases and adds subtype caching |
WurstTypeClass.java | Optimizes interface collection using manual loops instead of streams |
ImTranslator.java | Switches to fastutil data structures and optimizes call relation calculation |
Various other files | Performance improvements including fastutil adoption, iterative algorithms, and memory optimizations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/ILconstArray.java
Outdated
Show resolved
Hide resolved
de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/validation/WurstValidator.java
Show resolved
Hide resolved
de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/jurst/ExtendedJurstLexer.java
Outdated
Show resolved
Hide resolved
de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/jass/ExtendedJassLexer.java
Outdated
Show resolved
Hide resolved
de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/interpreter/State.java
Show resolved
Hide resolved
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.
Pull Request Overview
Copilot reviewed 46 out of 46 changed files in this pull request and generated 5 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/BugTests.java
Outdated
Show resolved
Hide resolved
de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/validation/WurstValidator.java
Outdated
Show resolved
Hide resolved
// visit children left→right: push in reverse | ||
for (int i = e.size() - 1; i >= 0; i--) { | ||
Element c = e.get(i); | ||
if (!(c instanceof ExprClosure) && !(c instanceof ExprStatementsBlock)) { | ||
stack.push(c); | ||
} | ||
} |
Copilot
AI
Sep 29, 2025
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.
[nitpick] The iteration is in reverse order (i--) which suggests intentional left-to-right processing, but this could be clarified with a comment explaining why the reverse iteration is necessary for correct traversal order.
Copilot uses AI. Check for mistakes.
while (!functionStack.isEmpty()) { | ||
ImFunction f = functionStack.pop(); | ||
while (!work.isEmpty()) { | ||
final ImFunction f = work.removeLast(); // LIFO (DFS); change to removeFirst() for BFS |
Copilot
AI
Sep 29, 2025
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.
[nitpick] The comment suggests that changing to removeFirst()
would make this BFS, but the data structure choice and traversal algorithm are more complex than just the removal method. Consider clarifying that DFS vs BFS also depends on how children are added to the queue.
final ImFunction f = work.removeLast(); // LIFO (DFS); change to removeFirst() for BFS | |
final ImFunction f = work.removeLast(); // LIFO (DFS); for BFS, use removeFirst() *and* ensure children are added to the end |
Copilot uses AI. Check for mistakes.
...rstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/interpreter/ILInterpreter.java
Outdated
Show resolved
Hide resolved
…tion/WurstValidator.java Co-authored-by: Copilot <[email protected]>
Also "an other" typo fix and use Arrays.hashCode
…rstScript into perf-improvements
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.
Pull Request Overview
Copilot reviewed 54 out of 70 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/OptimizerTests.java:1
- The condition
v <= 0
should bev < 0
for unary minus operation. Currently,-0
is being converted to0
, but mathematically-0
should remain0
, and the condition should only handle negative values.
package tests.wurstscript.tests;
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
||
|
||
} | ||
|
Copilot
AI
Sep 29, 2025
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.
[nitpick] Missing newline before the new test method. Add a blank line before @Test
to maintain consistent formatting with other test methods in the file.
Copilot uses AI. Check for mistakes.
...urstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/optimizer/SimpleRewrites.java
Show resolved
Hide resolved
...eeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/ImPrinter.java
Outdated
Show resolved
Hide resolved
…ation/imtranslation/ImPrinter.java Co-authored-by: Copilot <[email protected]>
…rstScript into perf-improvements
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.
Pull Request Overview
Copilot reviewed 54 out of 70 changed files in this pull request and generated 5 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
"endpackage"); | ||
String compiledAndOptimized = Files.toString(new File("test-output/OptimizerTests_test_unused_func_remover_opt.j"), Charsets.UTF_8); | ||
assertFalse("I2S should be removed", compiledAndOptimized.contains("I2S")); | ||
assertFalse(compiledAndOptimized.contains("I2S"), "I2S should be removed"); |
Copilot
AI
Sep 29, 2025
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.
[nitpick] The updated TestNG assertion syntax reverses the parameter order from the old AssertJUnit style. Consider updating all assertions consistently throughout the codebase to use the new syntax pattern assertX(actual, expected, message)
for better consistency.
Copilot uses AI. Check for mistakes.
Queue<WStatement> worklist = new ArrayDeque<>(scc); | ||
|
||
int iterations = 0; | ||
int maxIterations = scc.size() * scc.size() + 100; // Heuristic limit to prevent infinite loops |
Copilot
AI
Sep 29, 2025
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.
[nitpick] The magic number 100 in the iteration limit calculation should be extracted as a named constant to improve code readability and maintainability. Consider defining private static final int ITERATION_BUFFER = 100;
Copilot uses AI. Check for mistakes.
// crude cap to avoid unbounded growth; tune as needed | ||
private static final int SUBTYPE_MEMO_CAP = 16_384; |
Copilot
AI
Sep 29, 2025
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.
[nitpick] The cache size limit of 16,384 should be made configurable or at least documented with reasoning for this specific value. Consider adding a comment explaining the memory/performance trade-off that led to this choice.
// crude cap to avoid unbounded growth; tune as needed | |
private static final int SUBTYPE_MEMO_CAP = 16_384; | |
/** | |
* Maximum number of cached subtype checks before eviction. | |
* Default is 16,384, chosen to balance memory usage and performance for typical WurstScript projects. | |
* If the cache grows beyond this, it is fully cleared (simple eviction policy). | |
* You can override the default by setting the system property "wurst.subtypeMemoCap". | |
*/ | |
private static final int SUBTYPE_MEMO_CAP = Integer.getInteger("wurst.subtypeMemoCap", 16_384); |
Copilot uses AI. Check for mistakes.
return callRelations; | ||
} | ||
|
||
public ImFunction getMainFunc() { return mainFunc; } |
Copilot
AI
Sep 29, 2025
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.
[nitpick] These simplified getter methods should include null-safety documentation or add null annotations to clarify the contract, since the original methods were more explicit about potential null returns.
public ImFunction getMainFunc() { return mainFunc; } | |
public ImFunction getMainFunc() { return mainFunc; } | |
@Nullable |
Copilot uses AI. Check for mistakes.
// take only the last 3 digits | ||
int v = h % 1000; |
Copilot
AI
Sep 29, 2025
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.
[nitpick] The hash collision potential increases significantly when reducing to only 3 digits (1000 possible values). Consider using a larger range or document the trade-off between readability and collision probability.
// take only the last 3 digits | |
int v = h % 1000; | |
// take only the last 5 digits | |
int v = h % 100000; |
Copilot uses AI. Check for mistakes.
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.
Pull Request Overview
Copilot reviewed 86 out of 102 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...rc/main/java/de/peeeq/wurstscript/intermediatelang/optimizer/ConstantAndCopyPropagation.java
Outdated
Show resolved
Hide resolved
de.peeeq.wurstscript/src/main/java/de/peeeq/wurstio/languageserver/requests/MapRequest.java
Show resolved
Hide resolved
de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/WLogger.java
Outdated
Show resolved
Hide resolved
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.
Pull Request Overview
Copilot reviewed 85 out of 101 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...eeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/ImPrinter.java
Outdated
Show resolved
Hide resolved
if (e.getAnnotationMessage() != null && e.getAnnotationMessage().length() >= 1) { | ||
sb.append("("); | ||
sb.append(e.getAnnotationMessage()); | ||
sb.append(Utils.escapeString(e.getAnnotationMessage())); |
Copilot
AI
Oct 1, 2025
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.
[nitpick] Using Utils.escapeString()
on annotation messages could introduce security vulnerabilities if the escaping is not comprehensive. Ensure that all potentially dangerous characters are properly escaped to prevent injection attacks.
Copilot uses AI. Check for mistakes.
@SuppressWarnings("unchecked") final ObjectOpenHashSet<ImVar>[] in = new ObjectOpenHashSet[N]; | ||
@SuppressWarnings("unchecked") final ObjectOpenHashSet<ImVar>[] out = new ObjectOpenHashSet[N]; |
Copilot
AI
Oct 1, 2025
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.
Using generic arrays with @SuppressWarnings
is an anti-pattern. Consider using List<ObjectOpenHashSet<ImVar>>
instead of arrays to avoid unchecked warnings and improve type safety.
Copilot uses AI. Check for mistakes.
de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/validation/WurstValidator.java
Show resolved
Hide resolved
…ation/imtranslation/ImPrinter.java Co-authored-by: Copilot <[email protected]>
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.
Pull Request Overview
Copilot reviewed 134 out of 154 changed files in this pull request and generated 5 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/OptimizerTests.java
Show resolved
Hide resolved
de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/OptimizerTests.java
Show resolved
Hide resolved
...script/src/main/java/de/peeeq/wurstscript/translation/imtranslation/StackTraceInjector2.java
Outdated
Show resolved
Hide resolved
...stscript/src/main/java/de/peeeq/wurstscript/intermediatelang/optimizer/FunctionSplitter.java
Outdated
Show resolved
Hide resolved
...eeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imoptimizer/ImOptimizer.java
Show resolved
Hide resolved
…ediatelang/optimizer/FunctionSplitter.java Co-authored-by: Copilot <[email protected]>
…ation/imtranslation/StackTraceInjector2.java Co-authored-by: Copilot <[email protected]>
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.
Pull Request Overview
Copilot reviewed 134 out of 154 changed files in this pull request and generated 6 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
" println(I2S(blablub))", | ||
"endpackage"); | ||
String output = Files.toString(new File("./test-output/OptimizerTests_test_tempVarRemover2_inlopt.j"), Charsets.UTF_8); | ||
// Better not inline GetRandomInt call - it might have side effects! |
Copilot
AI
Oct 2, 2025
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.
[nitpick] The comment has a grammatical issue. It should be 'Better not to inline GetRandomInt call' or 'It's better not to inline GetRandomInt call'.
// Better not inline GetRandomInt call - it might have side effects! | |
// It's better not to inline GetRandomInt call - it might have side effects! |
Copilot uses AI. Check for mistakes.
while (!worklist.isEmpty()) { | ||
if (iterations++ > maxIterations) { | ||
// This should ideally not happen in a correct CFG with a monotonic transfer function | ||
throw new RuntimeException("Dataflow analysis did not converge. Possible infinite loop in component."); |
Copilot
AI
Oct 2, 2025
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.
The error message could be more helpful by including details about which component failed to converge and the current iteration count.
throw new RuntimeException("Dataflow analysis did not converge. Possible infinite loop in component."); | |
throw new RuntimeException( | |
"Dataflow analysis did not converge after " + iterations + " iterations (max allowed: " + maxIterations + "). " + | |
"Possible infinite loop in component: " + scc | |
); |
Copilot uses AI. Check for mistakes.
r.add((LocalVarDef) e); | ||
} | ||
private void collectLocalVars(Set<LocalVarDef> out, Element root) { | ||
ArrayDeque<Element> stack = new ArrayDeque<>(); |
Copilot
AI
Oct 2, 2025
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.
[nitpick] The iterative implementation is good for avoiding stack overflow, but consider using a more specific initial capacity for the ArrayDeque based on expected tree depth to reduce allocations.
ArrayDeque<Element> stack = new ArrayDeque<>(); | |
ArrayDeque<Element> stack = new ArrayDeque<>(32); |
Copilot uses AI. Check for mistakes.
// crude cap to avoid unbounded growth; tune as needed | ||
|
||
private static boolean isSubtypeCached(WurstType actual, WurstType expected, Annotation site) { | ||
if (actual == expected) return true; |
Copilot
AI
Oct 2, 2025
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.
[nitpick] The reference equality check actual == expected
before equalsType
is a good optimization, but ensure this doesn't bypass necessary type checking logic that might be required even for the same reference.
if (actual == expected) return true; |
Copilot uses AI. Check for mistakes.
// If no folding happened, try regular value propagation | ||
if (newValue == null) { |
Copilot
AI
Oct 2, 2025
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.
[nitpick] Consider extracting the constant folding logic into a separate method to improve readability of the transfer function.
Copilot uses AI. Check for mistakes.
return t.getPredecessors(); | ||
} | ||
}; | ||
// Use the path-based strong component algorithm [1] on the reversed CFG. |
Copilot
AI
Oct 2, 2025
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.
The reference '[1]' is incomplete. Consider providing the full citation for the path-based strong component algorithm being referenced.
// Use the path-based strong component algorithm [1] on the reversed CFG. | |
// Use the path-based strong component algorithm (see: R. Tarjan, "Depth-First Search and Linear Graph Algorithms", SIAM J. Comput., 1972) on the reversed CFG. |
Copilot uses AI. Check for mistakes.
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.
Pull Request Overview
Copilot reviewed 134 out of 154 changed files in this pull request and generated 5 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
" function GetRandomIntt(int a, int b) returns int", | ||
" return a + b", | ||
" init", | ||
" let blablub = GetRandomIntt(0,100)", |
Copilot
AI
Oct 2, 2025
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.
Function name has extra 't' - should be 'GetRandomInt' not 'GetRandomIntt'.
" function GetRandomIntt(int a, int b) returns int", | |
" return a + b", | |
" init", | |
" let blablub = GetRandomIntt(0,100)", | |
" function GetRandomInt(int a, int b) returns int", | |
" return a + b", | |
" init", | |
" let blablub = GetRandomInt(0,100)", |
Copilot uses AI. Check for mistakes.
package de.peeeq.wurstscript.intermediatelang.optimizer; | ||
|
||
import de.peeeq.datastructures.Worklist; | ||
import de.peeeq.datastructures.GraphInterpreter; |
Copilot
AI
Oct 2, 2025
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.
Excellent use of SCC-based analysis for liveness calculation. This provides significant performance improvements over naive worklist algorithms by containing iterative analysis within strongly connected components.
import de.peeeq.datastructures.GraphInterpreter; |
Copilot uses AI. Check for mistakes.
private enum Phase { LIGHT, HEAVY } | ||
private Phase phase = Phase.LIGHT; |
Copilot
AI
Oct 2, 2025
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.
Two-phase validation approach is a smart optimization - separating lightweight checks from expensive control-flow and dataflow analysis reduces overall compilation time.
Copilot uses AI. Check for mistakes.
ImIntVal newVal = JassIm.ImIntVal(inverseVal); | ||
opc.replaceBy(newVal); | ||
int v = ((ImIntVal) expr).getValI(); | ||
if (v != Integer.MIN_VALUE && v <= 0) { |
Copilot
AI
Oct 2, 2025
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.
The condition v <= 0
for unary minus folding is incorrect. Unary minus should fold for any representable value except Integer.MIN_VALUE. The condition should be v != Integer.MIN_VALUE
.
if (v != Integer.MIN_VALUE && v <= 0) { | |
if (v != Integer.MIN_VALUE) { |
Copilot uses AI. Check for mistakes.
private static boolean cache = false; | ||
private final ProgramState globalState; | ||
private final TimerMockHandler timerMockHandler = new TimerMockHandler(); | ||
|
Copilot
AI
Oct 2, 2025
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.
Removed cache parameter from constructor - this is a breaking change that could affect external code using this API.
/** | |
* @deprecated The cache parameter is no longer used. Use {@link #ILInterpreter(ImProg, WurstGui, Optional, ProgramState)} instead. | |
*/ | |
@Deprecated | |
public ILInterpreter(ImProg prog, WurstGui gui, Optional<File> mapFile, Object cache, ProgramState globalState) { | |
this(prog, gui, mapFile, globalState); | |
} |
Copilot uses AI. Check for mistakes.
Main improvements:
-noExtractMapScript
option thewar3map.j
no longer gets forcefully reparsed on every runmap command (it's already part of the model)