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

Maurizio Cimadamore maurizio.cimadamore at oracle.com
Tue Oct 25 08:41:45 UTC 2016


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?)

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