RFR (L) 8061205: MetadataOnStackMark only needs to walk code cache during class redefinition
Coleen Phillimore
coleen.phillimore at oracle.com
Thu Mar 12 18:16:50 UTC 2015
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.
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