RFR (L) 8061205: MetadataOnStackMark only needs to walk code cache during class redefinition
Stefan Karlsson
stefan.karlsson at oracle.com
Fri Mar 13 13:52:18 UTC 2015
On 2015-03-12 19:16, Coleen Phillimore wrote:
>
> Hi,
>
> On 3/12/15, 9:09 AM, Stefan Karlsson wrote:
>> Hi Coleen,
>>
>> On 2015-03-11 21:48, Coleen Phillimore wrote:
>>>
>>> Stefan,
>>>
>>> I made the changes you suggested.
>>
>> Thanks.
>>
>>> I also moved purge_previous_versions conditionally back to class
>>> unloading, where G1 doesn't do this until full GC.
>>
>> Thanks.
>>
>>> Do you prefer this to calling the separate function for
>>> ClassLoaderDataGraph::clean_redefinition_metadata()? I thought with
>>> a separate function we could be more judicious where to call it, but
>>> we can also add conditions to ClassLoaderDataGraph::do_unloading()
>>> to avoid metadata walking.
>>>
>>> http://javaweb.us.oracle.com/~cphillim/webrev/8061205.03/
>>
>> http://javaweb.us.oracle.com/~cphillim/webrev/8061205.03/src/share/vm/classfile/classLoaderData.cpp.udiff.html
>>
>> With this code:
>> while (data != NULL) {
>> if (data->is_alive(is_alive_closure)) {
>> *+ // clean metaspace*
>> *+ if (walk_all_metadata) {*
>> *+ data->classes_do(InstanceKlass::purge_previous_versions);*
>> *+ }*
>> *+ data->free_deallocate_list();*
>>
>> are you reintroducing the bug that Roland fixed with the follwing
>> change::
>>
>> http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/rev/c3388a74a6fb
>
> No, I moved clean_weak_method_links to the end of redefinition and
> clean MethodData to not have any old methods (not just ones on
> stack). This cleaning used to be inside of purge_previous_versions,
> which is why we needed the loop to deallocate after all the classes
> had called purge_previous_versions.
>
> I had to take out the assert that used to be clean_weak_method_links
> because it was confusing me. I don't think it was verifying anything
> since all the methods aren't marked is_old until all the classes are
> redefined, and they're cleaned out then.
>
> Thanks - I'm glad you remembered that bug.
Thanks for explaining. The GC parts looks good. I'll let others Review
the actual class redefinition parts.
Thanks,
StefanK
>
> Coleen
>
>>
>> + if (has_redefined_a_class) {
>> + // purge_previous_versions also cleans weak method links. Because
>> + // one method's MDO can reference another method from another
>> + // class loader, we need to first clean weak method links for all
>> + // class loaders here. Below, we can then free redefined methods
>> + // for all class loaders.
>> + while (data != NULL) {
>> + if (data->is_alive(is_alive_closure)) {
>> + data->classes_do(InstanceKlass::purge_previous_versions);
>> + }
>> + data = data->next();
>> + }
>> + }
>> + data = _head;
>> while (data != NULL) {
>> if (data->is_alive(is_alive_closure)) {
>> - if (has_redefined_a_class) {
>> - data->classes_do(InstanceKlass::purge_previous_versions);
>> - }
>>
>> Thanks,
>> StefanK
>>
>>>
>>> I've rerun all the class redefinition tests with -XX:+UseG1GC and
>>> with the default collector.
>>>
>>> Thanks,
>>> Coleen
>>>
>>> On 3/10/15, 3:50 PM, Coleen Phillimore wrote:
>>>>
>>>> Stefan, Thank you for reviewing this so quickly!
>>>>
>>>> On 3/10/15, 12:24 PM, Stefan Karlsson wrote:
>>>>> Hi Coleen,
>>>>>
>>>>> Thanks for fixing this and lowering the G1 remark times when class
>>>>> redefinition is used.
>>>>>
>>>>> I'm only reviewing the GC specific parts:
>>>>>
>>>>> On 2015-03-09 21:57, Coleen Phillimore wrote:
>>>>>> Summary: Only do full metadata walk during class redefinition and
>>>>>> only walk handles during class unloading.
>>>>>>
>>>>>> This change decouples metadata walking for redefinition and class
>>>>>> unloading, so that class unloading for G1 doesn't walk the code
>>>>>> cache. It also decouples GC and on_stack marking in the code cache.
>>>>>>
>>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8061205/
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~coleenp/8061205/src/share/vm/classfile/metadataOnStackMark.cpp.frames.html
>>>>>
>>>>>
>>>>> There are Atomic calls left to handle concurrent retiring of
>>>>> buffers. Do you want to keep it?:
>>>>
>>>> I didn't know whether I should or not. I suppose we should trust
>>>> our source code control system in case we ever need to do this
>>>> concurrently again. I'll remove the concurrency - it makes the
>>>> code a bit simpler. The code to do chunked lists is still good
>>>> though, so I'm glad you added that as a utility class.
>>>>>
>>>>> 97 void
>>>>> MetadataOnStackMark::retire_buffer(MetadataOnStackBuffer* buffer) {
>>>>> 98 if (buffer == NULL) {
>>>>> 99 return;
>>>>> 100 }
>>>>> 101
>>>>> 102 MetadataOnStackBuffer* old_head;
>>>>> 103
>>>>> 104 do {
>>>>> 105 old_head =
>>>>> const_cast<MetadataOnStackBuffer*>(_used_buffers);
>>>>> 106 buffer->set_next_used(old_head);
>>>>> 107 } while (Atomic::cmpxchg_ptr(buffer, &_used_buffers,
>>>>> old_head) != old_head);
>>>>> 108 }
>>>>>
>>>>> There's also some Atomic code in accessFlags that were added to
>>>>> support concurrent mark_on_stack calls. Maybe you want to get rid
>>>>> of that code as well?
>>>>
>>>> I like how you changed it so that the cpool and method is only
>>>> recorded if it's not already marked. That probably saves a lot of
>>>> time and space.
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~coleenp/8061205/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp.cdiff.html
>>>>>
>>>>>
>>>>> Could you get rid of the pre/post_work_verification functions?
>>>>> They were only added to be able to verify the MetadataOnStackMark
>>>>> state, and is not needed anymore.
>>>>>
>>>>> void pre_work_verification() {
>>>>> -
>>>>> assert(!MetadataOnStackMark::has_buffer_for_thread(Thread::current()),
>>>>> "Should be empty");
>>>>> }
>>>>> void post_work_verification() {
>>>>> -
>>>>> assert(!MetadataOnStackMark::has_buffer_for_thread(Thread::current()),
>>>>> "Should be empty");
>>>>> }
>>>>>
>>>>>
>>>>
>>>> Okay, thanks. I didn't know if you'd prefer that.
>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~coleenp/8061205/src/share/vm/runtime/thread.cpp.frames.html
>>>>>
>>>>>
>>>>> 4105 void Threads::metadata_handles_do(void f(Metadata*)) {
>>>>> 4106 // Only walk the Handles in Thread.
>>>>> 4107 ALL_JAVA_THREADS(p) {
>>>>> 4108 p->metadata_handles_do(f);
>>>>> 4109 }
>>>>> 4110 }
>>>>>
>>>>> This only walks metadata handles in the Java threads. Don't we
>>>>> have metadata handles in the VM Thread?
>>>>>
>>>>>
>>>>
>>>> Hm. We never walked non-Java threads before but yes, we could
>>>> have Metadata handles on these threads. Wow, thanks for finding
>>>> this bug! I think this should walk GC threads too (no?)
>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~coleenp/8061205/src/share/vm/gc_implementation/shared/vmGCOperations.cpp.frames.html
>>>>>
>>>>>
>>>>> Could you motivate why only Metadata induced Full GCs need to do
>>>>> this unloading?
>>>>>
>>>>> 268 // If redefinition, make a pass over the metadata to find
>>>>> any that
>>>>> 269 // can be marked to be deallocated
>>>>> 270 if (JvmtiExport::has_redefined_a_class()) {
>>>>> 271 ClassLoaderDataGraph::clean_redefinition_metadata();
>>>>> 272 }
>>>>>
>>>>> Don't we need it for our other Full GCs?
>>>>
>>>> No, I don't think we want it for all full GCs. The effect of
>>>> cleaning out the redefinition metadata is to remove the metadata
>>>> itself. There may be some mirrors unused from the redefined
>>>> classes. I suppose you could construct a test case where one of
>>>> the mirrors is gigantic and causes an out of memory situation in
>>>> Java heap.
>>>>
>>>> The main effect of walking the previous versions is to find more
>>>> metadata to clean out though, for the next attempt at class unloading.
>>>>>
>>>>> I would prefer if this code could be kept inside the GC code or
>>>>> where it used to be, inside the ClassLoaderDataGraph::do_unloading
>>>>> function. With the current change, the
>>>>> VM_CollectForMetadataAllocation class is burdened with the
>>>>> internal knowledge about class redefinition and class unloading.
>>>>
>>>> In ClassLoaderDataGraph::do_unloading() is used when you're not in
>>>> a Full GC, which is the problem. It can't go there unless we pass
>>>> down the logic that we're in a full GC or not. I think this is
>>>> messier. I think this VM_CollectForMetadataAllocation seems the
>>>> right place to clean up metadata, if needed. One place or
>>>> another, there has to be knowledge of class redefinition.
>>>>
>>>> Actually, my original change had it not called at all, since this
>>>> code is executed for every class redefinition. Then I thought
>>>> something should call it... The bias should be to avoid calling
>>>> this function though because generally it doesn't find very much to
>>>> do.
>>>>
>>>> I thought a better place to call this would be for last-ditch
>>>> collections, but I didn't know exactly where that was.
>>>>
>>>> Coleen
>>>>>
>>>>> Thanks,
>>>>> StefanK
>>>>>
>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8061205
>>>>>>
>>>>>> Tested with FMW performance runs. vm.quick.testlist,
>>>>>> jdk/test/java/lang/instrument tests and JPRT.
>>>>>>
>>>>>> Thanks,
>>>>>> Coleen
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-dev
mailing list