deduplicating lambda methods
Maurizio Cimadamore
maurizio.cimadamore at oracle.com
Wed Mar 21 21:09:03 UTC 2018
I think the way I'd like this to be addressed (not now, though) is to
enhance the tree differ to be smarter about certain edge cases:
* number of enclosing parens - e.g. (expr) === (((expr)))
* number of enclosing braces - e.g. { statement } === {{{ statement }}}
* skipping semicolons - e.g. { statement ; statement } === { statement
;;;;;; statement }
There are probably many more of this kind - so I would not like to do
something ad-hoc each time.
Maurizio
On 21/03/18 19:00, B. Blaser wrote:
> On 21 March 2018 at 13:48, Maurizio Cimadamore
> <maurizio.cimadamore at oracle.com> wrote:
>>
>> On 21/03/18 12:24, B. Blaser wrote:
>>> On 19 March 2018 at 22:31, Maurizio Cimadamore
>>> <maurizio.cimadamore at oracle.com> wrote:
>>>> [...]
>>>>
>>>> Also, a side note: we have learned during condy-folding that rewriting
>>>> trees
>>>> is not always the right approach - if you are compiling with -g, you
>>>> might
>>>> need the trees to remain in place (as you want the compiler to perform
>>>> less
>>>> aggressive folding) - so I'm not sure the compiler will always be able to
>>>> rely on constants to be rewritten in the constant folding phase.
>>>>
>>>> Maurizio
>>> This makes me think we could disable dedup with '-g' to preserve full
>>> debug-ability (as next), is this partially what you meant?
>> Yes and no, but I agree that it's better to conservatively disable
>> deduplication with -g.
>>
>> Maurizio
> I note also that we may want to skip empty blocs '{}' and statements ';'.
> The following examples would then produce only 2 lambda methods
> instead of currently 8:
>
> Runnable r = () -> {};
> r = () -> { ; };
> r = () -> { {} };
> r = () -> { { ; } };
>
> r = () -> { int i = 0; if (i==0) ; };
> r = () -> { int i = 0; if (i==0) {} };
> r = () -> { int i = 0; if (i==0) { ; } };
> r = () -> { int i = 0; if (i==0) { {} } };
>
> I had to write a 'NoOpFinder' class (does something like that already
> exist?) and I added a default action in 'TreeScanner' for that (see
> below).
>
> Does this look reasonable (I did only some quick tests)?
>
> Bernard
>
> $ diff -u src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TreeDiffer.java.r03
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TreeDiffer.java
> --- src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TreeDiffer.java.r03
> 2018-03-19 14:59:49.351288750 +0100
> +++ src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TreeDiffer.java
> 2018-03-21 19:04:46.282040040 +0100
> @@ -94,6 +94,8 @@
> import java.util.Iterator;
> import java.util.Objects;
>
> +import static com.sun.tools.javac.comp.NoOpFinder.isNoOp;
> +
> /** A visitor that compares two lambda bodies for structural equality. */
> public class TreeDiffer extends TreeScanner {
>
> @@ -109,8 +111,9 @@
> private boolean result;
>
> public boolean scan(JCTree tree, JCTree parameter) {
> - if (tree == null || parameter == null) {
> - return tree == null && parameter == null;
> + boolean nop1 = isNoOp(tree), nop2 = isNoOp(parameter);
> + if (nop1 || nop2) {
> + return nop1 && nop2;
> }
> tree = TreeInfo.skipParens(tree);
> parameter = TreeInfo.skipParens(parameter);
> @@ -172,6 +175,10 @@
> result = index == otherIndex;
> return;
> }
> + else {
> + result = symbol.name == otherSymbol.name;
> + return;
> + }
> }
> result = tree.sym == that.sym;
> }
>
> $ diff -u src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TreeHasher.java.r03
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TreeHasher.java
> --- src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TreeHasher.java.r03
> 2018-03-19 15:00:02.743120393 +0100
> +++ src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TreeHasher.java
> 2018-03-21 19:04:25.454301881 +0100
> @@ -61,7 +61,7 @@
>
> @Override
> public void scan(JCTree tree) {
> - if (tree == null) {
> + if (tree == null || NoOpFinder.isNoOp(tree)) {
> return;
> }
> tree = TreeInfo.skipParens(tree);
> @@ -84,6 +84,10 @@
> hash(idx);
> return;
> }
> + else {
> + hash(tree.sym.name);
> + return;
> + }
> }
> hash(sym);
> }
>
>
> diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/NoOpFinder.java
> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/NoOpFinder.java
> new file mode 100644
> --- /dev/null
> +++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/NoOpFinder.java
> @@ -0,0 +1,29 @@
> +package com.sun.tools.javac.comp;
> +
> +import com.sun.tools.javac.tree.JCTree;
> +import com.sun.tools.javac.tree.TreeScanner;
> +
> +public class NoOpFinder extends TreeScanner {
> +
> + public static boolean isNoOp(JCTree tree) {
> + NoOpFinder nop = new NoOpFinder();
> + if (tree != null)
> + nop.scan(tree);
> + return nop.result;
> + }
> +
> + private boolean result = true;
> +
> + private NoOpFinder() {
> + defaultAction = () -> { result = false; };
> + }
> +
> + @Override
> + public void visitSkip(JCTree.JCSkip tree) {
> + }
> +
> + @Override
> + public void visitBlock(JCTree.JCBlock tree) {
> + scan(tree.stats);
> + }
> +}
> diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeScanner.java
> b/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeScanner.java
> --- a/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeScanner.java
> +++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeScanner.java
> @@ -43,6 +43,8 @@
> */
> public class TreeScanner extends Visitor {
>
> + protected Runnable defaultAction = () -> {};
> +
> /** Visitor method: Scan a single node.
> */
> public void scan(JCTree tree) {
> @@ -63,16 +65,19 @@
> ****************************************************************************/
>
> public void visitTopLevel(JCCompilationUnit tree) {
> + defaultAction.run();
> scan(tree.defs);
> }
>
> public void visitPackageDef(JCPackageDecl tree) {
> + defaultAction.run();
> scan(tree.annotations);
> scan(tree.pid);
> }
>
> @Override
> public void visitModuleDef(JCModuleDecl tree) {
> + defaultAction.run();
> scan(tree.mods);
> scan(tree.qualId);
> scan(tree.directives);
> @@ -80,37 +85,44 @@
>
> @Override
> public void visitExports(JCExports tree) {
> + defaultAction.run();
> scan(tree.qualid);
> scan(tree.moduleNames);
> }
>
> @Override
> public void visitOpens(JCOpens tree) {
> + defaultAction.run();
> scan(tree.qualid);
> scan(tree.moduleNames);
> }
>
> @Override
> public void visitProvides(JCProvides tree) {
> + defaultAction.run();
> scan(tree.serviceName);
> scan(tree.implNames);
> }
>
> @Override
> public void visitRequires(JCRequires tree) {
> + defaultAction.run();
> scan(tree.moduleName);
> }
>
> @Override
> public void visitUses(JCUses tree) {
> + defaultAction.run();
> scan(tree.qualid);
> }
>
> public void visitImport(JCImport tree) {
> + defaultAction.run();
> scan(tree.qualid);
> }
>
> public void visitClassDef(JCClassDecl tree) {
> + defaultAction.run();
> scan(tree.mods);
> scan(tree.typarams);
> scan(tree.extending);
> @@ -119,6 +131,7 @@
> }
>
> public void visitMethodDef(JCMethodDecl tree) {
> + defaultAction.run();
> scan(tree.mods);
> scan(tree.restype);
> scan(tree.typarams);
> @@ -130,6 +143,7 @@
> }
>
> public void visitVarDef(JCVariableDecl tree) {
> + defaultAction.run();
> scan(tree.mods);
> scan(tree.vartype);
> scan(tree.nameexpr);
> @@ -137,23 +151,28 @@
> }
>
> public void visitSkip(JCSkip tree) {
> + defaultAction.run();
> }
>
> public void visitBlock(JCBlock tree) {
> + defaultAction.run();
> scan(tree.stats);
> }
>
> public void visitDoLoop(JCDoWhileLoop tree) {
> + defaultAction.run();
> scan(tree.body);
> scan(tree.cond);
> }
>
> public void visitWhileLoop(JCWhileLoop tree) {
> + defaultAction.run();
> scan(tree.cond);
> scan(tree.body);
> }
>
> public void visitForLoop(JCForLoop tree) {
> + defaultAction.run();
> scan(tree.init);
> scan(tree.cond);
> scan(tree.step);
> @@ -161,31 +180,37 @@
> }
>
> public void visitForeachLoop(JCEnhancedForLoop tree) {
> + defaultAction.run();
> scan(tree.var);
> scan(tree.expr);
> scan(tree.body);
> }
>
> public void visitLabelled(JCLabeledStatement tree) {
> + defaultAction.run();
> scan(tree.body);
> }
>
> public void visitSwitch(JCSwitch tree) {
> + defaultAction.run();
> scan(tree.selector);
> scan(tree.cases);
> }
>
> public void visitCase(JCCase tree) {
> + defaultAction.run();
> scan(tree.pat);
> scan(tree.stats);
> }
>
> public void visitSynchronized(JCSynchronized tree) {
> + defaultAction.run();
> scan(tree.lock);
> scan(tree.body);
> }
>
> public void visitTry(JCTry tree) {
> + defaultAction.run();
> scan(tree.resources);
> scan(tree.body);
> scan(tree.catchers);
> @@ -193,52 +218,63 @@
> }
>
> public void visitCatch(JCCatch tree) {
> + defaultAction.run();
> scan(tree.param);
> scan(tree.body);
> }
>
> public void visitConditional(JCConditional tree) {
> + defaultAction.run();
> scan(tree.cond);
> scan(tree.truepart);
> scan(tree.falsepart);
> }
>
> public void visitIf(JCIf tree) {
> + defaultAction.run();
> scan(tree.cond);
> scan(tree.thenpart);
> scan(tree.elsepart);
> }
>
> public void visitExec(JCExpressionStatement tree) {
> + defaultAction.run();
> scan(tree.expr);
> }
>
> public void visitBreak(JCBreak tree) {
> + defaultAction.run();
> }
>
> public void visitContinue(JCContinue tree) {
> + defaultAction.run();
> }
>
> public void visitReturn(JCReturn tree) {
> + defaultAction.run();
> scan(tree.expr);
> }
>
> public void visitThrow(JCThrow tree) {
> + defaultAction.run();
> scan(tree.expr);
> }
>
> public void visitAssert(JCAssert tree) {
> + defaultAction.run();
> scan(tree.cond);
> scan(tree.detail);
> }
>
> public void visitApply(JCMethodInvocation tree) {
> + defaultAction.run();
> scan(tree.typeargs);
> scan(tree.meth);
> scan(tree.args);
> }
>
> public void visitNewClass(JCNewClass tree) {
> + defaultAction.run();
> scan(tree.encl);
> scan(tree.typeargs);
> scan(tree.clazz);
> @@ -247,6 +283,7 @@
> }
>
> public void visitNewArray(JCNewArray tree) {
> + defaultAction.run();
> scan(tree.annotations);
> scan(tree.elemtype);
> scan(tree.dims);
> @@ -256,90 +293,110 @@
> }
>
> public void visitLambda(JCLambda tree) {
> + defaultAction.run();
> scan(tree.body);
> scan(tree.params);
> }
>
> public void visitParens(JCParens tree) {
> + defaultAction.run();
> scan(tree.expr);
> }
>
> public void visitAssign(JCAssign tree) {
> + defaultAction.run();
> scan(tree.lhs);
> scan(tree.rhs);
> }
>
> public void visitAssignop(JCAssignOp tree) {
> + defaultAction.run();
> scan(tree.lhs);
> scan(tree.rhs);
> }
>
> public void visitUnary(JCUnary tree) {
> + defaultAction.run();
> scan(tree.arg);
> }
>
> public void visitBinary(JCBinary tree) {
> + defaultAction.run();
> scan(tree.lhs);
> scan(tree.rhs);
> }
>
> public void visitTypeCast(JCTypeCast tree) {
> + defaultAction.run();
> scan(tree.clazz);
> scan(tree.expr);
> }
>
> public void visitTypeTest(JCInstanceOf tree) {
> + defaultAction.run();
> scan(tree.expr);
> scan(tree.clazz);
> }
>
> public void visitIndexed(JCArrayAccess tree) {
> + defaultAction.run();
> scan(tree.indexed);
> scan(tree.index);
> }
>
> public void visitSelect(JCFieldAccess tree) {
> + defaultAction.run();
> scan(tree.selected);
> }
>
> public void visitReference(JCMemberReference tree) {
> + defaultAction.run();
> scan(tree.expr);
> scan(tree.typeargs);
> }
>
> public void visitIdent(JCIdent tree) {
> + defaultAction.run();
> }
>
> public void visitLiteral(JCLiteral tree) {
> + defaultAction.run();
> }
>
> public void visitTypeIdent(JCPrimitiveTypeTree tree) {
> + defaultAction.run();
> }
>
> public void visitTypeArray(JCArrayTypeTree tree) {
> + defaultAction.run();
> scan(tree.elemtype);
> }
>
> public void visitTypeApply(JCTypeApply tree) {
> + defaultAction.run();
> scan(tree.clazz);
> scan(tree.arguments);
> }
>
> public void visitTypeUnion(JCTypeUnion tree) {
> + defaultAction.run();
> scan(tree.alternatives);
> }
>
> public void visitTypeIntersection(JCTypeIntersection tree) {
> + defaultAction.run();
> scan(tree.bounds);
> }
>
> public void visitTypeParameter(JCTypeParameter tree) {
> + defaultAction.run();
> scan(tree.annotations);
> scan(tree.bounds);
> }
>
> @Override
> public void visitWildcard(JCWildcard tree) {
> + defaultAction.run();
> scan(tree.kind);
> if (tree.inner != null)
> scan(tree.inner);
> @@ -347,26 +404,32 @@
>
> @Override
> public void visitTypeBoundKind(TypeBoundKind that) {
> + defaultAction.run();
> }
>
> public void visitModifiers(JCModifiers tree) {
> + defaultAction.run();
> scan(tree.annotations);
> }
>
> public void visitAnnotation(JCAnnotation tree) {
> + defaultAction.run();
> scan(tree.annotationType);
> scan(tree.args);
> }
>
> public void visitAnnotatedType(JCAnnotatedType tree) {
> + defaultAction.run();
> scan(tree.annotations);
> scan(tree.underlyingType);
> }
>
> public void visitErroneous(JCErroneous tree) {
> + defaultAction.run();
> }
>
> public void visitLetExpr(LetExpr tree) {
> + defaultAction.run();
> scan(tree.defs);
> scan(tree.expr);
> }
>
>>> Bernard
>>>
>>>
>>> $ diff -u
>>> ./src/jdk.compiler/share/classes/com/sun/tools/javac/comp/LambdaToMethod.java.r03
>>>
>>> ./src/jdk.compiler/share/classes/com/sun/tools/javac/comp/LambdaToMethod.java
>>> ---
>>> ./src/jdk.compiler/share/classes/com/sun/tools/javac/comp/LambdaToMethod.java.r03
>>> 2018-03-21 12:51:14.338021489 +0100
>>> +++
>>> ./src/jdk.compiler/share/classes/com/sun/tools/javac/comp/LambdaToMethod.java
>>> 2018-03-21 12:51:56.209495093 +0100
>>> @@ -113,6 +113,8 @@
>>> /** force serializable representation, for stress testing **/
>>> private final boolean forceSerializable;
>>>
>>> + private final boolean debug;
>>> +
>>> /** Flag for alternate metafactories indicating the lambda object
>>> is intended to be serializable */
>>> public static final int FLAG_SERIALIZABLE = 1 << 0;
>>>
>>> @@ -149,6 +151,7 @@
>>> dumpLambdaToMethodStats =
>>> options.isSet("debug.dumpLambdaToMethodStats");
>>> attr = Attr.instance(context);
>>> forceSerializable = options.isSet("forceSerializable");
>>> + debug = options.isSet(Option.G) ||
>>> options.isSet(Option.G_CUSTOM, "source");
>>> }
>>> // </editor-fold>
>>>
>>> @@ -365,7 +368,7 @@
>>> lambdaDecl.body = translate(makeLambdaBody(tree, lambdaDecl));
>>>
>>> boolean dedupe = false;
>>> - if (!localContext.isSerializable()) {
>>> + if (!localContext.isSerializable() && !debug) {
>>> DedupedLambda dedupedLambda = new
>>> DedupedLambda(lambdaDecl.sym, lambdaDecl.body);
>>> DedupedLambda existing =
>>> kInfo.dedupedLambdas.putIfAbsent(dedupedLambda, dedupedLambda);
>>> if (existing != null) {
>>
More information about the amber-dev
mailing list