7039089: G1: changeset for 7037276 broke heap verification, and related cleanups
Summary: In G1 heap verification, we no longer scan perm to G1-collected heap refs as part of process_strong_roots() but rather in a separate explicit oop iteration over the perm gen. This preserves the original perm card-marks. Added a new assertion in younger_refs_iterate() to catch a simple subcase where the user may have forgotten a prior save_marks() call, as happened in the case of G1's attempt to iterate perm to G1 refs when verifying the heap before exit. The assert was deliberately weakened for ParNew+CMS and will be fixed for that combination in a future CR. Also made some (non-G1) cleanups related to code and comments obsoleted by the migration of Symbols to the native heap.
Reviewed-by: iveresov, jmasa, tonyp
--- a/hotspot/src/share/vm/gc_implementation/concurrentMarkSweep/compactibleFreeListSpace.cpp Tue Apr 26 11:46:34 2011 -0700
+++ b/hotspot/src/share/vm/gc_implementation/concurrentMarkSweep/compactibleFreeListSpace.cpp Tue Apr 26 21:17:24 2011 -0700
@@ -1963,10 +1963,21 @@
// Iteration support, mostly delegated from a CMS generation
void CompactibleFreeListSpace::save_marks() {
- // mark the "end" of the used space at the time of this call;
+ assert(Thread::current()->is_VM_thread(),
+ "Global variable should only be set when single-threaded");
+ // Mark the "end" of the used space at the time of this call;
// note, however, that promoted objects from this point
// on are tracked in the _promoInfo below.
set_saved_mark_word(unallocated_block());
+#ifdef ASSERT
+ // Check the sanity of save_marks() etc.
+ MemRegion ur = used_region();
+ MemRegion urasm = used_region_at_save_marks();
+ assert(ur.contains(urasm),
+ err_msg(" Error at save_marks(): [" PTR_FORMAT "," PTR_FORMAT ")"
+ " should contain [" PTR_FORMAT "," PTR_FORMAT ")",
+ ur.start(), ur.end(), urasm.start(), urasm.end()));
+#endif
// inform allocator that promotions should be tracked.
assert(_promoInfo.noPromotions(), "_promoInfo inconsistency");
_promoInfo.startTrackingPromotions();
--- a/hotspot/src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp Tue Apr 26 11:46:34 2011 -0700
+++ b/hotspot/src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp Tue Apr 26 21:17:24 2011 -0700
@@ -3189,10 +3189,9 @@
}
void CMSCollector::setup_cms_unloading_and_verification_state() {
- const bool should_verify = VerifyBeforeGC || VerifyAfterGC || VerifyDuringGC
+ const bool should_verify = VerifyBeforeGC || VerifyAfterGC || VerifyDuringGC
|| VerifyBeforeExit;
- const int rso = SharedHeap::SO_Symbols | SharedHeap::SO_Strings
- | SharedHeap::SO_CodeCache;
+ const int rso = SharedHeap::SO_Strings | SharedHeap::SO_CodeCache;
if (should_unload_classes()) { // Should unload classes this cycle
remove_root_scanning_option(rso); // Shrink the root set appropriately
--- a/hotspot/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp Tue Apr 26 11:46:34 2011 -0700
+++ b/hotspot/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp Tue Apr 26 21:17:24 2011 -0700
@@ -2805,17 +2805,26 @@
bool silent,
bool use_prev_marking) {
if (SafepointSynchronize::is_at_safepoint() || ! UseTLAB) {
- if (!silent) { gclog_or_tty->print("roots "); }
+ if (!silent) { gclog_or_tty->print("Roots (excluding permgen) "); }
VerifyRootsClosure rootsCl(use_prev_marking);
CodeBlobToOopClosure blobsCl(&rootsCl, /*do_marking=*/ false);
- process_strong_roots(true, // activate StrongRootsScope
- false,
- SharedHeap::SO_AllClasses,
+ // We apply the relevant closures to all the oops in the
+ // system dictionary, the string table and the code cache.
+ const int so = SharedHeap::SO_AllClasses | SharedHeap::SO_Strings | SharedHeap::SO_CodeCache;
+ process_strong_roots(true, // activate StrongRootsScope
+ true, // we set "collecting perm gen" to true,
+ // so we don't reset the dirty cards in the perm gen.
+ SharedHeap::ScanningOption(so), // roots scanning options
&rootsCl,
&blobsCl,
&rootsCl);
+ // Since we used "collecting_perm_gen" == true above, we will not have
+ // checked the refs from perm into the G1-collected heap. We check those
+ // references explicitly below. Whether the relevant cards are dirty
+ // is checked further below in the rem set verification.
+ if (!silent) { gclog_or_tty->print("Permgen roots "); }
+ perm_gen()->oop_iterate(&rootsCl);
bool failures = rootsCl.failures();
- rem_set()->invalidate(perm_gen()->used_region(), false);
if (!silent) { gclog_or_tty->print("HeapRegionSets "); }
verify_region_sets();
if (!silent) { gclog_or_tty->print("HeapRegions "); }
--- a/hotspot/src/share/vm/memory/cardTableRS.cpp Tue Apr 26 11:46:34 2011 -0700
+++ b/hotspot/src/share/vm/memory/cardTableRS.cpp Tue Apr 26 21:17:24 2011 -0700
@@ -250,7 +250,34 @@
cl->gen_boundary());
ClearNoncleanCardWrapper clear_cl(dcto_cl, this);
- _ct_bs->non_clean_card_iterate_possibly_parallel(sp, sp->used_region_at_save_marks(),
+ const MemRegion urasm = sp->used_region_at_save_marks();
+#ifdef ASSERT
+ // Convert the assertion check to a warning if we are running
+ // CMS+ParNew until related bug is fixed.
+ MemRegion ur = sp->used_region();
+ assert(ur.contains(urasm) || (UseConcMarkSweepGC && UseParNewGC),
+ err_msg("Did you forget to call save_marks()? "
+ "[" PTR_FORMAT ", " PTR_FORMAT ") is not contained in "
+ "[" PTR_FORMAT ", " PTR_FORMAT ")",
+ urasm.start(), urasm.end(), ur.start(), ur.end()));
+ // In the case of CMS+ParNew, issue a warning
+ if (!ur.contains(urasm)) {
+ assert(UseConcMarkSweepGC && UseParNewGC, "Tautology: see assert above");
+ warning("CMS+ParNew: Did you forget to call save_marks()? "
+ "[" PTR_FORMAT ", " PTR_FORMAT ") is not contained in "
+ "[" PTR_FORMAT ", " PTR_FORMAT ")",
+ urasm.start(), urasm.end(), ur.start(), ur.end());
+ MemRegion ur2 = sp->used_region();
+ MemRegion urasm2 = sp->used_region_at_save_marks();
+ if (!ur.equals(ur2)) {
+ warning("CMS+ParNew: Flickering used_region()!!");
+ }
+ if (!urasm.equals(urasm2)) {
+ warning("CMS+ParNew: Flickering used_region_at_save_marks()!!");
+ }
+ }
+#endif
+ _ct_bs->non_clean_card_iterate_possibly_parallel(sp, urasm,
dcto_cl, &clear_cl);
}
--- a/hotspot/src/share/vm/memory/genCollectedHeap.hpp Tue Apr 26 11:46:34 2011 -0700
+++ b/hotspot/src/share/vm/memory/genCollectedHeap.hpp Tue Apr 26 21:17:24 2011 -0700
@@ -427,13 +427,13 @@
// explicitly mark reachable objects in younger generations, to avoid
// excess storage retention.) If "collecting_perm_gen" is false, then
// roots that may only contain references to permGen objects are not
- // scanned. The "so" argument determines which of the roots
+ // scanned; instead, the older_gens closure is applied to all outgoing
+ // references in the perm gen. The "so" argument determines which of the roots
// the closure is applied to:
// "SO_None" does none;
// "SO_AllClasses" applies the closure to all entries in the SystemDictionary;
// "SO_SystemClasses" to all the "system" classes and loaders;
- // "SO_Symbols_and_Strings" applies the closure to all entries in
- // SymbolsTable and StringTable.
+ // "SO_Strings" applies the closure to all entries in the StringTable.
void gen_process_strong_roots(int level,
bool younger_gens_as_roots,
// The remaining arguments are in an order
--- a/hotspot/src/share/vm/memory/sharedHeap.cpp Tue Apr 26 11:46:34 2011 -0700
+++ b/hotspot/src/share/vm/memory/sharedHeap.cpp Tue Apr 26 21:17:24 2011 -0700
@@ -46,7 +46,6 @@
SH_PS_Management_oops_do,
SH_PS_SystemDictionary_oops_do,
SH_PS_jvmti_oops_do,
- SH_PS_SymbolTable_oops_do,
SH_PS_StringTable_oops_do,
SH_PS_CodeCache_oops_do,
// Leave this one last.
@@ -161,13 +160,9 @@
if (!_process_strong_tasks->is_task_claimed(SH_PS_SystemDictionary_oops_do)) {
if (so & SO_AllClasses) {
SystemDictionary::oops_do(roots);
- } else
- if (so & SO_SystemClasses) {
- SystemDictionary::always_strong_oops_do(roots);
- }
- }
-
- if (!_process_strong_tasks->is_task_claimed(SH_PS_SymbolTable_oops_do)) {
+ } else if (so & SO_SystemClasses) {
+ SystemDictionary::always_strong_oops_do(roots);
+ }
}
if (!_process_strong_tasks->is_task_claimed(SH_PS_StringTable_oops_do)) {
--- a/hotspot/src/share/vm/memory/sharedHeap.hpp Tue Apr 26 11:46:34 2011 -0700
+++ b/hotspot/src/share/vm/memory/sharedHeap.hpp Tue Apr 26 21:17:24 2011 -0700
@@ -192,9 +192,8 @@
SO_None = 0x0,
SO_AllClasses = 0x1,
SO_SystemClasses = 0x2,
- SO_Symbols = 0x4,
- SO_Strings = 0x8,
- SO_CodeCache = 0x10
+ SO_Strings = 0x4,
+ SO_CodeCache = 0x8
};
FlexibleWorkGang* workers() const { return _workers; }
@@ -208,14 +207,13 @@
// Invoke the "do_oop" method the closure "roots" on all root locations.
// If "collecting_perm_gen" is false, then roots that may only contain
- // references to permGen objects are not scanned. If true, the
- // "perm_gen" closure is applied to all older-to-younger refs in the
+ // references to permGen objects are not scanned; instead, in that case,
+ // the "perm_blk" closure is applied to all outgoing refs in the
// permanent generation. The "so" argument determines which of roots
// the closure is applied to:
// "SO_None" does none;
// "SO_AllClasses" applies the closure to all entries in the SystemDictionary;
// "SO_SystemClasses" to all the "system" classes and loaders;
- // "SO_Symbols" applies the closure to all entries in SymbolsTable;
// "SO_Strings" applies the closure to all entries in StringTable;
// "SO_CodeCache" applies the closure to all elements of the CodeCache.
void process_strong_roots(bool activate_scope,
--- a/hotspot/src/share/vm/runtime/vmThread.cpp Tue Apr 26 11:46:34 2011 -0700
+++ b/hotspot/src/share/vm/runtime/vmThread.cpp Tue Apr 26 21:17:24 2011 -0700
@@ -291,7 +291,9 @@
// Among other things, this ensures that Eden top is correct.
Universe::heap()->prepare_for_verify();
os::check_heap();
- Universe::verify(true, true); // Silent verification to not polute normal output
+ // Silent verification so as not to pollute normal output,
+ // unless we really asked for it.
+ Universe::verify(true, !(PrintGCDetails || Verbose));
}
CompileBroker::set_should_block();