hs25 review request (2nd round): 8008511 JSR 292: MemberName vmtarget refs to methods must be updated at class redefinition

[serguei.spitsyn at oracle.com]

Hi David,


Thank you for the comments!


On 4/14/13 10:12 PM, David Holmes wrote:
> Hi Serguei,
>
> Not a full review ...
>
> In instanceKlass.cpp:
>
> +   MemberNameTable* mnt = member_names();
> +   if (mnt != NULL) {
> +     delete mnt;
> +   }
>
> a) do we need to call set_member_names(null) ?

Fixed, thanks.


> b) do we need to use the MemberNameTable_lock to guard this?

I don't think so.
This deallocation must be done at a safepoint.
We can't grab locks at safepoints.

These are the details:

classLoaderData.cpp:

// Deallocate free metadata on the free list.  How useful the PermGen was!
void ClassLoaderData::free_deallocate_list() {
   // Don't need lock, at safepoint
assert(SafepointSynchronize::is_at_safepoint(), "only called at safepoint");
       . . .
       } else if (m->is_klass()) {
         MetadataFactory::free_metadata(this, (InstanceKlass*)m);
      . . .
}

metadataFactory.hpp:

   // Deallocation method for metadata
   template <class T>
   static void free_metadata(ClassLoaderData* loader_data, T md) {
     if (md != NULL) {
       . . .
       md->deallocate_contents(loader_data);
loader_data->metaspace_non_null()->deallocate((MetaWord*)md, size, 
md->is_klass());
     }
   }

instanceKlass.cpp:

void InstanceKlass::deallocate_contents(ClassLoaderData* loader_data) {
   . . .
   release_C_heap_structures();
   . . .
}

>
> ---
>
> I'm still concerned about the removal of the No_Safepoint_verifier. 
> Has whomever added it indicated that removing it is indeed okay?

Yes, John confirmed that it it is Ok to remove the No_Safepoint_verifier.
We added handles (see methodHandles.cpp) for all potentially impacted oops.


Thanks!
Serguei

>
> Thanks,
> David
>
> On 13/04/2013 12:13 PM, serguei.spitsyn at oracle.com wrote:
>>
>> Please, review the following fix.
>>
>> This is the 2nd version that has a review fixes suggested by Coleen and
>> John in the 1st review round.
>>
>> Now the mtClass is used in allocation of MemberNameTable since it's part
>> of class metadata.
>> All call sites in the methodHandles.cpp that recursively invoke the
>> add_member_name()
>> are made safepoint-safe by replacing oop's with Handle's.
>> This allowed to get rid of the No_Safepoint_Verifier in the
>> find_MemberNames.
>> The MemberName's are added to MNT not only for methods but also for 
>> fields.
>>
>> Coleen,
>> You also suggested to put all new non-jvmti code under #if INCLUDE_JVMTI
>> condition.
>> I've not done it because now the MemberName's associated with the fields
>> are
>> also registered in the MNT which is not needed for JVMTI purposes.
>> Just have some doubt if it is a right thing to do.
>> I'm Ok to add this modification, but could you, confirm that it is still
>> important.
>>
>>
>> Webrev:
>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2013/hotspot/8008511-JVMTI-JSR292.2/ 
>>
>>
>>
>> Bug:
>> 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) is to lazily create and keep up-to-date
>> a MemberNameTable
>> which plays a role of MemberName cache assosiated with the 
>> InstanceKlass.
>> Then, at 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.
>>
>> Test coverage: vm.mlvm, nsk.jvmti, nsk.jdi tests on multiple platforms
>> (32 vs 64-bit too).
>> The testing looks good.
>>
>>
>> Thanks,
>> Serguei

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20130415/8d518407/attachment-0001.html