RFR (L) 8061205: MetadataOnStackMark only needs to walk code cache during class redefinition
Coleen Phillimore
coleen.phillimore at oracle.com
Fri Mar 13 16:33:19 UTC 2015
Thanks Stefan. Serguei reviewed the class redefinition parts.
Coleen
On 3/13/15, 9:52 AM, Stefan Karlsson wrote:
>
> 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