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

[Coleen Phillimore]

On 04/01/2013 06:02 PM, serguei.spitsyn at oracle.com wrote:
> 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.

Definitely would like to hear from John and Christian.   We did discuss 
it but I wasn't totally convinced this field is more important than the 
others that were going to be moved out of Klass* to make it smaller.   
Some statistics for some sample applications like eclipse would be great.

Coleen

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20130401/00cdd42b/attachment-0001.html