RFR 8062116: JVMTI GetClassMethods is Slow

[Chuck Rasbold]

Jeremy's webrev looks good to me.

-- Chuck

On Tue, Nov 4, 2014 at 1:05 PM, Jeremy Manson <jeremymanson at google.com>
wrote:

> FWIW, all of the JDK8 jtreg tests passed.
>
> On Tue, Nov 4, 2014 at 12:43 PM, Coleen Phillimore <
> coleen.phillimore at oracle.com> wrote:
>
>>
>> On 11/04/2014 02:57 PM, 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.
>>>
>>
>> Hi Serguei,  I ran all of vm.quick.testlist on this which includes jvmti,
>> jdi tests.  I'll run jtreg jdi tests too (where are they?)
>>
>> Thanks,
>> Coleen
>>
>>
>>
>>> 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
>>>>>
>>>>
>>>>
>>>
>>
>