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

Thu Jul 3 15:02:10 UTC 2014

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.

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. 
  - 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,
  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
> 