[PATCH] 8147527: Non-optimal code generated for postfix unary operators

Jan Lahoda jan.lahoda at oracle.com
Tue Oct 25 12:15:24 UTC 2016


On 25.10.2016 10:41, Maurizio Cimadamore wrote:
> Hi Jan,
> great analysis. I think (1) could be a safer path - but I admit I'm not
> entirely sold on another point; you say:
>
> "Further desugaring then replaces the "Issue8147527.super" with "this$0"
> (see "tree.selected = translate(tree.selected);" in Lower.visitSelect)"
>
> Isn't this already wrong? The static type of this$0 is Issue8147527,
> which is different from the static type of this.super (Parent).
> Shouldn't javac at least emit a cast there to preserve typing? While I
> don't dispute the other problems you discovered, which are real, I think
> the above desugaring doesn't make a lot of sense, and probably only
> succeeds because the visitor later fixes up the problem (probably by
> realizing that the owner of the symbol 'i' and the symbol of this$0
> disagree, therefore adding a cast?)

Normally (when the same tree is not reused), this is only a transient 
state inside the visitSelect method - the method keeps a flag 
(qualifiedSuperAccess) that the select expression is a the form 
"X.super", and this will then force generation of an accessor 
invocation, replacing the whole JCFieldAccess with something like 
"Issue8147527.access$101(this$0)". But in case of reused trees, this 
will only correct/replace the first use of the tree, and the subsequent 
one will remain broken.

Jan

>
> Maurizio
>
>
>
> On 24/10/16 21:11, Jan Lahoda wrote:
>> Hi,
>>
>> Yes, the current behavior indeed appears to be wrong.
>>
>> This is what I think happens:
>> Consider the "Issue8147527.super.i++;" statement from the example.
>>
>> In Lower.visitUnary, in the switch statement, in POSTINC/POSTDEC
>> section (this is the code that translates the ++/-- operators for
>> Integers to simplified AST that can be then written as bytecode):
>> http://hg.openjdk.java.net/jdk9/jdk9/langtools/file/88cc9b782624/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java#l3324
>>
>>
>> the result of "lowerBoxedPostop(tree)" is along these lines:
>>
>> (let /*synthetic*/ final Integer $997055773 =
>> (Integer)Issue8147527.super.i in (let /*synthetic*/ final Integer
>> $1063980005 = Issue8147527.super.i += 1 in $997055773))
>>
>> That seems OK. The problem is that after the invocation of translate,
>> the code looks like this:
>>
>> (let /*synthetic*/ final Integer $997055773 =
>> (Integer)Issue8147527.access$101(this$0) in (let /*synthetic*/ final
>> Integer $1063980005 = (let /*synthetic*/ final Issue8147527 $173214986
>> = this$0 in $173214986.i =
>> Integer.valueOf((int)($173214986.i.intValue() + 1))) in $997055773))
>>
>> This code is inside the Issue8147527.Inner class, and "this$0" is the
>> outer this, i.e. the enclosing instance Issue8147527. So the code is
>> actually manipulating the "i" variable from Issue8147527 not from the
>> Parent, which is apparently wrong. (In JDK-8143388 the situation was
>> different, as the Parent was in a different package, and so the
>> variable was not accessible and so the correct accessors were
>> generated after the fix.)
>>
>> I believe the main issue here is that in the first desugared code:
>>
>> (let /*synthetic*/ final Integer $997055773 =
>> (Integer)Issue8147527.super.i in (let /*synthetic*/ final Integer
>> $1063980005 = Issue8147527.super.i += 1 in $997055773))
>>
>> there is only a single tree node for "Issue8147527.super.i", reused on
>> two places. Further desugaring then replaces the "Issue8147527.super"
>> with "this$0" (see "tree.selected = translate(tree.selected);" in
>> Lower.visitSelect). This transiently leads to this code:
>>
>> (let /*synthetic*/ final Integer $997055773 = (Integer)this$0.i in
>> (let /*synthetic*/ final Integer $1063980005 = this$0.i += 1 in
>> $997055773))
>>
>> The first occurrence of "this$0.i" will get fixed by Lower.visitSelect
>> (see the call to the access method) to
>> "Issue8147527.access$101(this$0)", but the other one will not, causing
>> the problem.
>>
>> So far, it seems to me there are two possible fixes:
>> 1) stop reusing the same tree on two places (each of them will get
>> translated independently, avoiding the problem).
>> 2) avoid the modification of tree.selected in Lower.visitSelect
>>
>> Jan
>>
>> On 24.10.2016 15:50, Maurizio Cimadamore wrote:
>>> I agree that something is up here - I think the patch in 8143388
>>> affected the way in which the synthetic tree was typed and this results
>>> in javac picking up the from member.
>>>
>>> Maurizio
>>>
>>>
>>> On 24/10/16 12:34, bsrbnd wrote:
>>>> Hi,
>>>>
>>>> This issue is definitly very strange...
>>>> Examining deeply (with javap) my last patch, I came to the conclusion
>>>> that the changeset for issue 8143388 is perhaps wrong.
>>>> Consider the following example:
>>>>
>>>> class Issue8147527 extends Parent {
>>>>      public static void main(String[] args) {
>>>>          Issue8147527 self = new Issue8147527();
>>>>          self.testAll();
>>>>      }
>>>>
>>>>      private void testAll() {
>>>>          System.out.println(test());
>>>>          System.out.println(i);
>>>>
>>>>          Inner in = new Inner();
>>>>          System.out.println(in.test());
>>>>          System.out.println(i);
>>>>
>>>>          System.out.println(testParent());
>>>>          System.out.println(super.i);
>>>>
>>>>          System.out.println(in.testParent());
>>>>          System.out.println(super.i);
>>>>      }
>>>>
>>>>      Integer i=0;
>>>>      private Integer test() {
>>>>          return this.i++;
>>>>      }
>>>>
>>>>      class Inner {
>>>>          private Integer test() {
>>>>              return Issue8147527.this.i++;
>>>>          }
>>>>          private Integer testParent() {
>>>>              return Issue8147527.super.i++;
>>>>          }
>>>>      }
>>>>
>>>>      private Integer testParent() {
>>>>          return super.i++;
>>>>      }
>>>> }
>>>>
>>>> class Parent {
>>>>      protected Integer i=10;
>>>> }
>>>>
>>>> Running it gives the following output:
>>>> 0
>>>> 1
>>>> 1
>>>> 2
>>>> 10
>>>> 11
>>>> 11
>>>> 11
>>>>
>>>> If I rollback issue 8143388 changeset, I get the following output;
>>>> last line looks better (I think):
>>>> 0
>>>> 1
>>>> 1
>>>> 2
>>>> 10
>>>> 11
>>>> 11
>>>> 12
>>>>
>>>> Examining the generated code of "Inner" (I added a comment surrounded
>>>> with !!!), we had:
>>>>
>>>>    private java.lang.Integer testParent();
>>>>      Code:
>>>>         0: aload_0
>>>>         1: getfield      #3                  // Field
>>>> this$0:LIssue8147527;
>>>>         4: invokestatic  #8                  // Method
>>>> Issue8147527.access$201:(LIssue8147527;)Ljava/lang/Integer;
>>>>         7: astore_1                          // !!! REFERENCE TO VALUE
>>>> OF Field Parent.i:Ljava/lang/Integer; !!!
>>>>         8: aload_0
>>>>         9: getfield      #3                  // Field
>>>> this$0:LIssue8147527;
>>>>        12: astore_3
>>>>        13: aload_3
>>>>        14: aload_3
>>>>        15: getfield      #5                  // Field
>>>> Issue8147527.i:Ljava/lang/Integer;
>>>>        18: invokevirtual #6                  // Method
>>>> java/lang/Integer.intValue:()I
>>>>        21: iconst_1
>>>>        22: iadd
>>>>        23: invokestatic  #7                  // Method
>>>> java/lang/Integer.valueOf:(I)Ljava/lang/Integer;
>>>>        26: dup_x1
>>>>        27: putfield      #5                  // Field
>>>> Issue8147527.i:Ljava/lang/Integer;
>>>>        30: astore_2
>>>>        31: aload_1                           // !!! REFERENCE TO VALUE
>>>> OF Field Parent.i:Ljava/lang/Integer; !!!
>>>>        32: areturn
>>>>
>>>> And rollbacking issue 8143388 changeset, we have (which looks better,
>>>> I think):
>>>>
>>>>    private java.lang.Integer testParent();
>>>>      Code:
>>>>         0: aload_0
>>>>         1: getfield      #3                  // Field
>>>> this$0:LIssue8147527;
>>>>         4: astore_1
>>>>         5: aload_1
>>>>         6: getfield      #8                  // Field
>>>> Parent.i:Ljava/lang/Integer;
>>>>         9: astore_2
>>>>        10: aload_1
>>>>        11: aload_1
>>>>        12: getfield      #8                  // Field
>>>> Parent.i:Ljava/lang/Integer;
>>>>        15: invokevirtual #6                  // Method
>>>> java/lang/Integer.intValue:()I
>>>>        18: iconst_1
>>>>        19: iadd
>>>>        20: invokestatic  #7                  // Method
>>>> java/lang/Integer.valueOf:(I)Ljava/lang/Integer;
>>>>        23: dup_x1
>>>>        24: putfield      #8                  // Field
>>>> Parent.i:Ljava/lang/Integer;
>>>>        27: astore_3
>>>>        28: aload_2
>>>>        29: areturn
>>>>
>>>> Is issue 8143388 changeset really correct and harmless?
>>>>
>>>> Thanks,
>>>> Bernard
>>>>
>>>>
>>>> 2016-10-22 18:05 GMT+02:00 bsrbnd <bsrbnd at gmail.com>:
>>>>> Hi,
>>>>>
>>>>> Next is a probably better patch (derived from issue 8143388 changeset)
>>>>> that handles also "this, this$0, ..." in addition to the existing
>>>>> "super" handling. It optimizes the both cases ("this" and "super")
>>>>> described in issue 8147527.
>>>>> Notice that "thisDollar" could be part of the "Names" class if
>>>>> necessary.
>>>>>
>>>>> Bernard
>>>>>
>>>>> diff --git
>>>>> a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
>>>>> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
>>>>> ---
>>>>> a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
>>>>> +++
>>>>> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
>>>>> @@ -2218,8 +2218,10 @@
>>>>>                   return builder.build(rval);
>>>>>           }
>>>>>           Name name = TreeInfo.name(rval);
>>>>> -        if (name == names._super)
>>>>> +        Name thisDollar = names.fromString(names._this + "$");
>>>>> +        if (name != null && (name == names._super || name ==
>>>>> names._this || name.startsWith(thisDollar))) {
>>>>>               return builder.build(rval);
>>>>> +        }
>>>>>           VarSymbol var =
>>>>>               new VarSymbol(FINAL|SYNTHETIC,
>>>>>                             names.fromString(
>>>>>
>>>>>
>>>>> 2016-10-10 13:45 GMT+02:00 bsrbnd <bsrbnd at gmail.com>:
>>>>>> Hi,
>>>>>>
>>>>>> Consider the following example slightly modified from issue 8147527
>>>>>> description to incorporate an assignment operator and an inner class
>>>>>> which are both of them involved in the optimization process:
>>>>>>
>>>>>> class Issue8147527 {
>>>>>>      Integer i=0;
>>>>>>      private Integer test() {
>>>>>>          this.i += 3;
>>>>>>          for (; i<5; this.i++);
>>>>>>          return this.i++;
>>>>>>      }
>>>>>>
>>>>>>      Integer j=10;
>>>>>>      class Inner {
>>>>>>          private Integer test() {
>>>>>>              return Issue8147527.this.j++;
>>>>>>          }
>>>>>>      }
>>>>>>
>>>>>>      public static void main(String[] args) {
>>>>>>          Issue8147527 self = new Issue8147527();
>>>>>>          System.out.println(self.test());
>>>>>>          System.out.println(self.i);
>>>>>>
>>>>>>          Inner in = self.new Inner();
>>>>>>          System.out.println(in.test());
>>>>>>          System.out.println(self.j);
>>>>>>      }
>>>>>> }
>>>>>>
>>>>>> The following patch omits "this" for the special case of a
>>>>>> select-expression used as an lvalue.
>>>>>> Thus we had before optimization:
>>>>>>
>>>>>>    private java.lang.Integer test();
>>>>>>      Code:
>>>>>>         0: aload_0
>>>>>>         1: astore_1
>>>>>>         2: aload_1
>>>>>>         3: aload_1
>>>>>>         4: getfield      #3                  // Field
>>>>>> i:Ljava/lang/Integer;
>>>>>>         7: invokevirtual #5                  // Method
>>>>>> java/lang/Integer.intValue:()I
>>>>>>        10: iconst_3
>>>>>>        11: iadd
>>>>>>        12: invokestatic  #2                  // Method
>>>>>> java/lang/Integer.valueOf:(I)Ljava/lang/Integer;
>>>>>>        15: dup_x1
>>>>>>        16: putfield      #3                  // Field
>>>>>> i:Ljava/lang/Integer;
>>>>>>
>>>>>> And after optimization, we have:
>>>>>>
>>>>>>    private java.lang.Integer test();
>>>>>>      Code:
>>>>>>         0: aload_0
>>>>>>         1: aload_0
>>>>>>         2: getfield      #3                  // Field
>>>>>> i:Ljava/lang/Integer;
>>>>>>         5: invokevirtual #5                  // Method
>>>>>> java/lang/Integer.intValue:()I
>>>>>>         8: iconst_3
>>>>>>         9: iadd
>>>>>>        10: invokestatic  #2                  // Method
>>>>>> java/lang/Integer.valueOf:(I)Ljava/lang/Integer;
>>>>>>        13: putfield      #3                  // Field
>>>>>> i:Ljava/lang/Integer;
>>>>>>
>>>>>> I've run all javac tests and it seems quite good.
>>>>>> Notice that I haven't checked the "super" case yet, waiting for any
>>>>>> feedback about the first optimization.
>>>>>>
>>>>>> Regards,
>>>>>> Bernard
>>>>>>
>>>>>> diff --git
>>>>>> a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
>>>>>> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
>>>>>> ---
>>>>>> a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
>>>>>> +++
>>>>>> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
>>>>>> @@ -2253,6 +2253,7 @@
>>>>>>           case SELECT: {
>>>>>>               final JCFieldAccess s = (JCFieldAccess)lval;
>>>>>>               Symbol lid = TreeInfo.symbol(s.selected);
>>>>>> +            if (lid != null && lid.name.equals(names._this)) return
>>>>>> builder.build(make.Ident(s.sym));
>>>>>>               if (lid != null && lid.kind == TYP) return
>>>>>> builder.build(lval);
>>>>>>               return abstractRval(s.selected, new TreeBuilder() {
>>>>>>                       public JCExpression build(final JCExpression
>>>>>> selected) {
>>>
>


More information about the compiler-dev mailing list