hs25 review request: 8008511 JSR 292: MemberName vmtarget refs to methods must be updated at class redefinition

[serguei.spitsyn at oracle.com]

On 4/1/13 1:09 PM, Coleen Phillimore wrote:
>
> Hi Serguei,
>
> Sorry for the delay in reviewing this.
>
> In instanceKlass.cpp line 2730, can you make that mtClass since it's 
> part of class metadata?

Fixed.
Thank you for the catch!
>
> In methodHandles.cpp, I think the whole member name table mechanism 
> could be protected by #if INCLUDE_JVMTI since it's not needed unless 
> you have jvmti built in the vm.   And you don't need to call 
> add_method_handle() or just have it do JVMTI_RETURN or something for 
> the minimal vm.

My understanding was that John is considering to use the
MemberNameTable in the future for the compilation purposes.
However, I can temporarily place it under #if INCLUDE_JVMTI
until the compiler team decides what to do with it.

>
> Also, I think you should consider a global table rather than one per 
> class.  I think we discussed this and may have decided that speed of 
> adding these is important, but depending on how many are actually used 
> per class, there's one pointer per class.

I thought we've reached a consensus and decided to add MNT pointer
per class as it is performance sensitive for indy calls.
Anyway, before changing it to a hashtable I'd want to hear from John and 
Christian first.

> Maybe they could be organized as a hashtable instead like the method 
> handle intrinsics?
>
> The code itself looks good everywhere, except for the concern about 
> footprint.

I very appreciate you found a time to review it!
There are not many people having an expertize in this area.


Thanks,
Serguei

>
> Thanks,
> Coleen
>
> Can you make this mtClass on line
> On 03/26/2013 04:37 AM, serguei.spitsyn at oracle.com wrote:
>>
>> Please, review the following fix.
>> The fix was preliminarily reviewed by Coleen, Christian
>> and John but still a final and open jdk review is needed.
>> So that, everyone is welcome to review the fix!
>>
>> This is open webrev:
>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2013/hotspot/8008511-JVMTI-JSR292.1/
>>
>> The CR is:
>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8008511
>> https://jbs.oracle.com/bugs/browse/JDK-8008511
>>
>>
>> The problem is that the old version of the bootstrap method is re-invoked
>> after a popframe from the bootstrap method execution.
>> It is because the MemberName keeps a stale reference to the old 
>> method version.
>>
>> The solution (suggested by John Rose) is to lazily create and keep 
>> up-to-date a MemberNameTable
>> which plays a role of MemberName cache assosiated with the InstanceKlass.
>> Then, at the Class redefinition, this cache is used to replace the 
>> old method
>> references in the MemberName's with the new method references.
>>
>> The MemberNameTable is based on the GrowableArray<jweek>.
>> A C_HEAP array is allocated at the first call to 
>> InstanceKlass::add_member_name().
>> It is released in the InstanceKlass::release_C_heap_structures().
>>
>> A global week reference to member name oop is stored in the 
>> MemberNameTable.
>> It allowed to avoid having the oops_do() in the MemberNameTable.
>> Also, the MemberNameTable won't hold member name oops in memory.
>>
>> The MemberNameTable_lock mutex is added to serialize 
>> MemberNameTable's updates.
>>
>> The following No_Safepoint_Verifier has been disabled:
>> *  src/share/vm/prims/methodHandles.cpp*:
>>     799 int MethodHandles::find_MemberNames(Klass* k,
>>     . . .
>>     803 DEBUG_ONLY(No_Safepoint_Verifier nsv);
>>     It is probably Ok, but, please, let me know if it is not.
>>     The assert related to locking is fired if it is not disabled.
>>
>> Test coverage: vm.mlvm, nsk.jvmti, nsk.jdi tests on multiple 
>> platforms (32 vs 64-bit too).
>> The testing looks good so far.
>>
>>
>> Thanks,
>> Serguei
>>
>>
>