deduplicating lambda methods

[Maurizio Cimadamore]

Hi,
the patch looks good, and I'm ok with going ahead with this.

Taking a step back, I'd like clarify a bit what's going on here: the 
lambda deduplication effort is something that is likely to generate a 
flurry of follow up enhancements, as more edge cases are discovered. 
While it is tempting to try and fix each of edge cases with separate 
fixes, we have also to ask ourselves what is the relative priority of 
this vs. all the other things we might want to get done.

The reason why I think this specific enhancement is a good fix is, I 
believe, not because it improves the dedup results - but because it 
makes javac a more consistent place: javac always had an invariant that 
symbols are shared; as pointed out in earlier discussion, this invariant 
is violated in the case of dynamic symbols and also of synthetic .class 
field access symbols, leading to hiccups (which then manifest in 
suboptimal dedup results).

So, this kind of enhancement is what I'd call a good one - not only it 
improves (albeit marginally) the dedup story, but it also makes javac 
more consistent.

Other ad-hoc fixes I've seen discussed in this thread don't fall in this 
category of 'making javac a better place' - as such, they can only be 
justified if the improvements in terms of dedup performances is big 
enough - which means that these kind of changes would need to be backed 
up by some kind of evidence of how many additional lambdas get 
deduplicated. 100 additional LoC in a 0.1% dedup improvement is, for 
example, not what winning looks like here.

Cheers
Maurizio


On 02/04/18 20:09, B. Blaser wrote:
> On 2 April 2018 at 15:46, Vicente Romero <vicente.romero at oracle.com> wrote:
>> I assume the two patches below should be folded right?
>>
>> Vicente
> Yes, the test & the fix, as attached.
>
> Thanks,
> Bernard
>
>> On 03/31/2018 11:15 AM, B. Blaser wrote:
>>> On 30 March 2018 at 18:29, B. Blaser <bsrbnd at gmail.com> wrote:
>>>> On 29 March 2018 at 22:02, Maurizio Cimadamore
>>>> <maurizio.cimadamore at oracle.com> wrote:
>>>>> I wonder if there could be a way to reuse the logic in
>>>>> Pool::DynamicMethod -
>>>>> that code is essentially doing the same thing!
>>>>>
>>>>> Maurizio
>>>> Yes (as next), great!
>>>> All lambda tests are passing successfully but we'll have to re-run the
>>>> others which seem OK with the previous key.
>>>>
>>>> Bernard
>>> I've also updated the de-duplication test with meta-factory calls
>>> inside lambdas, as here under.
>>>
>>> Bernard
>>>
>>> diff -r 9925be430918
>>> test/langtools/tools/javac/lambda/deduplication/Deduplication.java
>>> --- a/test/langtools/tools/javac/lambda/deduplication/Deduplication.java
>>>      Wed Mar 28 14:24:17 2018 +0100
>>> +++ b/test/langtools/tools/javac/lambda/deduplication/Deduplication.java
>>>      Sat Mar 31 16:49:48 2018 +0200
>>> @@ -32,6 +32,12 @@
>>>        void group(Object... xs) {}
>>>
>>>        void test() {
>>> +
>>> +        group(
>>> +                (Runnable) () -> { ( (Runnable) () -> {} ).run(); },
>>> +                (Runnable) () -> { ( (Runnable) () -> {} ).run(); }
>>> +        );
>>> +
>>>            group((Function<String, Integer>) x -> x.hashCode());
>>>            group((Function<Object, Integer>) x -> x.hashCode());
>>>
>>>
>>>> diff -r 9925be430918
>>>>
>>>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/LambdaToMethod.java
>>>> ---
>>>> a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/LambdaToMethod.java
>>>>      Wed Mar 28 14:24:17 2018 +0100
>>>> +++
>>>> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/LambdaToMethod.java
>>>>      Fri Mar 30 18:11:49 2018 +0200
>>>> @@ -67,6 +67,7 @@
>>>>    import static com.sun.tools.javac.code.Kinds.Kind.*;
>>>>    import static com.sun.tools.javac.code.TypeTag.*;
>>>>    import static com.sun.tools.javac.tree.JCTree.Tag.*;
>>>> +import static com.sun.tools.javac.jvm.Pool.DynamicMethod;
>>>>
>>>>    import javax.lang.model.element.ElementKind;
>>>>    import javax.lang.model.type.TypeKind;
>>>> @@ -226,6 +227,8 @@
>>>>
>>>>            private Map<DedupedLambda, DedupedLambda> dedupedLambdas;
>>>>
>>>> +        private Map<DynamicMethod, DynamicMethodSymbol> dynMethSyms =
>>>> new HashMap<>();
>>>> +
>>>>            /**
>>>>             * list of deserialization cases
>>>>             */
>>>> @@ -1218,9 +1221,10 @@
>>>>                                                (MethodSymbol)bsm,
>>>>                                                indyType,
>>>>                                                staticArgs.toArray());
>>>> -
>>>>                JCFieldAccess qualifier =
>>>> make.Select(make.QualIdent(site.tsym), bsmName);
>>>> -            qualifier.sym = dynSym;
>>>> +            DynamicMethodSymbol existing =
>>>> kInfo.dynMethSyms.putIfAbsent(
>>>> +                    new DynamicMethod(dynSym, types), dynSym);
>>>> +            qualifier.sym = existing != null ? existing : dynSym;
>>>>                qualifier.type = indyType.getReturnType();
>>>>
>>>>                JCMethodInvocation proxyCall = make.Apply(List.nil(),
>>>> qualifier, indyArgs);
>>>> diff -r 9925be430918
>>>> src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/Pool.java
>>>> --- a/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/Pool.java
>>>>      Wed Mar 28 14:24:17 2018 +0100
>>>> +++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/Pool.java
>>>>      Fri Mar 30 18:11:49 2018 +0200
>>>> @@ -180,10 +180,10 @@
>>>>            }
>>>>        }
>>>>
>>>> -    static class DynamicMethod extends Method {
>>>> +    public static class DynamicMethod extends Method {
>>>>            public Object[] uniqueStaticArgs;
>>>>
>>>> -        DynamicMethod(DynamicMethodSymbol m, Types types) {
>>>> +        public DynamicMethod(DynamicMethodSymbol m, Types types) {
>>>>                super(m, types);
>>>>                uniqueStaticArgs = getUniqueTypeArray(m.staticArgs, types);
>>>>            }
>>