deduplicating lambda methods

B. Blaser bsrbnd at gmail.com
Thu Mar 22 18:29:06 UTC 2018


You're right but I think all no-ops might also be treated as
equivalent ( example: for(;;) ; === for(;;) {} ) without being afraid
of added complexity. The default action in 'NoOpFinder' could simply
throw some exception instead of visiting the full tree and about 99%
of 'isNoOp()' invocations would be treated in constant time...

Bernard


On 21 March 2018 at 22:09, Maurizio Cimadamore
<maurizio.cimadamore at oracle.com> wrote:
> 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