fix: run the Spoon pipeline on a thread with a configurable stack size - #6882
Open
monperrus wants to merge 2 commits into
Open
fix: run the Spoon pipeline on a thread with a configurable stack size#6882monperrus wants to merge 2 commits into
monperrus wants to merge 2 commits into
Conversation
Spoon's visitors are recursive: they use one JVM frame per AST node on the path from the root to the node being visited. Legal but deeply nested expressions, such as the ~32,000 binary operators of OpenJDK's test/langtools/tools/javac/DeepStringConcat.java, therefore exhaust the default thread stack of a few hundred kilobytes. The recursion is not confined to one visitor: AstParentConsistencyChecker, ProcessingVisitor, EarlyTerminatingScanner (model queries), CloneVisitor, EqualsVisitor and DefaultJavaPrettyPrinter all overflow on the same input. Rather than rewriting each of them iteratively, Launcher now runs buildModel(), process(), prettyprint() and run() on a thread whose stack size is Environment#getStackSize(), which defaults to 64 MB. The stack is reserved address space, not committed memory, so this costs nothing on ordinary models. Setting it to 0 restores the previous behaviour for embedders that manage their own threads. Measured on the reproducer: 16 MB is enough for every operation; the whole pipeline, including pretty-printing, runs in about 5 s. Fixes INRIA#6804 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
|
This has the potential of a significant slowdown when invoking the relevant methods for small elements, right? On Java 21, we could use virtual threads which would be cheaper in that aspect. Other than that, are there more sophisticated patterns to avoid stack overflows for deep visitor-based interactions? |
This file contains hidden or 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
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.
Summary
Alternative to #6805 for #6804: give the Spoon pipeline a stack proportional to the problem, instead of rewriting individual visitors iteratively.
Problem
Spoon's visitors are recursive — one JVM frame per AST node on the path from the root to the node being visited. OpenJDK's valid
DeepStringConcat.javahas ~32,000 binary operators in one expression, which exhausts the default thread stack of a few hundred kilobytes.AstParentConsistencyCheckerandProcessingVisitorare the two visitors named in the issue, but they are only the first two to be reached. On currentmaster, with the 32k-operator file:buildModel()— parent checkprocess()— processorsprettyprint()getElements(filter)root.toString()root.clone()equalsThe recursion lives in
CtScanner,EarlyTerminatingScanner,LexicalScopeScanner,CloneVisitor,EqualsVisitor,DefaultJavaPrettyPrinterand every user-writtenCtScannersubclass. Binary operators are also only the easiest deep chain — nestedif/elseand long.a().b().c()chains recurse the same way. Making visitors iterative one at a time does not converge.Change
LauncherrunsbuildModel(),process(),prettyprint()andrun()on a thread whose stack size isEnvironment#getStackSize().setStackSize(0)runs the pipeline on the calling thread, restoring today's behaviour for embedders that manage their own threads.run()spawns one thread, not four.RuntimeExceptionorErroris rethrown as-is, anything else is wrapped inSpoonException.Environmentmethods aredefault, so third-partyEnvironmentimplementations keep compiling.+140 lines of main code, against 373 in #6805, and it covers seven operations rather than two.
Known limitation
A user calling
element.toString()orgetElements()on their own thread, outside the launcher, is not covered — they can size their own thread, andEnvironment#getStackSize()'s Javadoc says so. Nothing short of rewriting every visitor covers that case.Validation
DeepExpressionTest#testDeepExpressionPipelinebuilds, processes and pretty-prints the 32k-operator expression, then asserts that the processor saw all 32,000 operators and that the printed file contains all 32,000+. It runs in ~6 s.setStackSize(0)the same test fails withStackOverflowError.setStackSize(0), single-thread reuse across nested steps, and rejection of a negative stack size.