RFR: 8048248: G1 Class Unloading after completing a concurrent mark cycle
stefan.karlsson at oracle.com
Thu Jul 3 20:02:03 UTC 2014
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.
>> 1) The first webrev didn't include the change to this file:
>> 2) Fixes a bug that happens with Class Redefinition.
>> 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
> 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 :)
> - 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
> - 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
I'd like to handle this with a separate RFE.
> - 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 :)
> - the indentation in line 102 to 107 seems to be messed up.
> - newlines at nmethod::verify_icholder_relocations()
> - 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
> - 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!
>> 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.
>>> 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
>>> 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
>>> 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
>>> 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
>>> 8032379: Remove the is_scavenging flag to process_strong_roots
>>> We've been running Kitchensink, gc-test-suite, internal nightly
>>> testing and test lists, and CRM FA benchmarks.
>>> StefanK & Mikael Gerdin
More information about the hotspot-dev