[PATCH] 8147527: Non-optimal code generated for postfix unary operators
Maurizio Cimadamore
maurizio.cimadamore at oracle.com
Mon Oct 24 13:50:59 UTC 2016
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