On Fri, 13 Jan 2023 21:28:51 GMT, Archie L. Cobbs <d...@openjdk.org> wrote:
>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> line 1088:
>>
>>> 1086: private <T extends JCTree> void visitLooped(T tree, Consumer<T>
>>> visitor) {
>>> 1087: visitScoped(tree, false, t -> {
>>> 1088: while (true) {
>>
>> I have also been thinking if the loop analysis could go wild and execute a
>> large, unbound number of times. But it seems from Archie's experiments that
>> this probably won't occur in "normal" code and worst case scenario if that
>> were to occur we can always limit the number of times we will process loops
>> to a set number of times
>
> 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.
-------------
PR: https://git.openjdk.org/jdk/pull/11874
