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

[Maurizio Cimadamore]

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
>>>>
>