RFR 8062116: JVMTI GetClassMethods is Slow
Jeremy Manson
jeremymanson at google.com
Thu Nov 6 05:44:53 UTC 2014
Wow, go take care of my toddler for a few hours, come back, and all the
questions are answered for me! Thanks, Coleen.
To be fair, the original code was actually correct (instead of, you know,
implementation-dependent-correct), so I feel a little weird about the whole
thing.
Jeremy
On Wed, Nov 5, 2014 at 9:35 PM, David Holmes <david.holmes at oracle.com>
wrote:
> On 6/11/2014 3:02 PM, Coleen Phillimore wrote:
>
>>
>> David and Serguei (and Jeremy), see below. Summary: I think Jeremy's
>> code and comments are good.
>>
>> On 11/5/14, 10:11 PM, David Holmes wrote:
>>
>>> On 6/11/2014 1:00 PM, Coleen Phillimore wrote:
>>>
>>>>
>>>> On 11/5/14, 8:11 PM, serguei.spitsyn at oracle.com wrote:
>>>>
>>>>>
>>>>> On 11/5/14 4:35 PM, Jeremy Manson wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> On Wed, Nov 5, 2014 at 3:40 PM, Coleen Phillimore
>>>>>> <coleen.phillimore at oracle.com <mailto:coleen.phillimore at oracle.com>>
>>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> On 11/5/14, 6:13 PM, Jeremy Manson wrote:
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Tue, Nov 4, 2014 at 8:56 PM, serguei.spitsyn at oracle.com
>>>>>>> <mailto:serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com
>>>>>>> <mailto: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;*
>>>>>>>
>>>>>>>
>>>>>>> Ah, you have found our crappy workaround for wild pointers to
>>>>>>> non-aligned places in the middle of _methods.
>>>>>>>
>>>>>>
>>>>>> Can you explain this? Why are there wild pointers?
>>>>>>
>>>>>>
>>>>>> My belief was that end user code could pass any old garbage to this
>>>>>> function. It's called by Method::is_method_id, which is called
>>>>>> by jniCheck::validate_jmethod_id. The idea was that this would help
>>>>>> check jni deliver useful information in the case of the end user
>>>>>> inputting garbage that happened to be in the right memory range.
>>>>>>
>>>>>> Having said that, at a second glance, it looks as if it that call is
>>>>>> protected by a call to is_method() (in checked_resolve_jmethod_id),
>>>>>> so the program will probably crash before it gets to this check.
>>>>>>
>>>>>> The other point about it was that the result of >= and < is
>>>>>> technically unspecified; if it were ever implemented as anything
>>>>>> other than a binary comparison between integers (which it never is,
>>>>>> now that no one has a segmented architecture), the comparison could
>>>>>> pass spuriously, so checking would be a good thing. Of course, the
>>>>>> comparison could fail spuriously, too.
>>>>>>
>>>>>> Anyway, I'm happy to leave it in as belt-and-suspenders (and add a
>>>>>> comment, obviously, since it has caused confusion), or take it out.
>>>>>> Your call.
>>>>>>
>>>>>
>>>>> I'm still confused.
>>>>>
>>>>> How this code could possibly check anything?
>>>>> ptrdiff_t idx = m - b->_methods;
>>>>> if (b->_methods + idx == m) {
>>>>>
>>>>> The condition above always gives true:
>>>>> b->_methods + (idx) == b->_methods + (m - b->_methods) ==
>>>>> (b->_methods- b->_methods) + m == (0 + m) == m
>>>>>
>>>>> Even if m was unaligned then at the end we compare m with m which is
>>>>> still true.
>>>>> Do I miss anything?
>>>>>
>>>>
>>>> If 'm' is unaligned we would fail this comparison:
>>>>
>>>> (gdb) print &methods->_data[2]
>>>> $34 = (Method **) 0x7fffe0022440
>>>> (gdb) print &methods->_data[0]
>>>> $35 = (Method **) 0x7fffe0022430
>>>> (gdb) print 0x7fffe0022444 - 0x7fffe0022430
>>>> $32 = 20
>>>>
>>>
>>> I was confused about this too. What we have here is pointer
>>> arithmetic, not regular arithmetic, so I'm assuming an unaligned value
>>> has to be adjusted before the actual difference is computed. So in
>>> practice:
>>>
>>> m - b->_methods
>>>
>>> is really
>>>
>>> adjusted_for_alignment(m) - b->_methods
>>>
>>
>> It's not adjusted for alignment:
>>
>
> Right - now I get it. Pointer difference is an algebraic subtraction with
> "div sizeof what is pointed to". For aligned pointers there will be no
> remainder and adding back the difference to the initial pointer will yield
> the end pointer. But if one of the pointers is not aligned that is not the
> case.
>
> All rather icky.
>
> Thanks,
> David
> ----
>
>
> #include <cstddef>
>>
>> extern "C" int printf(const char *,...);
>> class Method {
>> int i ; int j; int k;
>> };
>>
>> Method* array[10] = { new Method(),new Method(),new Method(),new
>> Method(),new Method(),n
>> ew Method(),new Method(),new Method(),new Method(),new Method() };
>>
>> void test(Method** m) {
>> printf("m is 0x%p ", m);
>> ptrdiff_t idx = m - array;
>> if (array + idx == m) {
>> printf("true %ld\n", idx);
>> } else {
>> printf("false %ld\n", idx);
>> }
>> }
>> main() {
>> Method** xx = &array[3];
>> test(xx);
>> test((Method**)(((char*)xx) - 2));
>> }
>>
>> cphilli% a.out
>> m is 0x0x601098 true 3
>> m is 0x0x601096 false 2
>>
>>
>> Coleen
>>
>>
>>> David
>>> -----
>>>
>>> (gdb) print 20/8
>>>> $33 = 2
>>>>
>>>> if m is misaligned 0x7fffe0022444 the idx would be 2 and the expression
>>>> (b->_methods + idx) would evaluate to the aligned 0xfffe0022440 so not
>>>> equal m.
>>>>
>>>> But the code could check for misaligned m instead (or it would have
>>>> already crashed). I think all bets are off if the address space is
>>>> segmented.
>>>>
>>>> The comment Jeremy added is:
>>>>
>>>> if (b->_methods <= m && m < b->_methods +
>>>> b->_number_of_methods) {
>>>> // This is a bit of extra checking, for two reasons. One is
>>>> // that contains() deals with pointers that are passed in by
>>>> // JNI code, so making sure that the pointer is aligned
>>>> // correctly is valuable. The other is that <= and > are
>>>> // technically not defined on pointers, so the if guard can
>>>> // pass spuriously; no modern compiler is likely to make that
>>>> // a problem, though (and if one did, the guard could also
>>>> // fail spuriously, which would be bad).
>>>> ptrdiff_t idx = m - b->_methods;
>>>> if (b->_methods + idx == m) {
>>>> return true;
>>>> }
>>>>
>>>> Coleen
>>>>
>>>>>
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>> **
>>>>>
>>>>>>
>>>>>> Jeremy
>>>>>>
>>>>>
>>>>>
>>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20141105/5a31b656/attachment-0001.html>
More information about the serviceability-dev
mailing list