On Mon, 16 Jan 2023 12:51:49 GMT, Maurizio Cimadamore <[email protected]>
wrote:
>> The number of times around any single loop is bounded by the number of new
>> references that can possibly be created during the analysis of that loop.
>>
>> That number is at most 2 * (1 + 1 + 1 + 1 + N) where N is the number of
>> parameters and/or variables declared in that scope (the 2 is for direct or
>> indirect, and the 1's are for each of the singleton reference types
>> `ThisRef`, `OuterRef`, `ExprRef`, and `ReturnRef`).
>>
>> If you have nested scopes A, B, C each with Na, Nb, and Nc variables
>> declared therein (respectively), then the bound would be something like 2 *
>> (1 + 1 + 1 + 1 + Na + (Na * Nb) + (Na * Nb * Nc)) worst case (waving hands
>> here).
>
> I have played a bit with the patch, trying to disable certain features. The
> main reason to deal with loops and recursive calls the way the patch does is
> to eliminate false positives. If we see a loop, and we can't perform the
> analysis multiple times (as the PR does), then we'd have to conclude that the
> loop can be potentially escaping (as we might have missed an local var
> update). Same goes for recursive calls (e.g. a recursive call would be
> treated as escaping). Same goes for analyzing invoked methods that fall in
> the same compilation unit. If we don't do that, more methods might look as
> "escaping".
>
> So, here's what I found:
>
> * compiling the JDK with the current patch produces 2376 warnings
> * disabling support for recursive calls produces 2427 warnings
> * treating all method calls inside a loop to be escaping produces 2464
> warnings
> * disabling scanning of methods in same compilation unit produces 4317
> warnings
>
> (Note: the patches I used to do this analysis are a bit blunt, and perhaps
> can be made a bit less conservative, which might result in less false
> positives added - I just went for the simplest possible approach, just to
> test the contribute of each analysis).
>
> This seems to suggest that even a blunt approach to deal with recursion and
> loop does not result in a significant increase of false positives (~2% more).
> That said, disabling scanning of methods in the same compilation unit results
> in a big impact in terms of false positive (~100% increase).
>
> So, I'm pretty confident that, should performance problems arise, we could
> probably dial back the analysis not to do loops (or to do them in a bounded
> way, as Vicente suggest, which is much better of what I tried here) - and
> that will probably give us the same results we have today (or a very very
> minor increase of false positives). But scanning of dependent methods in same
> compilation unit seems to be more or less a must-have.
Thanks for doing that analysis - very interesting.
It looks like you might be counting the "here via invocation" lines as separate
warnings. These are really part of the previous `possible 'this' escape` line,
e.g.:
.../java/awt/Frame.java:429: Note: possible 'this' escape before subclass is
fully initialized
init(title, null);
^
.../java/awt/Frame.java:460: Note: previous possible 'this' escape happens here
via invocation
SunToolkit.checkAndSetPolicy(this);
^
Semi-related... I was also curious what would happen if we changed the
semantics of the warning from "subclass must be in a separate compilation unit"
to "subclass must be in a separate package".
I'm not proposing we change this definition, and obviously there are trade-offs
in where you draw this boundary, but was curious anywan (at one point I thought
it might be worth having an option for this, e.g., with variants like
`-Xlint:this-escape` vs. `-Xlint:this-escape:package`, or even
`-Xlint:this-escape:module`, etc.).
In any case, here are the results:
* Warnings for subclass in separate compilation unit: 2093
* Warnings for subclass in separate package: 1334
So a 36% reduction - not too surprising since the JDK includes a bunch of
non-public implementation classes.
FWIW this is the patch I used:
diff --git
a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
index e81df22b017..f309a4742aa 100644
---
a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
+++
b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
@@ -216,24 +217,24 @@ class ThisEscapeAnalyzer extends TreeScanner {
private Lint lint = ThisEscapeAnalyzer.this.lint;
private JCClassDecl currentClass;
- private boolean privateOuter;
+ private boolean nonPublicOuter;
@Override
public void visitClassDef(JCClassDecl tree) {
JCClassDecl currentClassPrev = currentClass;
- boolean privateOuterPrev = privateOuter;
+ boolean nonPublicOuterPrev = nonPublicOuter;
Lint lintPrev = lint;
lint = lint.augment(tree.sym);
try {
currentClass = tree;
- privateOuter |= tree.sym.isAnonymous();
- privateOuter |= (tree.mods.flags & Flags.PRIVATE) != 0;
+ nonPublicOuter |= tree.sym.isAnonymous();
+ nonPublicOuter |= (tree.mods.flags & Flags.PUBLIC) == 0;
// Recurse
super.visitClassDef(tree);
} finally {
currentClass = currentClassPrev;
- privateOuter = privateOuterPrev;
+ nonPublicOuter = nonPublicOuterPrev;
lint = lintPrev;
}
}
@@ -268,10 +269,10 @@ class ThisEscapeAnalyzer extends TreeScanner {
// Determine if this is a constructor we should analyze
boolean analyzable = currentClassIsExternallyExtendable()
&&
TreeInfo.isConstructor(tree) &&
- !tree.sym.isPrivate() &&
+ (tree.sym.isPublic() || (tree.sym.flags() &
Flags.PROTECTED) != 0) &&
!suppressed.contains(tree.sym);
- // Determine if this method is "invokable" in an analysis
(can't be overridden)
+ // Determine if this method is "invokable" in an analysis
(can't be overridden outside package)
boolean invokable = !currentClassIsExternallyExtendable()
||
TreeInfo.isConstructor(tree) ||
(tree.mods.flags & (Flags.STATIC | Flags.PRIVATE |
Flags.FINAL)) != 0;
@@ -286,12 +287,13 @@ class ThisEscapeAnalyzer extends TreeScanner {
}
}
- // Determines if the current class could be extended in some
external compilation unit
+ // Determines if the current class could be extended in some other
package
private boolean currentClassIsExternallyExtendable() {
return !currentClass.sym.isFinal() &&
+ currentClass.sym.isPublic() &&
!(currentClass.sym.isSealed() &&
currentClass.permitting.isEmpty()) &&
!(currentClass.sym.owner.kind == MTH) &&
- !privateOuter;
+ !nonPublicOuter;
}
}.scan(env.tree);
-------------
PR: https://git.openjdk.org/jdk/pull/11874
