JDK 8147527 bis: Lower.makeComma() optimization and refactoring
bsrbnd
bsrbnd at gmail.com
Sat Nov 19 15:28:40 UTC 2016
Hi,
I would like to make one more comment aside this issue but related to it.
Lower.makeComma() creates a temporary variable and discard it
immediatly without using it.
This should be avoided, too. The "let expression" for the simple case
of "this.i++" should use only one temporary variable:
class Issue8147527 {
Integer i=0;
private Integer test() {
return this.i++;
}
}
The patch, here below, is a suggestion of refactoring for this
optimization (needs to be well tested).
Thus, before this we had the following desugared and generated code:
private Integer test() {
return (let /*synthetic*/ final Integer $5222779 =
(Integer)this.i in (let /*synthetic*/ final Integer $18695331 = this.i
= Integer.valueOf((int)(this.i.intValue() + 1)) in $5222779));
}
private java.lang.Integer test();
Code:
0: aload_0
1: getfield #3 // Field i:Ljava/lang/Integer;
4: astore_1
5: aload_0
6: aload_0
7: getfield #3 // Field i:Ljava/lang/Integer;
10: invokevirtual #4 // Method
java/lang/Integer.intValue:()I
13: iconst_1
14: iadd
15: invokestatic #2 // Method
java/lang/Integer.valueOf:(I)Ljava/lang/Integer;
18: dup_x1
19: putfield #3 // Field i:Ljava/lang/Integer;
22: astore_2
23: aload_1
24: areturn
}
And after, we would have:
private Integer test() {
return (let /*synthetic*/ final Integer $5222779 =
(Integer)this.i in (eval this.i =
Integer.valueOf((int)(this.i.intValue() + 1)); in $5222779));
}
private java.lang.Integer test();
Code:
0: aload_0
1: getfield #3 // Field i:Ljava/lang/Integer;
4: astore_1
5: aload_0
6: aload_0
7: getfield #3 // Field i:Ljava/lang/Integer;
10: invokevirtual #4 // Method
java/lang/Integer.intValue:()I
13: iconst_1
14: iadd
15: invokestatic #2 // Method
java/lang/Integer.valueOf:(I)Ljava/lang/Integer;
18: putfield #3 // Field i:Ljava/lang/Integer;
21: aload_1
22: areturn
}
Which is much better, I think; we save space in the local variable
array and reduce the number of instructions...
What's your opinion on that, should something like this be included in
a near future?
Thanks,
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
@@ -2208,6 +2208,10 @@
* in the let expression.
*/
JCExpression abstractRval(JCExpression rval, Type type,
TreeBuilder builder) {
+ return abstractRval(rval, type, false, builder);
+ }
+
+ JCExpression abstractRval(JCExpression rval, Type type, boolean
discard, TreeBuilder builder) {
rval = TreeInfo.skipParens(rval);
switch (rval.getTag()) {
case LITERAL:
@@ -2218,8 +2222,18 @@
return builder.build(rval);
}
Name name = TreeInfo.name(rval);
- if (name == names._super)
+ if (name == names._super || name == names._this)
return builder.build(rval);
+
+ if (discard) {
+ rval = convert(rval,type);
+ JCExpressionStatement eval = make.Exec(rval);
+ JCExpression built = builder.build(null);
+ JCExpression res = make.LetComma(eval, built);
+ res.type = built.type;
+ return res;
+ }
+
VarSymbol var =
new VarSymbol(FINAL|SYNTHETIC,
names.fromString(
@@ -2283,11 +2297,7 @@
// evaluate and discard the first expression, then evaluate the second.
JCExpression makeComma(final JCExpression expr1, final
JCExpression expr2) {
- return abstractRval(expr1, new TreeBuilder() {
- public JCExpression build(final JCExpression discarded) {
- return expr2;
- }
- });
+ return abstractRval(expr1, expr1.type, true, (final
JCExpression discarded) -> expr2);
}
/**************************************************************************
@@ -3205,7 +3215,7 @@
newTag,
tree.type,
tree.rhs.type);
- JCExpression expr = lhs;
+ JCExpression expr = (JCExpression)lhs.clone();
if (expr.type != tree.type)
expr = make.TypeCast(tree.type, expr);
JCBinary opResult = make.Binary(newTag, expr,
tree.rhs);
@@ -3288,9 +3298,10 @@
public JCExpression build(final
JCExpression tmp2) {
JCTree.Tag opcode = (tree.hasTag(POSTINC))
? PLUS_ASG : MINUS_ASG;
- JCTree lhs = cast
- ? make.TypeCast(tree.arg.type, tmp1)
- : tmp1;
+ JCExpression lhs = (JCExpression)tmp1.clone();
+ lhs = cast
+ ? make.TypeCast(tree.arg.type, lhs)
+ : lhs;
JCExpression update = makeAssignop(opcode,
lhs,
make.Literal(1));
@@ -3857,6 +3868,10 @@
public void visitLetExpr(LetExpr tree) {
tree.defs = translateVarDefs(tree.defs);
+ if (tree instanceof LetComma) {
+ LetComma comma = (LetComma)tree;
+ comma.eval = translate(comma.eval);
+ }
tree.expr = translate(tree.expr, tree.type);
result = tree;
}
diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/Gen.java
b/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/Gen.java
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/Gen.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/Gen.java
@@ -1574,9 +1574,9 @@
((JCUnary) e).setTag(PREDEC);
break;
}
- Assert.check(code.state.stacksize == 0);
+ int stacksize = code.state.stacksize;
genExpr(tree.expr, tree.expr.type).drop();
- Assert.check(code.state.stacksize == 0);
+ Assert.check(code.state.stacksize == stacksize);
}
public void visitBreak(JCBreak tree) {
@@ -2129,6 +2129,10 @@
letExprDepth++;
int limit = code.nextreg;
genStats(tree.defs, env);
+ if (tree instanceof LetComma) {
+ LetComma comma = (LetComma)tree;
+ genStat(comma.eval, env);
+ }
result = genExpr(tree.expr, tree.expr.type).load();
code.endScopes(limit);
letExprDepth--;
diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/JCTree.java
b/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/JCTree.java
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/JCTree.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/JCTree.java
@@ -2860,6 +2860,14 @@
}
}
+ public static class LetComma extends LetExpr {
+ public JCExpressionStatement eval;
+ protected LetComma(JCExpressionStatement eval, JCExpression expr) {
+ super(List.nil(), expr);
+ this.eval = eval;
+ }
+ }
+
/** An interface for tree factories
*/
public interface Factory {
diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/Pretty.java
b/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/Pretty.java
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/Pretty.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/Pretty.java
@@ -1433,7 +1433,12 @@
public void visitLetExpr(LetExpr tree) {
try {
- print("(let " + tree.defs + " in " + tree.expr + ")");
+ if (tree instanceof LetComma) {
+ LetComma comma = (LetComma)tree;
+ print("(eval " + comma.eval + " in " + comma.expr + ")");
+ }
+ else
+ print("(let " + tree.defs + " in " + tree.expr + ")");
} catch (IOException e) {
throw new UncheckedIOException(e);
}
diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeMaker.java
b/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeMaker.java
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeMaker.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeMaker.java
@@ -594,6 +594,12 @@
return tree;
}
+ public LetComma LetComma(JCExpressionStatement eval, JCExpression expr) {
+ LetComma tree = new LetComma(eval, expr);
+ tree.pos = pos;
+ return tree;
+ }
+
/* ***************************************************************************
* Derived building blocks.
****************************************************************************/
More information about the compiler-dev
mailing list