RFR: 8048248: G1 Class Unloading after completing a concurrent mark cycle

Sat Jul 5 17:05:41 UTC 2014

Hi, I have looked at the runtime and metadata changes and glanced at one 
file of g1 changes.  I have a couple of comments but this looks good.

http://cr.openjdk.java.net/~stefank/8048248/webrev.02/src/share/vm/utilities/array.hpp.udiff.html
Why does this include classLoaderData.hpp?

http://cr.openjdk.java.net/~stefank/8048248/webrev.02/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp.frames.html
Does this have to include metadataOnStackMark.hpp ?  Or is that leftover 
from the parallel metadataOnStackMark code?

5068 class G1CodeCacheUnloadingTask {
5235 class G1KlassCleaningTask {

These need to inherit from allocation types (CHeapObj, ResourceObj or 
whatever for NMT).  Can you check your other classes to see if they have 
inherited from memory allocation classes too?

For the G1ClassCleaningTask and parallel cleaning weak links, you might 
want a comment to explain why this is too slow serially and must be done 
with all this code.

Also please add a comment in front of 
ClassLoaderDataGraphKlassIteratorAtomic in classLoaderData.hpp what this 
is used for (and maybe why).  I think related to above.

That's all I saw that didn't look okay to me.   Reviewed.   This is a 
ton of work!

Coleen

On 7/4/14, 11:41 AM, Erik Helin wrote:
> Hi Stefan and Mikael,
>
> thanks for all your hard work with this patch!
>
> On Thursday 03 July 2014 21:57:29 PM Stefan Karlsson wrote:
>> Hi,
>>
>> A new patch can be found at:
>> http://cr.openjdk.java.net/~stefank/8048248/webrev.02/
>> http://cr.openjdk.java.net/~stefank/8048248/webrev.02.delta/
> This looks good to me, Reviewed!
>
> Thanks,
> Erik
>
>> The new patch:
>> 1) Fixes a bug when the user specifies -XX:ParallelGCThreads=0
>>
>> 2) Fixes most of Thomas Schatzl's review comments
>>
>> 3) Fixes a bug in how G1RemarkGCTraceTime is used, which caused
>> incorrect measurements of the phases "System Dictionary Unloading" and
>> "Parallel Unloading".
>>
>> thanks,
>> StefanK
>>
>> On 2014-07-03 13:14, Stefan Karlsson wrote:
>>> Hi again,
>>>
>>> Here's a new patch.
>>>
>>> Changes:
>>> 1) The first webrev didn't include the change to this file:
>>> http://cr.openjdk.java.net/~stefank/8048248/webrev.01/test/testlibrary/whi
>>> tebox/sun/hotspot/WhiteBox.java.udiff.html
>>>
>>>
>>> 2) Fixes a bug that happens with Class Redefinition.
>>> http://cr.openjdk.java.net/~stefank/8048248/webrev.01/src/share/vm/classfi
>>> le/metadataOnStackMark.cpp.udiff.html
>>>
>>>
>>> We don't want to call
>>> CodeCache::alive_nmethods_do(nmethod::mark_on_stack) unnecessarily,
>>> since its one of the more expensive operations done during the remark
>>> pause. If Class Redefinition isn't used we don't need to mark through
>>> the code cache, since no deallocated metadata should have made its way
>>> into a nmethod. Unfortunately, this is not true for Class
>>> Redefinition. Class Redefinition will create new versions of Methods
>>> and ConstantPools and needs to keep the old versions alive until all
>>> references to the old version have been cleaned out from the JVM.
>>>
>>> The current patch only calls
>>> CodeCache::alive_nmethods_do(nmethod::mark_on_stack) if Class
>>> Redefintion is used. This has the effect that code using Class
>>> Redefinition will have higher remark pauses.
>>>
>>> In an earlier version of the G1 class unloading patch I parallelized
>>> and combined the nmethod mark_on_stack code with the CodeCache
>>> cleaning code, but it was removed in favor of removing the call to
>>> CodeCache::alive_nmethods_do. We might want to revive that patch, or
>>> optimize this some other way, but I would prefer to not do that in
>>> this patch.
>>>
>>> thanks,
>>> StefanK
>>>
>>> On 2014-07-01 15:44, Stefan Karlsson wrote:
>>>> Hi all,
>>>>
>>>> Please, review this patch to enable unloading of classes and other
>>>> metadata after a G1 concurrent cycle.
>>>>
>>>> http://cr.openjdk.java.net/~stefank/8048248/webrev.00/
>>>> https://bugs.openjdk.java.net/browse/JDK-8048248
>>>>
>>>> The patch includes the following changes:
>>>>
>>>> 1) Tracing through alive Klasses and CLDs during concurrent mark,
>>>> instead of marking all of them during the initial mark pause.
>>>> 2) Making HeapRegions walkable in the presence of unparseable objects
>>>> due to their classes being unloaded.
>>>> 3) The process roots code has been changed to allow G1's combined
>>>> initial mark and scavenge.
>>>> 4) The CodeBlobClosures have been refactored to distinguish the
>>>> marking variant from the oop updating variants.
>>>> 5) Calls to the G1 pre-barrier have been added to some places, such
>>>> as the StringTable, to guard against object resurrection, similar to
>>>> how j.l.ref.Reference#get is treated with a read barrier.
>>>> 6) Parallelizing the cleaning of metadata and compiled methods during
>>>> the remark pause.
>>>>
>>>> A number of patches to prepare for this RFE has already been pushed
>>>> to JDK 9:
>>>>
>>>> 8047362: Add a version of CompiledIC_at that doesn't create a new
>>>> RelocIterator
>>>> 8047326: Consolidate all CompiledIC::CompiledIC implementations and
>>>> move it to compiledIC.cpp
>>>> 8047323: Remove unused _copy_metadata_obj_cl in
>>>> G1CopyingKeepAliveClosure
>>>> 8047373: Clean the ExceptionCache in one pass
>>>> 8046670: Make CMS metadata aware closures applicable for other
>>>> collectors
>>>> 8035746: Add missing Klass::oop_is_instanceClassLoader() function
>>>> 8035648: Don't use Handle in java_lang_String::print
>>>> 8035412: Cleanup ClassLoaderData::is_alive
>>>> 8035393: Use CLDClosure instead of CLDToOopClosure in
>>>> frame::oops_interpreted_do
>>>> 8034764: Use process_strong_roots to adjust the StringTable
>>>> 8034761: Remove the do_code_roots parameter from process_strong_roots
>>>> 8033923: Use BufferingOopClosure for G1 code root scanning
>>>> 8033764: Remove the usage of StarTask from BufferingOopClosure
>>>> 8012687: Remove unused is_root checks and closures
>>>> 8047818: G1 HeapRegions can no longer be ContiguousSpaces
>>>> 8048214: Linker error when compiling G1SATBCardTableModRefBS after
>>>> include order changes
>>>> 8047821: G1 Does not use the save_marks functionality as intended
>>>> 8047820: G1 Block offset table does not need to support generic Space
>>>> classes
>>>> 8047819: G1 HeapRegionDCTOC does not need to inherit
>>>> ContiguousSpaceDCTOC
>>>> 8038405: Clean up some virtual fucntions in Space class hierarchy
>>>> 8038412: Move object_iterate_careful down from Space to
>>>> ContigousSpace and CFLSpace
>>>> 8038404: Move object_iterate_mem from Space to CMS since it is only
>>>> ever used by CMS
>>>> 8038399: Remove dead oop_iterate MemRegion variants from SharedHeap,
>>>> Generation and Space classe
>>>> 8037958: ConcurrentMark::cleanup leaks BitMaps if VerifyDuringGC is
>>>> enabled
>>>> 8032379: Remove the is_scavenging flag to process_strong_roots
>>>>
>>>> Testing:
>>>>
>>>> We've been running Kitchensink, gc-test-suite, internal nightly
>>>> testing and test lists, and CRM FA benchmarks.
>>>>
>>>> thanks,
>>>> StefanK & Mikael Gerdin