[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