deduplicating lambda methods

[Maurizio Cimadamore]

On 28/03/18 18:58, B. Blaser wrote:
> On 28 March 2018 at 19:44, Maurizio Cimadamore
> <maurizio.cimadamore at oracle.com> wrote:
>> Uhm - seems to me that the currently implemented logic is more subtle than
>> comparing symbols by name in the visitIdent case? E.g. it (correctly) uses
>> information about the relative position of a lambda parameter in the
>> parameter list and compares that instead of names. And visitSelect is
>> correctly doing a == on the accessed symbol, so that
>>
>> class Foo { String x; }
>>
>>
>> (Foo a) -> a.x
>>
>> (Foo b) -> b.x
>>
>> are deemed equal because:
>>
>> (i) - a and b have same positional info
>> (ii) - a.x and b.x refer to the same (==) symbol (Foo::x)
> I missed this e-mail but I just sent 5 minutes ago the full patch
> which preserve relative positions of lambda parameters, of course.
> If you try the two examples I sent previously without the patch,
> you'll see that:
>        r1 = () -> {
>            Class<?> c = Integer.class;
>        };
>
> isn't de-duplicated because occurrences of Integer.class aren't '==' and:
>
>        r1 = () -> {
>            Runnable r2 = () -> {};
>        };
>
> isn't de-duplicated because calls to lambda meta-factory aren't '=='.
To me this seems to suggest that, if we want to get better results out 
of the dedup machinery, we have to up our lowering game, so that we 
don't do excessive generation of fresh symbols. Right now we don't cache 
.class symbols and also metafactory calls with same static args and BSM 
- and that's the problem you are seeing, I believe.

Maurizio
>
> Bernard
>
>> Maurizio
>>
>>
>>
>> On 28/03/18 16:44, B. Blaser wrote:
>>> On 27 March 2018 at 20:22, B. Blaser <bsrbnd at gmail.com> wrote:
>>>> On 27 March 2018 at 19:33, Vicente Romero <vicente.romero at oracle.com>
>>>> wrote:
>>>>>
>>>>> On 03/27/2018 12:58 PM, Brian Goetz wrote:
>>>>>> It looks like there were no changes in the outcome, perhaps because
>>>>>> there
>>>>>> were no within-file duplications in the JDK.  (Which I believe.)  A
>>>>>> more
>>>>>> streams/Rx/CompletableFuture-heavy codebase would likely see an
>>>>>> improvement.
>>>>>
>>>>> right, no difference :(, let's see what happens with Liam's numbers :)
>>>> Did you include the improvement I suggested to compare local variables
>>>> *by name* instead of * by symbol* ?
>>>> If not, there's quite no chance to find duplicates excepted trivial ones
>>>> ;-)
>>> Field access symbols don't seem to be '==' neither (is that right?).
>>> So, to begin with something simple, I would compare all identifiers by
>>> name:
>>>
>>> TreeDiffer.visitIdent:
>>>       result = tree.sym.name == that.sym.name;
>>>
>>> TreeDiffer.visitSelect:
>>>       result = scan(tree.selected, that.selected) && tree.sym.name ==
>>> that.sym.name;
>>>
>>> TreeHasher.visitIdent:
>>>       hash(sym.name);
>>>
>>> TreeHasher.visitSelect:
>>>       hash(tree.sym.name);
>>>
>>> I'll try to write it a bit better, run some more tests and find a
>>> couple of examples like:
>>>
>>>       Runnable r1 = () -> {
>>>           Runnable r2 = () -> {};
>>>       };
>>>       r1 = () -> {
>>>           Runnable r2 = () -> {};
>>>       };
>>>
>>>       r1 = () -> {
>>>           Class<?> c = Integer.class;
>>>       };
>>>       r1 = () -> {
>>>           Class<?> c = Integer.class;
>>>       };
>>>
>>> What do you think?
>>>
>>> Thanks,
>>> Bernard
>>>
>>>
>>>> Bernard
>>>>
>>>>> Vicente
>>