Repository navigation
Conversation
TestJavaPatternPerformance runs java pattern operations over a deterministic, synthetic codebase (BenchmarkCorpus) in two scenarios: - api-migration: 3 patterns migrating legacy APIs (a generic method, a constructor, and a pattern with a @substitute method), whose method names appear in a minority of files - jdk-idioms: 2 patterns (3 @javapatterns) replacing JDK idioms, whose method names appear in almost every file The corpus contains near misses (same method names, different shape or types) as well as matches. Each scenario is timed: - end-to-end: AstraCore.run over a fresh copy of the corpus - matching: AstraCore#runOperations(Set, CompilationUnit) over pre-parsed compilation units, isolating matching from parsing. That method is made protected (like AstraCore's other extension points) so the benchmark measures AstraCore's own code rather than a copy of it. Every run checks that exactly the expected files changed, that all matches were replaced and no near misses were, and reports a digest of the output so that runs before and after a change can be shown to produce identical output. By default it runs as a quick functional test over 20 files. To measure: mvn -pl astra-core test -Dtest=TestJavaPatternPerformance \ -Dastra.performance=true "-DargLine=-Xms2g -Xmx2g -XX:+AlwaysPreTouch" Baseline (400 files, 3 warmup + 8 measured iterations, 1 thread, median): api-migration end-to-end 3774 ms matching 624 ms jdk-idioms end-to-end 4000 ms matching 541 ms Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pcf5Hu86PtKtfXhaAJrJDA
Parsing with bindings is by far the most expensive part of a run, and a java pattern can only match source code that contains the names of the methods it invokes and the types it instantiates (other than those it captures: @substitute methods, parameters, type variables and arguments after a varargs parameter). - ASTOperation gets a default getContentPrefilteringPredicate(), which accepts every file. Returning false promises run() would do nothing for any node of that file. - AstraCore only parses a file if the use case's content predicate accepts it AND at least one operation's predicate does. With the default predicate, nothing changes for existing operations. - JavaPatternASTOperation rejects files which don't contain all of the identifiers required by at least one of its patterns. The identifiers are found by JavaPatternRequiredIdentifiers, following how JavaPatternASTMatcher compares nodes. Content containing unicode escapes is always accepted, and a pattern whose bindings can't be resolved requires nothing. Benchmark (median of 8, 400 files): end-to-end matching api-migration 3774 ms -> 1535 ms 624 ms -> 618 ms (unchanged) jdk-idioms 4000 ms -> 3950 ms 541 ms -> 512 ms (unchanged) jdk-idioms is unaffected because its method names appear in every file. The output digests are identical to the baseline. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pcf5Hu86PtKtfXhaAJrJDA
When matching a MethodInvocation from a java pattern, the method name was compared last: after resolving both method bindings to compare parameter counts and after matching (and type checking) every argument. Most candidate invocations call a different method, so almost all of that work was wasted. The names are now compared first. This only rejects candidates that the full match would also reject, so a different name is still allowed where the name is captured rather than compared: @substitute methods, names of pattern parameters, and (to leave that case unchanged) pattern methods whose binding can't be resolved. Benchmark (median of 8, 400 files): end-to-end matching api-migration 1535 ms -> 1465 ms 618 ms -> 466 ms jdk-idioms 3950 ms -> 3952 ms 512 ms -> 420 ms The output digests are identical to the baseline. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pcf5Hu86PtKtfXhaAJrJDA
AstraCore passes every visited node (simple names, types, declarations,
...) to every operation, and for every node and pattern a new
JavaPatternMatcher was created, only for the ASTMatcher method for the
pattern's node type to reject a node of any other type immediately.
matchAndCapture now skips a pattern when the candidate is a different type
of node, using the same instanceof test that every ASTMatcher.match(T,
Object) starts with (no concrete JDT DOM node class extends another, so
this is exactly equivalent). Patterns which are names are always tried,
as JavaPatternMatcher matches names more leniently.
Benchmark (median of 8, 400 files):
end-to-end matching
api-migration 1465 ms -> 1534 ms 466 ms -> 404 ms
jdk-idioms 3952 ms -> 3788 ms 420 ms -> 338 ms
End-to-end changes here are within run to run variation. The output
digests are identical to the baseline.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pcf5Hu86PtKtfXhaAJrJDA
For every candidate node, the matcher re-derived facts that only depend on the pattern: - finding a pattern parameter by name streamed over the parameters, calling toString() (which runs an AST flattener) on both names each time - checking whether a pattern invocation calls a @substitute method resolved its binding and compared it with every substitute method - checking whether an invocation's name is captured (added by the previous commit) did both of the above, for every candidate invocation with a different name SingleASTNodePatternMatcher now works these out once, when the pattern is parsed: parameters are looked up in a map by identifier, and the invocations of substitute methods, and those whose names are captured, are kept in identity sets. Besides the repeated work, this avoids resolving the pattern's bindings during matching: binding resolution synchronizes on the pattern's binding resolver, which every thread shares. The matcher no longer needs the substitute methods themselves, so they are no longer passed to it. Benchmark (median of 8, 400 files): end-to-end matching api-migration 1534 ms -> 1444 ms 404 ms -> 322 ms jdk-idioms 3788 ms -> 3700 ms 338 ms -> 284 ms The output digests are identical to the baseline. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pcf5Hu86PtKtfXhaAJrJDA
JavaPatternASTOperation.run created a JavaPatternASTMatcher (and a list
for its matches) for every node in every file, and matchAndCapture then
created a JavaPatternMatcher (with four maps) for every candidate of the
same node type as a pattern, including invocations of differently named
methods, which the matcher then rejects straight away.
The cheap checks are now made before creating anything:
SingleASTNodePatternMatcher.couldMatch checks the node type and, for a
pattern which is a method invocation, the method name (unless it's
captured), which are exactly the first checks the matcher makes. run()
returns early if no pattern could match the node, and matchAndCapture only
creates matchers for the patterns that could.
Benchmark (median of 8, 400 files):
end-to-end matching
api-migration 1444 ms -> 1397 ms 322 ms -> 304 ms
jdk-idioms 3700 ms -> 3716 ms 284 ms -> 277 ms
A small gain: most of the remaining matching time is now spent in
ClassVisitor, rather than in the java pattern operation. End-to-end
changes are within run to run variation. The output digests are identical
to the baseline.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pcf5Hu86PtKtfXhaAJrJDA
AstraCore#runOperations called ClassVisitor#getVisitedNodes() inside the
loop over operations. That builds a new set from all of the visitor's
lists every time, so with several operations (for example several java
pattern files in one use case) the same set was rebuilt for each one.
The set is now built once per compilation unit. It contains the same
nodes, built in the same way, so each operation still visits the same
nodes in the same order.
Benchmark (median of 8, 400 files):
end-to-end matching
api-migration 1397 ms -> 1448 ms 304 ms -> 275 ms (3 operations)
jdk-idioms 3716 ms -> 3701 ms 277 ms -> 252 ms (2 operations)
End-to-end changes are within run to run variation. The output digests
are identical to the baseline.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pcf5Hu86PtKtfXhaAJrJDA
Every visit method of ClassVisitor (and MethodDeclarationVisitor) logged
with log.debug("..." + node). The message was built even with debug
logging disabled, and building it calls ASTNode.toString(), which
flattens the node's entire subtree back into source code. As the visitor
visits nested nodes (type, then method, then statement, ...), most of a
file was flattened many times over, for every file AstraCore processes.
The messages now use SLF4J's parameterized logging, which only calls
toString() when debug logging is enabled, giving the same message when it
is.
ClassVisitor runs over every file processed by any use case, so this
helps all operations, not just java patterns.
Benchmark (median of 8, 400 files):
end-to-end matching
api-migration 1448 ms -> 1366 ms 275 ms -> 157 ms
jdk-idioms 3701 ms -> 3538 ms 252 ms -> 155 ms
The output digests are identical to the baseline.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pcf5Hu86PtKtfXhaAJrJDA
| operationPredicates.add(operation.getContentPrefilteringPredicate()); | ||
| } | ||
| return content -> operationPredicates.stream().anyMatch(predicate -> predicate.test(content)); | ||
| } |
There was a problem hiding this comment.
Not entirely convinced this is safe for all operations...
There was a problem hiding this comment.
I think that this is equivalent to creating a single predicate by using Predicate.and(..) on the whole chain. I can't see why this wouldn't be safe.
There was a problem hiding this comment.
Agreed. As an aside, perhaps this method could be condensed to this? Avoids the intermediate List.
return operations.stream()
.map(ASTOperation::getContentPrefilteringPredicate)
.anyMatch(predicate -> predicate.test(content));
| @Override | ||
| public boolean visit(SimpleName node) { | ||
| log.debug("Simple name: " + node); | ||
| log.debug("Simple name: {}", node); |
| * so this lets cheap text checks avoid the cost of parsing files that an operation cannot apply to. | ||
| * The default implementation accepts every file, which is always safe. | ||
| */ | ||
| default Predicate<String> getContentPrefilteringPredicate() { |
There was a problem hiding this comment.
Following this PR, we now have three prefilters - use case level path-based prefiltering, use case level content-based prefiltering, and operation level content-based prefiltering. Think it's probably worth mentioning that in the Javadoc for the UseCase level content prefilter
| /** | ||
| * Ordinary code. None of it is matched by the benchmark patterns. | ||
| */ | ||
| private static final String[][] COMMON_SNIPPETS = { |
There was a problem hiding this comment.
What is the rationale for having these snippets in Strings, rather than example files, as with other tests?
- astra-core README: new "Skipping files that can't be changed" section, covering UseCase.getContentPrefilteringPredicate(), the new ASTOperation.getContentPrefilteringPredicate() and its contract, how the two are combined, and the predicate JavaPatternASTOperation provides - astra-maven-plugin README: note that java pattern use cases only parse the files that could match, linking to that section - CHANGELOG: the operation predicate, the java pattern prefilter, the benchmark and the performance changes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pcf5Hu86PtKtfXhaAJrJDA
…s conventions - Test methods are named testXxx(), with Javadoc, like the rest of the tests: testApiMigrationPerformance(), testJdkIdiomsPerformance(), and the prefilter tests - Temporary directories are created with Files.createTempDirectory and deleted in @after, as in TestAstraCoreParallelExecution and TestAstraCoreSharedEnvironment, rather than with a TemporaryFolder rule - The generated code is called example files, as test input is elsewhere: BenchmarkCorpus becomes PerformanceExampleGenerator, and it generates org.alfasoftware.astra.performance.PerformanceExampleNNNN rather than bench.app.ComponentNNNN - The types the example files use (LegacyCache, Money, ServiceRegistry) move from performance.lib to org.alfasoftware.astra.exampleTypes, with the other shared example types No behaviour change. The benchmark's output digests change, because the generated package and class names do. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pcf5Hu86PtKtfXhaAJrJDA
| ## Java patterns | ||
| `JavaPatternASTOperation` provides this predicate itself, so java pattern use cases get it with no extra work. A `@JavaPattern` can only match code which contains the names of the methods it invokes and the types it instantiates, so files without them, for every `@JavaPattern` in the matcher file, are skipped. Names which a pattern captures rather than matches are not required: `@Substitute` methods, the pattern's parameters, type variables, and the arguments captured by a varargs parameter. | ||
|
|
||
| This works best when those names are rare in the codebase, as they usually are when migrating away from a particular API. A pattern for something like `string.equals("")` can't skip much, because nearly every file mentions `equals`. |
There was a problem hiding this comment.
"nearly every file mentions equals"
This feels like more bold of a statement than intended. Perhaps we could reword to say that a pattern for something common like equals will skip less than something more unique.
The astra-core README is a walkthrough of writing a first refactor, so a section on content prefiltering appended to it was out of place. The documentation now lives where the related topics already are: - the wiki: "How does Astra work?" (where files are skipped in a run), "UseCases" (how to configure it, with the use case and operation predicates) and "Java Pattern Refactor" (what it skips automatically). Those pages are a separate repository, so are updated separately. - Javadoc: AstraCore's class Javadoc now describes the step, and JavaPatternASTOperation's points to its predicate. The UseCase and ASTOperation methods were already documented. The README section and the astra-maven-plugin README paragraph are removed, and the CHANGELOG links to the wiki, as earlier entries do. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pcf5Hu86PtKtfXhaAJrJDA
No description provided.