Skip to content

8369039: JDK-8348611 caused regression in Javac-Hot-Generate #27651

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.

Sign up for GitHub

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

Closed
wants to merge 6 commits into from
Closed

8369039: JDK-8348611 caused regression in Javac-Hot-Generate #27651

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f992660
Avoid unnecessary recursion in LintMapper.
archiecobbs Oct 4, 2025
d098d01
Avoid using LintMapper when knowing rootLint suffices.
archiecobbs Oct 5, 2025
02d2e73
Remove drive-by tweaks that aren\t the real issue.
archiecobbs Oct 6, 2025
fdc907a
Put back the drive-by tweaks; they seem to matter.
archiecobbs Oct 7, 2025
816162e
Unstream loops.
archiecobbs Oct 7, 2025
c7d5d30
Do more desugaring of Stream and Optional.
archiecobbs Oct 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 28 additions & 22 deletions src/jdk.compiler/share/classes/com/sun/tools/javac/code/LintMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,18 +25,14 @@

package com.sun.tools.javac.code;

import java.util.ArrayList;
import java.util.Comparator;
import java.util.HashMap;
import java.util.Iterator;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.function.Consumer;
import java.util.stream.Stream;

import javax.tools.DiagnosticListener;
import javax.tools.JavaFileObject;

import com.sun.tools.javac.tree.EndPosTable;
Expand Down Expand Up @@ -126,9 +122,10 @@ public boolean isKnown(JavaFileObject sourceFile) {
*/
public Optional<Lint> lintAt(JavaFileObject sourceFile, DiagnosticPosition pos) {
initializeIfNeeded();
return Optional.of(sourceFile)
.map(fileInfoMap::get)
.flatMap(fileInfo -> fileInfo.lintAt(pos));
FileInfo fileInfo = fileInfoMap.get(sourceFile);
if (fileInfo != null)
return fileInfo.lintAt(pos);
return Optional.empty();
}

/**
Expand Down Expand Up @@ -180,15 +177,15 @@ public void finishParsingFile(JCCompilationUnit tree) {
private static class FileInfo {

final LintRange rootRange; // the root LintRange (covering the entire source file)
final List<Span> unmappedDecls = new ArrayList<>(); // unmapped top-level declarations awaiting attribution
final List<Span> unmappedDecls = new LinkedList<>(); // unmapped top-level declarations awaiting attribution
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LinkedList has a ton of nodes, which translate to extra object headers, forward/backward pointers, and worse cache locality. To add N items, it requires O(N) allocations. In contrast, ArrayList requires O(log(N)) allocations (resizing) and is almost always better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@archiecobbs measured a win. I haven't seen it in profiles but it could be that the code in afterAttr contribute to the case for LinkedList as it is set up to remove matching item from the list, likely at an early index. Which is one of the few things a LinkedList actually does better than an ArrayList.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I agree ArrayLists are often better, @archiecobbs measured the LinkedLists perform better here, as @cl4es says.

I am not quite clear why linked lists are faster, but it might be many of the lists are either empty (all the children of the leaf LintRange instances will be empty lists, I think, and an empty LinkedList is, I think, cheaper than an empty ArrayList), and some of them will have only a few entries (like the unmappedDecls list here: AFAIK, this has one entry for each top-level declaration, and hence is highly unlikely to have more than 2 entries - one for the package clause, and one for the top-level class). If ArrayLists with substantial number of entries are only a small minority, it might explain why the use of LinkedLists leads to better results.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the right way to fix is not to use LinkedList, but to update afterAttr to use List.removeIf - this was added back in 8 to avoid the overhead of shifting from multiple removals.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second look my last analysis was wrong. This is removing one element which is probably better done as an operation on a TreeSet, which is okay. However, I don't find why we would use a LinkedList for LintRange.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, this has one entry for each top-level declaration, and hence is highly unlikely to have more than 2 entries - one for the package clause, and one for the top-level class

Actually it will most likely only have one entry - package declarations are not included because they can never contain @SuppressWarnings annotations. This may explain why LinkedList is faster on average.

I don't find why we would use a LinkedList for LintRange.

The same answer may apply here, but honestly I'm just guessing at this. I was also surprised that LinkedList was faster than ArrayList, but the numbers seem to be saying that.

In fact, as you point out, unmappedDecls could be a Set instead of a List. But whether that would actually help is not clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it will most likely only have one entry - package declarations are not included because they can never contain @SuppressWarnings annotations. This may explain why LinkedList is faster on average.

Argh, ignore that, I was looking at something else. So "2" is the right number on average.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh, ignore that, I was looking at something else. So "2" is the right number on average.

But there's no reason for that! 99.99% of package declarations have no annotations and therefore do not need to wait for attribution. So we can get this number down to "1".

I was curious if that would make any difference (using this additional patch). When I try it on my laptop I get a slight improvement:

Benchmark                        (stopStage)  Mode  Cnt   Score   Error  Units
SingleJavacBenchmark.compileHot     Generate    ss   10  14.841 ± 0.618   s/op

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was curious if that would make any difference (using this additional patch). When I try it on my laptop I get a slight improvement:

Can't reallt establish any statistically significant difference either way here, neither on score or allocations.

So just to understand this a bit better I instrumented to see how common that condition is and it seems that when tree.getTag() == Tree.PACKAGEDEF then tree.annotations is never empty. At least in this benchmark. Perhaps the case when there are no annotations are skipped earlier?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So just to understand this a bit better I instrumented to see how common that condition is and it seems that when tree.getTag() == Tree.PACKAGEDEF then tree.annotations is never empty. At least in this benchmark. Perhaps the case when there are no annotations are skipped earlier?

Really? I'm not seeing that. When I apply this patch, I see size=0 unless there's actually an annotation there:

--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/code/LintMapper.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/code/LintMapper.java
@@ -179,6 +179,7 @@ private static class FileInfo {
         FileInfo(Lint rootLint, JCCompilationUnit tree) {
             rootRange = new LintRange(rootLint);
             for (JCTree decl : tree.defs) {
+if (decl instanceof JCPackageDecl p) System.out.println("package "+p.pid+": annotations.size="+p.annotations.size());
                 if (isTopLevelDecl(decl))
                     unmappedDecls.add(new Span(decl, tree.endPositions));
             }
         }


// After parsing: Add top-level declarations to our "unmappedDecls" list
FileInfo(Lint rootLint, JCCompilationUnit tree) {
rootRange = new LintRange(rootLint);
tree.defs.stream()
.filter(this::isTopLevelDecl)
.map(decl -> new Span(decl, tree.endPositions))
.forEach(unmappedDecls::add);
for (JCTree decl : tree.defs) {
if (isTopLevelDecl(decl))
unmappedDecls.add(new Span(decl, tree.endPositions));
}
}

// After attribution: Discard the span from "unmappedDecls" and populate the declaration's subtree under "rootRange"
Expand All @@ -205,8 +202,11 @@ void afterAttr(JCTree tree, EndPosTable endPositions) {

// Find the most specific Lint configuration applying to the given position, unless the position has not been mapped yet
Optional<Lint> lintAt(DiagnosticPosition pos) {
boolean mapped = unmappedDecls.stream().noneMatch(span -> span.contains(pos));
return mapped ? Optional.of(rootRange.bestMatch(pos).lint) : Optional.empty();
for (Span span : unmappedDecls) {
if (span.contains(pos))
return Optional.empty();
}
return Optional.of(rootRange.bestMatch(pos).lint);
}

boolean isTopLevelDecl(JCTree tree) {
Expand Down Expand Up @@ -252,21 +252,27 @@ private record LintRange(

// Create a node representing the entire file, using the root lint configuration
LintRange(Lint rootLint) {
this(Span.MAXIMAL, rootLint, new ArrayList<>());
this(Span.MAXIMAL, rootLint, new LinkedList<>());
}

// Create a node representing the given declaration and its corresponding Lint configuration
LintRange(JCTree tree, EndPosTable endPositions, Lint lint) {
this(new Span(tree, endPositions), lint, new ArrayList<>());
this(new Span(tree, endPositions), lint, new LinkedList<>());
}

// Find the most specific node in this tree (including me) that contains the given position, if any
LintRange bestMatch(DiagnosticPosition pos) {
return children.stream()
.map(child -> child.bestMatch(pos))
.filter(Objects::nonNull)
.reduce((a, b) -> a.span.contains(b.span) ? b : a)
.orElseGet(() -> span.contains(pos) ? this : null);
LintRange bestMatch = null;
for (LintRange child : children) {
if (!child.span.contains(pos)) // don't recurse unless necessary
continue;
LintRange childBestMatch = child.bestMatch(pos);
if (childBestMatch != null && (bestMatch == null || bestMatch.span.contains(childBestMatch.span)))
bestMatch = childBestMatch;
}
if (bestMatch == null)
bestMatch = span.contains(pos) ? this : null;
return bestMatch;
}

// Populate a sparse subtree corresponding to the given nested declaration.
Expand Down
10 changes: 10 additions & 0 deletions src/jdk.compiler/share/classes/com/sun/tools/javac/util/Log.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,16 @@ public final void report(JCDiagnostic diag) {
if (category != null) { // this is a lint warning; find the applicable Lint
DiagnosticPosition pos = diag.getDiagnosticPosition();
if (pos != null && category.annotationSuppression) { // we should apply the Lint from the warning's position

// Optimization: We don't need to go through the trouble of calculating the Lint instance at "pos" if
// (a) "category" is disabled at the root level, and (b) the diagnostic doesn't have the DEFAULT_ENABLED
// flag: @SuppressWarnings can only disable lint categories, so "category" is disabled in the entire file.
if (!rootLint().isEnabled(category) &&
!diag.isFlagSet(DEFAULT_ENABLED) &&
!diag.getCode().equals(RequiresTransitiveAutomatic.key())) // accommodate the "requires" hack below
return;

// Wait for the Lint instance at "pos" to be calculated, then proceed
if ((lint = lintFor(diag)) == null) {
addLintWaiter(currentSourceFile(), diag); // ...but we don't know it yet, so defer
return;
Expand Down