[PATCH] 8147527: Non-optimal code generated for postfix unary operators
Jan Lahoda
jan.lahoda at oracle.com
Mon Oct 24 20:11:50 UTC 2016
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