RFR (L) 8061205: MetadataOnStackMark only needs to walk code cache during class redefinition
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Thu Mar 12 09:45:45 UTC 2015
Coleen,
The updated webrev .03 looks good.
Thank you for the changes!
Thanks,
Serguei
On 3/11/15 1:48 PM, Coleen Phillimore wrote:
>
> Stefan,
>
> I made the changes you suggested. I also moved
> purge_previous_versions conditionally back to class unloading, where
> G1 doesn't do this until full GC. 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/
>
> 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