RFR 8062116: JVMTI GetClassMethods is Slow

[Coleen Phillimore]

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:

#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
>>>
>>