RFR 8062116: JVMTI GetClassMethods is Slow
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Wed Nov 5 06:08:05 UTC 2014
Got rid of the bold selection below to make it more readable.
Thanks,
Serguei
On 11/4/14 8:56 PM, serguei.spitsyn at oracle.com wrote:
> The fix looks good in general.
>
> src/share/vm/oops/method.cpp
> 1785 bool contains(Method** m) {
> 1786 if (m == NULL) return false;
> 1787 for (JNIMethodBlockNode* b = &_head; b != NULL; b = b->_next) {
> 1788 if (b->_methods <= m && m < b->_methods + b->_number_of_methods) {
> 1789 ptrdiff_t idx = m - b->_methods;
> 1790 if (b->_methods + idx == m) {
> 1791 return true;
> 1792 }
> 1793 }
> 1794 }
> 1795 return false; // not found
> 1796 }
>
> Just noticed that the lines 1789-1792 can be replaced with one liner:
> **return true;
>
> It is because the condition (b->_methods + idx == m) is always true.
> :)
>
> Also, should we check the condition: *m != _free_method?
> What about the following ?:
> **return (*m != _free_method);
>
>
> Thanks,
> Serguei
>
>
> On 11/4/14 5:52 PM, Jeremy Manson wrote:
>> Updated patch here:
>>
>> http://cr.openjdk.java.net/~jmanson/8062116/webrev.01/
>> <http://cr.openjdk.java.net/%7Ejmanson/8062116/webrev.01/>
>>
>> Jeremy
>>
>> On Tue, Nov 4, 2014 at 2:15 PM, serguei.spitsyn at oracle.com
>> <mailto:serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com
>> <mailto: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
>> <mailto: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
>> <http://cr.openjdk.java.net/%7Ejmanson/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/ <http://cr.openjdk.java.net/%7Ejmanson/8062116/webrev.00/>
>>
>> A reproducible test case can be found here:
>>
>> http://cr.openjdk.java.net/~jmanson/8062116/repro/
>> <http://cr.openjdk.java.net/%7Ejmanson/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/2f4f2ce2/attachment.html>
More information about the serviceability-dev
mailing list