deduplicating lambda methods
Louis Wasserman
lowasser at google.com
Thu Mar 22 18:40:12 UTC 2018
I'm with Brian: unless there's clear evidence that lambdas in the same file
that are equivalent except for these kinds of differences *actually come up
with regularity in real code,* it's not worth adding complexity to the tree
diffing logic.
I would guess that that's actively unlikely: since we're doing only
intra-file duplication right now, lambdas in a given file are more likely
than not to be written in the same style by the same developer.
On Thu, Mar 22, 2018 at 11:29 AM B. Blaser <bsrbnd at gmail.com> wrote:
> 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