deduplicating lambda methods

[Vicente Romero]

On 03/17/2018 10:04 PM, Liam Miller-Cushon wrote:
> Hi Vicente,
>
> On Sat, Mar 17, 2018 at 11:44 AM Vicente Romero 
> <vicente.romero at oracle.com <mailto:vicente.romero at oracle.com>> wrote:
>
>     overall looks good, but I think that a set of regression tests
>     should be included along with the patch.
>
>
> Definitely, I have some functional tests that I'll work on turning 
> into a jtreg test.
>
> Do you have advice about how much test coverage is appropriate for 
> tree comparison?
> Getting to 100% branch coverage would require a lot of test cases. I 
> could probably
> generate some of them programatically. FWIW a certain amount of 
> TreeDiffer was
> generated and not hand-written, so any bugs are probably structural 
> problems rather
> than typos. (That doesn't lessen the need for tests, of 
> course--there's still the risk of
> adding typos when changes are made in the future.)

well I assume that covering most common cases with a combo test should 
be the general guideline here. I don't think we need 100% coverage to 
validate the approach. I would also add some corner cases, probably in 
separate tests, those corner cases should include lambdas returning 
lambdas, lambdas with inner classes in their bodies, etc.

>     at first glance it seemed to me that the hash function would
>     return the same value for:
>     (int i) -> i / 5; and (int i) -> 5 / i; I haven't test it but this
>     was my impression.
>
>
> The hash function currently traverses all nodes and hashes their tags, 
> so for those two
> examples it would see something like `[DIV, IDENT, LITERAL]` and 
> `[DIV, LITERAL, IDENT]`,
> which would not result in the same value.

yep I got that but I didn't test it manually, thanks for checking it

>
> This does mean that `x + 42` and `y + 43` both hashed to the same 
> value. I updated the
> hash function to include literal values and symbols (with the same 
> handling of method
> parameters as the diff). With that change I am no longer seeing hash 
> collisions that result in
> unnecessary diffing. There are still cases where two lambda bodies 
> hash to the same
> value but have different target types, but that case doesn't require a 
> tree diff (the check
> that the target types are the same happens first).
>
> The updates to hashing are here:
> http://cr.openjdk.java.net/~cushon/lambdadedup/webrev.03/ 
> <http://cr.openjdk.java.net/%7Ecushon/lambdadedup/webrev.03/>