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

[Stefan Karlsson]

Hi Thomas,

Thanks for reviewing these changes!

I've fixed most of your comments, but I've indicated below were I've 
deferred your cleanup suggestions.

On 2014-07-03 17:02, Thomas Schatzl wrote:
> Hi Mikael+Stefan,
>
> On Thu, 2014-07-03 at 13:14 +0200, 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/whitebox/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/classfile/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.
> Fine with me.
>
> I went through the change (again after the recent internal review) and
> could not find any big issue. Thanks for latest modifications.
>
> Here is a list of minor comments for this, most current change.
>
> Note that I am no expert in runtime/compiler code, but I think it looks
> reasonable :)
>
> classLoaderData.cpp:
>   - indentation of AllAliveClosure::found_dead() body wrong
>   - AllAliveClosure should be put under #ifdef ASSERT
>   - line 898 should be removed, the "next" declared in this line is not
> used (Klass* next = null; and another one within the while-loop)
>   - additional newline in line 917/918
>
> stringTable.cpp:
>    - maybe instead of "notify_gc" call the method
> "ensure_string_alive(...)". I would like that better. And potentially
> add a method "ensure_oop_alive()" or so in CollectedHeap which default
> implementation does nothing, and G1 overrides. That seems cleaner to me
> than the string table and the ciObjectFactory knowing about the
> G1SATB*randomstring*BS.

I'd like to handle this with a separate RFE.

>
> codeCache.cpp:
>    - extra added line 341
>    - first, thanks for making a fast-exit for the scavengable_nmethods()
> mechanism. Maybe instead of doing the early exit on UseG1GC, what do you
> think about adding a predicate in CollectedHeap about that? Not sure
> about a name, and it's up to you if you want to do that.

I agree that this isn't a nice solution. I think we should have one 
entry point for adding/removing the code roots remset and then dispatch 
to different implementations for the different GCs. I'd prefer to handle 
this as a separate cleanup.

>   
>    - either line 535 or 536 could be removed :)
>
> compiledIC.cpp:
>    - the indentation in line 102 to 107 seems to be messed up.
>    - newlines at nmethod::verify_icholder_relocations()
>
> nmethod.cpp:
>    - extra newline at 1303
>
> sharedHeap/g1CollectedHeap: the barriers implementation for the
> strongrootsscope and the G1CodeCacheUnloadingTask are different, maybe
> it would be good to make them similar. Both implementations seem to be
> okay.
>
> g1CollectedHeap.cpp:
>   - line 5226: "post-poned" -> "postponed" (I think)
>   - line 5235: additional newline
>
> In general I really like that G1 can do class unloading after remark
> now :)
> That helps a lot in longer-running applications.
>
> Thanks for your great work,

Thanks a lot!

StefanK

>    Thomas
>
>> 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
>