RFR 8062116: JVMTI GetClassMethods is Slow

[Jeremy Manson]

Updated patch here:

http://cr.openjdk.java.net/~jmanson/8062116/webrev.01/

Jeremy

On Tue, Nov 4, 2014 at 2:15 PM, serguei.spitsyn at oracle.com <
serguei.spitsyn at oracle.com> wrote:

> Jeremy and Coleen,
>
> Thank you for taking care about this bug!
>
> The fix looks good to me.
> I do not see any issues.
>
> Coleen,
>
> Please, let me know if you need any help with testing or anything else.
>
> Thanks,
> Serguei
>
>
> On 11/4/14 11:57 AM, serguei.spitsyn at oracle.com wrote:
>
>> Hi Jeremy and Coleen,
>>
>> I'm reviewing this too.
>> We also need to run the nsk.jvmti, nsk.jdi and jtreg jdi tests.
>>
>> Thanks,
>> Serguei
>>
>> On 11/3/14 12:19 PM, Coleen Phillimore wrote:
>>
>>>
>>> Hi Jeremy,
>>>
>>> I reviewed your new code and it looks fine.  I had one comment in
>>>
>>> http://cr.openjdk.java.net/~jmanson/8062116/webrev.00/src/
>>> share/vm/prims/jvmtiEnv.cpp.udiff.html
>>>
>>> The name "need_to_resolve" doesn't make sense when reading this code.
>>> Isn't it more like "need_to_ensure_space" ?  I think method resolution with
>>> the other name, which it doesn't do.
>>>
>>> I was trying to find a way to make this new code not appear twice (maybe
>>> with a local jvmtiEnv function get_jmethodID(m) - instanceK_h is
>>> m->method_holder()).
>>>
>>
>> Agreed on the above.
>>
>>
>>> Also, I was trying to figure out if the new class in utilities called
>>> chunkedList.hpp could be used to store jmethodIDs, since the data
>>> structures are similar.  There is still more things in JNIMethodBlock has
>>> to do so I think a specialized structure is still needed (which is why I
>>> originally wrote it to be very simple).  I'm not sure if the comment above
>>> it still applies. Maybe only the first and third sentences.  Can you
>>> rewrite the comment slightly?
>>>
>>> Your other comments in the changes are good.
>>>
>>> I can't completely answer your question about reusing free_methods - but
>>> if a jmethodID is created provisionally in InstanceKlass::get_jmethod_id
>>> and not needed because it loses the race in the method id cache, it's never
>>> handed back to native code, so it's safe to reuse.  This is different than
>>> jmethodIDs for methods that are unloaded.  They are cleared and never
>>> reused.  At least that's my reading of this caching code but it's pretty
>>> complicated stuff.
>>>
>>> I've also run our nsk and jck vm/jvmti on this change and they all
>>> passed.  I'd be happy to sponsor it with these suggested changes and it
>>> needs another reviewer.
>>>
>>> Thanks for diagnosing and fixing this problem!
>>> Coleen
>>>
>>>
>>> On 10/30/2014 01:02 PM, Jeremy Manson wrote:
>>>
>>>> There's a significant regression in the speed of JVMTI GetClassMethods
>>>> in
>>>> JDK8. I've tracked this down to allocation of jmethodids in a tight
>>>> loop.
>>>> The issue can be addressed by preallocating enough space for all of the
>>>> jmethodids when starting the operation and not iterating over all of the
>>>> existing jmethodids when you allocate a new one.
>>>>
>>>> A patch is here:
>>>>
>>>> http://cr.openjdk.java.net/~jmanson/8062116/webrev.00/
>>>>
>>>> A reproducible test case can be found here:
>>>>
>>>> http://cr.openjdk.java.net/~jmanson/8062116/repro/
>>>>
>>>> It's a benchmark, though: I have no idea how to turn it into a test.
>>>>
>>>> For whoever reviews it: can you explain to me why it is okay that this
>>>> code
>>>> reuses jmethodIDs (in JNIMethodBlock::add_method?  I can imagine a lot
>>>> of
>>>> problems stemming from accidental reuse.
>>>>
>>>> Jeremy
>>>>
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20141104/efdf2052/attachment.html>