6912621: iCMS: Error: assert(_markBitMap.isMarked(addr + 1),"Missing Printezis bit?")

Summary: Fix block_size_if_printezis_bits() so it does not expect the bits, only uses them when available. Fix block_size_no_stall() so it does not stall when the bits are missing such cases, letting the caller deal with zero size returns. Constant pool cache oops do not need to be unparsable or conc_unsafe after their klass pointer is installed. Some cosmetic clean-ups and some assertion checking for conc-usafety which, in the presence of class file redefinition, has no a-priori time boundedness, so all GCs must be able to safely deal with putatively conc-unsafe objects in a stop-world pause.
Reviewed-by: jmasa, johnc

src/share/vm/classfile/classFileParser.cpp

@@ -331,7 +331,7 @@ constantPoolHandle ClassFileParser::parse_constant_pool(TRAPS) {
     length, CHECK_(nullHandle));
   constantPoolOop constant_pool =
                       oopFactory::new_constantPool(length,
-                                                   methodOopDesc::IsSafeConc,
+                                                   oopDesc::IsSafeConc,
                                                    CHECK_(nullHandle));
   constantPoolHandle cp (THREAD, constant_pool);
 
@@ -1929,10 +1929,9 @@ methodHandle ClassFileParser::parse_method(constantPoolHandle cp, bool is_interf
   }
 
   // All sizing information for a methodOop is finally available, now create it
-  methodOop m_oop  = oopFactory::new_method(
-    code_length, access_flags, linenumber_table_length,
-    total_lvt_length, checked_exceptions_length,
-    methodOopDesc::IsSafeConc, CHECK_(nullHandle));
+  methodOop m_oop  = oopFactory::new_method(code_length, access_flags, linenumber_table_length,
+                                            total_lvt_length, checked_exceptions_length,
+                                            oopDesc::IsSafeConc, CHECK_(nullHandle));
   methodHandle m (THREAD, m_oop);
 
   ClassLoadingService::add_class_method_size(m_oop->size()*HeapWordSize);

src/share/vm/gc_implementation/concurrentMarkSweep/compactibleFreeListSpace.cpp

@@ -1040,9 +1040,10 @@ const {
     } else {
       // must read from what 'p' points to in each loop.
       klassOop k = ((volatile oopDesc*)p)->klass_or_null();
-      if (k != NULL &&
-          ((oopDesc*)p)->is_parsable() &&
-          ((oopDesc*)p)->is_conc_safe()) {
+      // We trust the size of any object that has a non-NULL
+      // klass and (for those in the perm gen) is parsable
+      // -- irrespective of its conc_safe-ty.
+      if (k != NULL && ((oopDesc*)p)->is_parsable()) {
         assert(k->is_oop(), "Should really be klass oop.");
         oop o = (oop)p;
         assert(o->is_oop(), "Should be an oop");
@@ -1051,6 +1052,7 @@ const {
         assert(res != 0, "Block size should not be 0");
         return res;
       } else {
+        // May return 0 if P-bits not present.
         return c->block_size_if_printezis_bits(p);
       }
     }

src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp

@@ -6360,18 +6360,16 @@ size_t CMSCollector::block_size_using_printezis_bits(HeapWord* addr) const {
 // A variant of the above (block_size_using_printezis_bits()) except
 // that we return 0 if the P-bits are not yet set.
 size_t CMSCollector::block_size_if_printezis_bits(HeapWord* addr) const {
-  if (_markBitMap.isMarked(addr)) {
-    assert(_markBitMap.isMarked(addr + 1), "Missing Printezis bit?");
+  if (_markBitMap.isMarked(addr + 1)) {
+    assert(_markBitMap.isMarked(addr), "P-bit can be set only for marked objects");
     HeapWord* nextOneAddr = _markBitMap.getNextMarkedWordAddress(addr + 2);
     size_t size = pointer_delta(nextOneAddr + 1, addr);
     assert(size == CompactibleFreeListSpace::adjustObjectSize(size),
            "alignment problem");
     assert(size >= 3, "Necessary for Printezis marks to work");
     return size;
-  } else {
-    assert(!_markBitMap.isMarked(addr + 1), "Bit map inconsistency?");
-    return 0;
   }
+  return 0;
 }
 
 HeapWord* CMSCollector::next_card_start_after_block(HeapWord* addr) const {
@@ -9212,7 +9210,6 @@ bool MarkRefsIntoAndScanClosure::take_from_overflow_list() {
 
 size_t MarkDeadObjectsClosure::do_blk(HeapWord* addr) {
   size_t res = _sp->block_size_no_stall(addr, _collector);
-  assert(res != 0, "Should always be able to compute a size");
   if (_sp->block_is_obj(addr)) {
     if (_live_bit_map->isMarked(addr)) {
       // It can't have been dead in a previous cycle
@@ -9221,6 +9218,7 @@ size_t MarkDeadObjectsClosure::do_blk(HeapWord* addr) {
       _dead_bit_map->mark(addr);      // mark the dead object
     }
   }
+  // Could be 0, if the block size could not be computed without stalling.
   return res;
 }
 

src/share/vm/gc_interface/collectedHeap.cpp

@@ -100,8 +100,7 @@ void CollectedHeap::check_for_bad_heap_word_value(HeapWord* addr, size_t size) {
   }
 }
 
-void CollectedHeap::check_for_non_bad_heap_word_value(HeapWord* addr, size_t size)
- {
+void CollectedHeap::check_for_non_bad_heap_word_value(HeapWord* addr, size_t size) {
   if (CheckMemoryInitialization && ZapUnusedHeapArea) {
     for (size_t slot = 0; slot < size; slot += 1) {
       assert((*(intptr_t*) (addr + slot)) == ((intptr_t) badHeapWordVal),

src/share/vm/interpreter/rewriter.cpp

@@ -67,13 +67,11 @@ void Rewriter::compute_index_maps() {
 
 
 // Creates a constant pool cache given a CPC map
-// This creates the constant pool cache initially in a state
-// that is unsafe for concurrent GC processing but sets it to
-// a safe mode before the constant pool cache is returned.
 void Rewriter::make_constant_pool_cache(TRAPS) {
   const int length = _cp_cache_map.length();
   constantPoolCacheOop cache =
-      oopFactory::new_constantPoolCache(length, methodOopDesc::IsUnsafeConc, CHECK);
+      oopFactory::new_constantPoolCache(length, CHECK);
+  No_Safepoint_Verifier nsv;
   cache->initialize(_cp_cache_map);
 
   // Don't bother with the next pass if there is no JVM_CONSTANT_InvokeDynamic.

src/share/vm/memory/oopFactory.cpp

@@ -111,10 +111,9 @@ constantPoolOop oopFactory::new_constantPool(int length,
 
 
 constantPoolCacheOop oopFactory::new_constantPoolCache(int length,
-                                                       bool is_conc_safe,
                                                        TRAPS) {
   constantPoolCacheKlass* ck = constantPoolCacheKlass::cast(Universe::constantPoolCacheKlassObj());
-  return ck->allocate(length, is_conc_safe, CHECK_NULL);
+  return ck->allocate(length, CHECK_NULL);
 }
 
 

src/share/vm/memory/oopFactory.hpp

@@ -69,7 +69,6 @@ class oopFactory: AllStatic {
                                                     bool is_conc_safe,
                                                     TRAPS);
   static constantPoolCacheOop new_constantPoolCache(int length,
-                                                    bool is_conc_safe,
                                                     TRAPS);
 
   // Instance classes

src/share/vm/oops/constantPoolKlass.cpp

@@ -71,6 +71,12 @@ constantPoolOop constantPoolKlass::allocate(int length, bool is_conc_safe, TRAPS
   c->set_is_conc_safe(is_conc_safe);
   // all fields are initialized; needed for GC
 
+  // Note: because we may be in this "conc_unsafe" state when allocating
+  // t_oop below, which may in turn cause a GC, it is imperative that our
+  // size be correct, consistent and henceforth stable, at this stage.
+  assert(c->is_parsable(), "Else size() below is unreliable");
+  DEBUG_ONLY(int sz = c->size();)
+
   // initialize tag array
   // Note: cannot introduce constant pool handle before since it is not
   //       completely initialized (no class) -> would cause assertion failure
@@ -82,6 +88,8 @@ constantPoolOop constantPoolKlass::allocate(int length, bool is_conc_safe, TRAPS
   }
   pool->set_tags(tags());
 
+  // Check that our size was stable at its old value.
+  assert(sz == c->size(), "size() changed");
   return pool();
 }
 

src/share/vm/oops/cpCacheKlass.cpp

@@ -49,43 +49,31 @@ int constantPoolCacheKlass::oop_size(oop obj) const {
 
 
 constantPoolCacheOop constantPoolCacheKlass::allocate(int length,
-                                                      bool is_conc_safe,
                                                       TRAPS) {
   // allocate memory
   int size = constantPoolCacheOopDesc::object_size(length);
 
   KlassHandle klass (THREAD, as_klassOop());
 
-  // This is the original code.  The code from permanent_obj_allocate()
-  // was in-lined to allow the setting of is_conc_safe before the klass
-  // is installed.
+  // Commented out below is the original code.  The code from
+  // permanent_obj_allocate() was in-lined so that we could
+  // set the _length field, necessary to correctly compute its
+  // size(), before setting its klass word further below.
   // constantPoolCacheOop cache = (constantPoolCacheOop)
   //   CollectedHeap::permanent_obj_allocate(klass, size, CHECK_NULL);
 
   oop obj = CollectedHeap::permanent_obj_allocate_no_klass_install(klass, size, CHECK_NULL);
-  constantPoolCacheOop cache = (constantPoolCacheOop) obj;
-  cache->set_is_conc_safe(is_conc_safe);
-  // The store to is_conc_safe must be visible before the klass
-  // is set.  This should be done safely because _is_conc_safe has
-  // been declared volatile.  If there are any problems, consider adding
-  // OrderAccess::storestore();
-  CollectedHeap::post_allocation_install_obj_klass(klass, obj, size);
   NOT_PRODUCT(Universe::heap()->check_for_bad_heap_word_value((HeapWord*) obj,
                                                               size));
-
-  // The length field affects the size of the object.  The allocation
-  // above allocates the correct size (see calculation of "size") but
-  // the size() method of the constant pool cache oop will not reflect
-  // that size until the correct length is set.
-  cache->set_length(length);
-
-  // The store of the length must be visible before is_conc_safe is
-  // set to a safe state.
-  // This should be done safely because _is_conc_safe has
-  // been declared volatile.  If there are any problems, consider adding
-  // OrderAccess::storestore();
-  cache->set_is_conc_safe(methodOopDesc::IsSafeConc);
+  constantPoolCacheOop cache = (constantPoolCacheOop) obj;
+  assert(!UseConcMarkSweepGC || obj->klass_or_null() == NULL,
+         "klass should be NULL here when using CMS");
+  cache->set_length(length);  // should become visible before klass is set below.
   cache->set_constant_pool(NULL);
+
+  OrderAccess::storestore();
+  obj->set_klass(klass());
+  assert(cache->size() == size, "Incorrect cache->size()");
   return cache;
 }
 
@@ -176,11 +164,6 @@ int constantPoolCacheKlass::oop_adjust_pointers(oop obj) {
   return size;
 }
 
-bool constantPoolCacheKlass::oop_is_conc_safe(oop obj) const {
-  assert(obj->is_constantPoolCache(), "should be constant pool");
-  return constantPoolCacheOop(obj)->is_conc_safe();
-}
-
 #ifndef SERIALGC
 void constantPoolCacheKlass::oop_push_contents(PSPromotionManager* pm,
                                                oop obj) {

src/share/vm/oops/cpCacheKlass.hpp

@@ -39,7 +39,7 @@ class constantPoolCacheKlass: public Klass {
 
   // Allocation
   DEFINE_ALLOCATE_PERMANENT(constantPoolCacheKlass);
-  constantPoolCacheOop allocate(int length, bool is_conc_safe, TRAPS);
+  constantPoolCacheOop allocate(int length, TRAPS);
   static klassOop create_klass(TRAPS);
 
   // Casting from klassOop
@@ -55,7 +55,6 @@ class constantPoolCacheKlass: public Klass {
   // Garbage collection
   void oop_follow_contents(oop obj);
   int oop_adjust_pointers(oop obj);
-  virtual bool oop_is_conc_safe(oop obj) const;
 
   // Parallel Scavenge and Parallel Old
   PARALLEL_GC_DECLS

src/share/vm/oops/cpCacheOop.hpp

@@ -321,9 +321,6 @@ class constantPoolCacheOopDesc: public oopDesc {
  private:
   int             _length;
   constantPoolOop _constant_pool;                // the corresponding constant pool
-  // If true, safe for concurrent GC processing,
-  // Set unconditionally in constantPoolCacheKlass::allocate()
-  volatile bool        _is_conc_safe;
 
   // Sizing
   debug_only(friend class ClassVerifier;)
@@ -390,12 +387,6 @@ class constantPoolCacheOopDesc: public oopDesc {
     return entry_at(primary_index);
   }
 
-  // GC support
-  // If the _length field has not been set, the size of the
-  // constantPoolCache cannot be correctly calculated.
-  bool is_conc_safe()                            { return _is_conc_safe; }
-  void set_is_conc_safe(bool v)                  { _is_conc_safe = v; }
-
   // Code generation
   static ByteSize base_offset()                  { return in_ByteSize(sizeof(constantPoolCacheOopDesc)); }
   static ByteSize entry_offset(int raw_index) {

src/share/vm/oops/methodOop.cpp

@@ -985,9 +985,11 @@ methodHandle methodOopDesc:: clone_with_new_data(methodHandle m, u_char* new_cod
                                               IsUnsafeConc,
                                               CHECK_(methodHandle()));
   methodHandle newm (THREAD, newm_oop);
+  NOT_PRODUCT(int nmsz = newm->is_parsable() ? newm->size() : -1;)
   int new_method_size = newm->method_size();
   // Create a shallow copy of methodOopDesc part, but be careful to preserve the new constMethodOop
   constMethodOop newcm = newm->constMethod();
+  NOT_PRODUCT(int ncmsz = newcm->is_parsable() ? newcm->size() : -1;)
   int new_const_method_size = newm->constMethod()->object_size();
 
   memcpy(newm(), m(), sizeof(methodOopDesc));
@@ -999,9 +1001,19 @@ methodHandle methodOopDesc:: clone_with_new_data(methodHandle m, u_char* new_cod
   // or concurrent marking but those phases will be correct.  Setting and
   // resetting is done in preference to a careful copying into newcm to
   // avoid having to know the precise layout of a constMethodOop.
-  m->constMethod()->set_is_conc_safe(false);
+  m->constMethod()->set_is_conc_safe(oopDesc::IsUnsafeConc);
+  assert(m->constMethod()->is_parsable(), "Should remain parsable");
+
+  // NOTE: this is a reachable object that transiently signals "conc_unsafe"
+  // However, no allocations are done during this window
+  // during which it is tagged conc_unsafe, so we are assured that any concurrent
+  // thread will not wait forever for the object to revert to "conc_safe".
+  // Further, any such conc_unsafe object will indicate a stable size
+  // through the transition.
   memcpy(newcm, m->constMethod(), sizeof(constMethodOopDesc));
-  m->constMethod()->set_is_conc_safe(true);
+  m->constMethod()->set_is_conc_safe(oopDesc::IsSafeConc);
+  assert(m->constMethod()->is_parsable(), "Should remain parsable");
+
   // Reset correct method/const method, method size, and parameter info
   newcm->set_method(newm());
   newm->set_constMethod(newcm);
@@ -1035,6 +1047,8 @@ methodHandle methodOopDesc:: clone_with_new_data(methodHandle m, u_char* new_cod
 
   // Only set is_conc_safe to true when changes to newcm are
   // complete.
+  assert(!newm->is_parsable()  || nmsz  < 0 || newm->size()  == nmsz,  "newm->size()  inconsistency");
+  assert(!newcm->is_parsable() || ncmsz < 0 || newcm->size() == ncmsz, "newcm->size() inconsistency");
   newcm->set_is_conc_safe(true);
   return newm;
 }

src/share/vm/oops/methodOop.hpp

@@ -144,9 +144,6 @@ class methodOopDesc : public oopDesc {
 
  public:
 
-  static const bool IsUnsafeConc         = false;
-  static const bool IsSafeConc           = true;
-
   // accessors for instance variables
   constMethodOop constMethod() const             { return _constMethod; }
   void set_constMethod(constMethodOop xconst)    { oop_store_without_check((oop*)&_constMethod, (oop)xconst); }

src/share/vm/oops/oop.hpp

@@ -71,6 +71,11 @@ class oopDesc {
   static BarrierSet* _bs;
 
  public:
+  enum ConcSafeType {
+    IsUnsafeConc = false,
+    IsSafeConc   = true
+  };
+
   markOop  mark() const         { return _mark; }
   markOop* mark_addr() const    { return (markOop*) &_mark; }
 

src/share/vm/prims/jvmtiRedefineClasses.cpp

@@ -1247,12 +1247,12 @@ jvmtiError VM_RedefineClasses::merge_cp_and_rewrite(
   // Constant pools are not easily reused so we allocate a new one
   // each time.
   // merge_cp is created unsafe for concurrent GC processing.  It
-  // should be marked safe before discarding it because, even if
-  // garbage.  If it crosses a card boundary, it may be scanned
+  // should be marked safe before discarding it. Even though
+  // garbage,  if it crosses a card boundary, it may be scanned
   // in order to find the start of the first complete object on the card.
   constantPoolHandle merge_cp(THREAD,
     oopFactory::new_constantPool(merge_cp_length,
-                                 methodOopDesc::IsUnsafeConc,
+                                 oopDesc::IsUnsafeConc,
                                  THREAD));
   int orig_length = old_cp->orig_length();
   if (orig_length == 0) {
@@ -2343,7 +2343,7 @@ void VM_RedefineClasses::set_new_constant_pool(
     // sized constant pool with the klass to save space.
     constantPoolHandle smaller_cp(THREAD,
       oopFactory::new_constantPool(scratch_cp_length,
-                                   methodOopDesc::IsUnsafeConc,
+                                   oopDesc::IsUnsafeConc,
                                    THREAD));
     // preserve orig_length() value in the smaller copy
     int orig_length = scratch_cp->orig_length();

src/share/vm/prims/methodHandleWalk.cpp

@@ -1134,8 +1134,9 @@ int MethodHandleCompiler::cpool_primitive_put(BasicType bt, jvalue* con) {
 
 constantPoolHandle MethodHandleCompiler::get_constant_pool(TRAPS) const {
   constantPoolHandle nullHandle;
-  bool is_conc_safe = true;
-  constantPoolOop cpool_oop = oopFactory::new_constantPool(_constants.length(), is_conc_safe, CHECK_(nullHandle));
+  constantPoolOop cpool_oop = oopFactory::new_constantPool(_constants.length(),
+                                                           oopDesc::IsSafeConc,
+                                                           CHECK_(nullHandle));
   constantPoolHandle cpool(THREAD, cpool_oop);
 
   // Fill the real constant pool skipping the zero element.
@@ -1180,10 +1181,9 @@ methodHandle MethodHandleCompiler::get_method_oop(TRAPS) const {
   else
     flags_bits = (/*JVM_MH_INVOKE_BITS |*/ JVM_ACC_PUBLIC | JVM_ACC_FINAL | JVM_ACC_SYNTHETIC);
 
-  bool is_conc_safe = true;
   methodOop m_oop = oopFactory::new_method(bytecode_length(),
                                            accessFlags_from(flags_bits),
-                                           0, 0, 0, is_conc_safe, CHECK_(nullHandle));
+                                           0, 0, 0, oopDesc::IsSafeConc, CHECK_(nullHandle));
   methodHandle m(THREAD, m_oop);
   m_oop = NULL;  // oop not GC safe