[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