On Thu, 12 Jan 2023 11:09:35 GMT, Maurizio Cimadamore <mcimadam...@openjdk.org>
wrote:
>> Archie L. Cobbs has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Use the more appropriate Type comparison method Types.isSameType().
>> - Add some more comments to clarify how the analysis works.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 444:
>
>> 442: if (referenceExpressionNode) {
>> 443:
>> 444: // We treat instance methods as having a "value" equal to
>> their instance
>
> The comment is slightly misleading - e.g. I'd suggest clarifying "having a
> "value" whose type is the same as that of the class in which the method is
> defined"
Agreed - will fix.
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 454:
>
>> 452:
>> 453: // If the expression type is incompatible with 'this',
>> discard it
>> 454: if (type != null &&
>> !this.isSubtype(this.targetClass.sym.type, type))
>
> Instead of adding the direct reference, and then having to check if the
> reference needs to be removed, would it be possible not to add the reference
> in the first place if the types mismatch?
No because (for example) what if you cast?
The thing you're casting might be compatible, but after the cast it might
become incompatible.
> This will then determine how to interpret this in the context of that method
> analysis - e.g. when we see a JCIdent for this, we create a direct/indirect
> ExprRef based on what the receiver kind was. Correct?
Yes, exactly.
When we "invoke" a method, we "preload" the set of current references that the
method is going to have when it starts. That "preload" step includes any
references from (a) the method receiver (non-static methods only) and (b)
method parameters.
To the extent the method receiver has a direct/indirect reference, that turns
into a direct/indirect `ThisRef` during method execution. You can see this
happen in the very next lines after what you quoted. But of course the method
invocation (the "apply") could have been applied to any arbitrary expression as
the receiver.
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 817:
>
>> 815: // Methods - the "value" of a non-static method is a reference
>> to its instance
>> 816: final Symbol sym = tree.sym;
>> 817: if (sym.kind == MTH) {
>
> This is perhaps where filtering based on the declaring class could make sense
> (to avoid having to filter later) ? Perhaps this could also be centralized -
> e.g. whenever you create an ExprRef you also pass the type for it, and if the
> type matches that for the current class you create it and add to the list,
> otherwise you skip it.
Yes but see previous comment regarding casting.
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 875:
>
>> 873: // Reference to this?
>> 874: if (tree.name == names._this || tree.name == names._super) {
>> 875: if (this.refs.contains(ThisRef.direct()))
>
> This idiom occurs quite a lot. If I'm correct, this basically amounts at
> asking as to whether the receiver of the method we're currently evaluating is
> direct or not (which is an invariant, given a method body - e.g. for a given
> method this "fact" should stay the same). If that's the case, perhaps
> capturing this in a flag could be better - then you could have just have a
> single method e.g. `XYZRef.create(boolean direct)`, and remove the branching
> (here and elsewhere).
The code you quoted has nothing specifically to do with method invocations.
This code is simply handing the evaluation of the expressions `this` and
`super`. For example, `this` could just be a parameter we're passing to another
method.
When `this` or `super` expressions are evaluated, the thing left on top of the
stack is a direct/indirect reference (i.e., an `ExprRef`) exactly when the
there is a direct/indirect reference of type `ThisRef` in the current reference
set.
In cases where `this` is then "invoked", e.g., `this()`, the `ExprRef` (top of
Java stack) becomes the method receiver, and when the method is "invoked" it
turns back into a `ThisRef` (see earlier question).
Regarding the optimization you mention, in light of the above I'm not sure it
would still work.
-------------
PR: https://git.openjdk.org/jdk/pull/11874
