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

[serguei.spitsyn at oracle.com]

Hi David,

Thank you for your opinion and concern on this!

The removal of the NoSafepointVerifier is the biggest concern in this fix.
It was raised in the preliminary review and already discussed a little bit.
Now I hope to clear it up during the open code review.
John wanted to look at it more closely but did not have enough time yet.

Thanks,
Serguei


On 3/26/13 2:17 AM, David Holmes wrote:
> Hi Serguei,
>
> The removal of the NoSafepointVerifier could be problematic. If that 
> method is never meant to be on the stack when you block for a 
> safepoint then the method is atomic with respect to the safepoint - 
> and so can not suffer interference from actions that occur at a 
> safepoint (like class unloading (?) ). Now I presume that somewhere 
> from that method you end up taking the mutex and so can block at a 
> safepoint (or block on the lock and so still allow a safepoint to 
> occur) - which might invalidate assumptions the method has about 
> "safepoint atomicity".
>
> You need to understand why the NSV was put there in the first place.
>
> David
> ------
>
> On 26/03/2013 6:37 PM, 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
>>
>>