deduplicating lambda methods

[Liam Miller-Cushon]

Thanks for the comments. I uploaded a new webrev, and responses are inline.

http://cr.openjdk.java.net/~cushon/lambdadedup/webrev.01/

On Thu, Mar 15, 2018 at 12:00 PM Brian Goetz <brian.goetz at oracle.com> wrote:

> I think we wanted to exclude serializable lambdas from being deduplicated?
>

Thanks for the reminder, done.


> Stats on its effectiveness on a representative corpus would be useful.
>

Agreed, I will collect more data and see if matches what we'd expect from
the research Louis did.


> Hopefully the hash code is discriminative enough that TreeDiffer doesn't
> actually run that often.
>

I hope so too, and will try to quantify the compile-time performance
overhead.

On Thu, Mar 15, 2018 at 1:31 PM Vicente Romero <vicente.romero at oracle.com>
wrote:

> - it seems like: map dedupedLambda, could be a set.
>

When a duplicate is detected, we want the symbol of the first equivalent
method that was seen. Currently the implementation is getting that from the
return value of putIfAbsent. I'm not sure how to get that from the Set API?


> - TreeDiffer, consider the benefits / trade-offs of extending
> com.sun.tools.javac.tree.TreeScanner instead of implementing TreeVisitor.
> The code will change a bit and you will probably need to declare a boolean
> field for the result but you will have access to the subclasses of JCTree
> without casting and will be able to use the getTag() method in JCTree, in
> most cases being JCExpression the exception, to avoid most instanceof
> checks.
>

Thanks, I'll investigate that. My initial impression is that using fields
to replace the return value and the second parameter of the visit methods
in TreeVisitor will complicate control flow somewhat.

Have you considered adding a TreeVisitor-like API that for JCTrees to javac?


> - symbols are unique so you can use == to test equality
> - you don't need: Objects.equals(tree.isStatic(), that.isStatic()), to
> compare two booleans
> - names are also unique so you can use == instead of
> Objects.equals(tree.getLabel(), that.getLabel())
>

Thanks, fixed.

- in general there are complete implementations for trees that can't appear
> inside the method's body, but I think that they were provided for
> completeness.
>

Right, my sense was that this might be useful for things besides lambda
de-duplication, so it was worth handling nodes that won't appear in method
bodies.
If you think that should be deferred until it's actually needed, I can
remove it.