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

[Aleksey Shipilev]

Yes, please!

Thanks,
-Aleksey

On 08/21/2015 07:29 PM, Maurizio Cimadamore wrote:
> 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
>>>>>>>>>
>>
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20150821/91efbf01/signature.asc>