RFR 8129547 (S): Excess entries in BootstrapMethods with the same (bsm, bsmKind, bsmStaticArgs), but different dynamicArgs

[Maurizio Cimadamore]

I will put everything together, run tests and submit a review - is that ok?

Maurizio

On 21/08/15 16:51, Aleksey Shipilev wrote:
> I agree. Please sponsor?
>
> Not sure what pre-commit checks are needed to be run. I have only tested
> with that regression test, plus some other related ones for indy string
> concat.
>
> -Aleksey
>
> On 08/21/2015 06:42 PM, Maurizio Cimadamore wrote:
>> Cool - many thanks!
>>
>> I'd say we can go ahead with this.
>>
>> Maurizio
>>
>> On 21/08/15 16:10, Aleksey Shipilev wrote:
>>> Hey Maurizio,
>>>
>>> I have tried to slim down TestInvokeDynamic to make a regression test.
>>> It fails on current javac, and passes on a patched one:
>>>     http://cr.openjdk.java.net/~shade/8129547/webrev.02/
>>>
>>> Thanks,
>>> -Aleksey
>>>
>>> On 08/18/2015 12:23 AM, Maurizio Cimadamore wrote:
>>>> Whoops - that's what I meant to write, sorry; I'm glad you like the
>>>> overall scheme. I will cook up a test tomorrow - we do have one test
>>>> which attaches fake dynamic symbols to AST nodes, (using an annotation
>>>> processor) then invokes code generation and checks the resulting
>>>> bytecode [1]. I think one such test might be useful here.
>>>>
>>>> [1] -
>>>> http://hg.openjdk.java.net/jdk9/jdk9/langtools/file/tip/test/tools/javac/lambda/TestInvokeDynamic.java
>>>>
>>>>
>>>>
>>>> Maurizio
>>>>
>>>> On 17/08/15 20:24, Aleksey Shipilev wrote:
>>>>> Hi Maurizio,
>>>>>
>>>>> Thanks for an idea! It almost works, but not quite: super.equals() from
>>>>> Method checks the type, so we need to guard super.*, as you did in
>>>>> hashCode(). We also need a proper subclass check for the keys alone,
>>>>> which means we might move equals() there, and make key a proper
>>>>> subclass
>>>>> to gain access to "other".
>>>>>
>>>>> In short, this version works:
>>>>>     http://cr.openjdk.java.net/~shade/8129547/webrev.01/
>>>>>
>>>>> Thanks,
>>>>> -Aleksey
>>>>>
>>>>> On 17.08.2015 17:15, Maurizio Cimadamore wrote:
>>>>>> Ok, I think I know what you meant now; DynamicMethod is used for two
>>>>>> different purposes:
>>>>>>
>>>>>> * for the NameAndType CP entry associated with an invokedynamic
>>>>>> instruction (pointed by the CONSTANT_InvokeDynamic entry); this bit
>>>>>> _requires_ dynamic info to be taken into account - as different
>>>>>> invoked
>>>>>> types need to map to different entries - even if their underlying BSM
>>>>>> entry is the same.
>>>>>>
>>>>>> * as keys in the BootstrapMethod attribute; in this case you want the
>>>>>> dynamic info to be gone, as all it's going to be stored in the
>>>>>> table is
>>>>>> a bsmKind + static args list.
>>>>>>
>>>>>> How about something like this:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~mcimadamore/8129547/
>>>>>>
>>>>>> Maurizio
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 17/08/15 14:48, Maurizio Cimadamore wrote:
>>>>>>> Hi Alex,
>>>>>>> the only dfference I see between your patch and the current code is
>>>>>>> that the current code is comparing the invokedType, while yours is
>>>>>>> not
>>>>>>> - am I reading your patch correctly?
>>>>>>>
>>>>>>> If that's the case, wouldn't dropping the call to super.equals from
>>>>>>> Pool.DynamicMethod also resolve the issue?
>>>>>>>
>>>>>>> I'm, saying this because, from a design perspective, javac symbols
>>>>>>> usually don't care about uniqueness etc. - that's an extra value
>>>>>>> added
>>>>>>> when storing them into a constant pool.
>>>>>>>
>>>>>>> Maurizio
>>>>>>>
>>>>>>> On 17/08/15 13:33, Aleksey Shipilev wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> This issue gets into way with my current work:
>>>>>>>>       https://bugs.openjdk.java.net/browse/JDK-8129547
>>>>>>>>
>>>>>>>> There is a proof-of-concept patch:
>>>>>>>>       http://cr.openjdk.java.net/~shade/8129547/webrev.00/
>>>>>>>>
>>>>>>>> The crux of an issue seems to be the Method.equals() call in
>>>>>>>> DynamicMethod.equals(), that compares the type. The type of
>>>>>>>> DynamicMethod includes the DynamicMethodSymbol.type, that includes
>>>>>>>> dynamic args. It seems DynamicMethodSymbol is a better fit for
>>>>>>>> BootstrapMethod table key, since it includes BSM symbol and static
>>>>>>>> arguments only.
>>>>>>>>
>>>>>>>> I would be grateful if somebody from compiler team can help me out
>>>>>>>> with
>>>>>>>> this. Is this the fix above valid? Can you do it better?
>>>>>>>>
>>>>>>>> How would one write a regression test for it? My cursory grep
>>>>>>>> through
>>>>>>>> langtools tests does not yield a clear way to emit a special-shaped
>>>>>>>> indy
>>>>>>>> for such a test.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> -Aleksey
>>>>>>>>
>