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

Tue Apr 2 14:26:41 PDT 2013

On 4/2/13 2:09 PM, Coleen Phillimore wrote:
>
> On 04/02/2013 01:02 PM, serguei.spitsyn at oracle.com wrote:
>> On 4/1/13 3:51 PM, Coleen Phillimore wrote:
>>>
>>> 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.
>>
>> I'm not sure what statistics do you mean here.
>> Do you mean a footprint or performance statistics?
>>
>> As I understand from our previous discussion there can be a more 
>> general solution for the footprint issue.
>> As such we could attack the footprint problem separately.
>
> Yes, there are more general solutions to the footprint issue. One 
> would be moving these JVMTI fields and initializing them lazily and 
> there are other footprint improvements.   One additional improvement 
> would be to remove this field which will be mostly zero and put this 
> in a hashtable and not allocate it per class.
>
>> For instance, we could separate all JVMTI fields from InstanceKlass 
>> and initialize them lazily.
>> But I'd not want to go deep into this discussion.
>> You do not want me to get involved into the footprint work, do you? :)
>>
>> Also, the decision how to represent the MNT depends on its future 
>> usage by the compiler team.
>> As we agreed, the compiler team is going to adjust the MNT to their needs
>> at some point when it is more convenient for them.
>> So that, could we make a final decision when the whole picture is ready?
>> It would be better to approach it in some steps.
>> Currently, this bug blocks other work on the JVMTI support of jsr-292.
>
> I don't know what the jsr 292 team has in store for this field but 
> it's still a footprint cost that's for only a special case.  So this 
> is okay if you file a bug so that we can remove it and reimplement 
> this table to be global or a hashtable.

Ok, I'll file a bug on the footprint issue and attach it to the next 
round of review.

Thanks, Coleen!
Serguei

>
> Coleen
>
>>
>> Thanks,
>> Serguei
>>
>>>
>>> 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
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>