8035841: assert(dp_src->tag() == dp_dst->tag()) failed: should be same tags 1...

8035841: assert(dp_src->tag() == dp_dst->tag()) failed: should be same tags 1 != 0 at ciMethodData.cpp:90
Summary: concurrent update of traps with construction of ciMethodData
Reviewed-by: kvn, twisti

hotspot/src/share/vm/ci/ciMethodData.cpp

@@ -87,8 +87,9 @@ void ciMethodData::load_extra_data() {
   DataLayout* dp_dst  = extra_data_base();
   for (;; dp_src = MethodData::next_extra(dp_src), dp_dst = MethodData::next_extra(dp_dst)) {
     assert(dp_src < end_src, "moved past end of extra data");
-    assert(dp_src->tag() == dp_dst->tag(), err_msg("should be same tags %d != %d", dp_src->tag(), dp_dst->tag()));
-    switch(dp_src->tag()) {
+    // New traps in the MDO can be added as we translate the copy so
+    // look at the entries in the copy.
+    switch(dp_dst->tag()) {
     case DataLayout::speculative_trap_data_tag: {
       ciSpeculativeTrapData* data_dst = new ciSpeculativeTrapData(dp_dst);
       SpeculativeTrapData* data_src = new SpeculativeTrapData(dp_src);
@@ -102,7 +103,7 @@ void ciMethodData::load_extra_data() {
       // An empty slot or ArgInfoData entry marks the end of the trap data
       return;
     default:
-      fatal(err_msg("bad tag = %d", dp_src->tag()));
+      fatal(err_msg("bad tag = %d", dp_dst->tag()));
     }
   }
 }

hotspot/src/share/vm/oops/methodData.cpp

@@ -1071,7 +1071,8 @@ void MethodData::post_initialize(BytecodeStream* stream) {
 }
 
 // Initialize the MethodData* corresponding to a given method.
-MethodData::MethodData(methodHandle method, int size, TRAPS) {
+MethodData::MethodData(methodHandle method, int size, TRAPS)
+  : _extra_data_lock(Monitor::leaf, "MDO extra data lock") {
   No_Safepoint_Verifier no_safepoint;  // init function atomic wrt GC
   ResourceMark rm;
   // Set the method back-pointer.
@@ -1235,7 +1236,7 @@ DataLayout* MethodData::next_extra(DataLayout* dp) {
   return (DataLayout*)((address)dp + DataLayout::compute_size_in_bytes(nb_cells));
 }
 
-ProfileData* MethodData::bci_to_extra_data_helper(int bci, Method* m, DataLayout*& dp) {
+ProfileData* MethodData::bci_to_extra_data_helper(int bci, Method* m, DataLayout*& dp, bool concurrent) {
   DataLayout* end = extra_data_limit();
 
   for (;; dp = next_extra(dp)) {
@@ -1257,10 +1258,11 @@ ProfileData* MethodData::bci_to_extra_data_helper(int bci, Method* m, DataLayout
       if (m != NULL) {
         SpeculativeTrapData* data = new SpeculativeTrapData(dp);
         // data->method() may be null in case of a concurrent
-        // allocation. Assume it's for the same method and use that
+        // allocation. Maybe it's for the same method. Try to use that
         // entry in that case.
         if (dp->bci() == bci) {
           if (data->method() == NULL) {
+            assert(concurrent, "impossible because no concurrent allocation");
             return NULL;
           } else if (data->method() == m) {
             return data;
@@ -1289,40 +1291,40 @@ ProfileData* MethodData::bci_to_extra_data(int bci, Method* m, bool create_if_mi
   // Allocation in the extra data space has to be atomic because not
   // all entries have the same size and non atomic concurrent
   // allocation would result in a corrupted extra data space.
-  while (true) {
-    ProfileData* result = bci_to_extra_data_helper(bci, m, dp);
-    if (result != NULL) {
+  ProfileData* result = bci_to_extra_data_helper(bci, m, dp, true);
+  if (result != NULL) {
+    return result;
+  }
+
+  if (create_if_missing && dp < end) {
+    MutexLocker ml(&_extra_data_lock);
+    // Check again now that we have the lock. Another thread may
+    // have added extra data entries.
+    ProfileData* result = bci_to_extra_data_helper(bci, m, dp, false);
+    if (result != NULL || dp >= end) {
       return result;
     }
 
-    if (create_if_missing && dp < end) {
-      assert(dp->tag() == DataLayout::no_tag || (dp->tag() == DataLayout::speculative_trap_data_tag && m != NULL), "should be free");
-      assert(next_extra(dp)->tag() == DataLayout::no_tag || next_extra(dp)->tag() == DataLayout::arg_info_data_tag, "should be free or arg info");
-      u1 tag = m == NULL ? DataLayout::bit_data_tag : DataLayout::speculative_trap_data_tag;
-      // SpeculativeTrapData is 2 slots. Make sure we have room.
-      if (m != NULL && next_extra(dp)->tag() != DataLayout::no_tag) {
-        return NULL;
-      }
-      DataLayout temp;
-      temp.initialize(tag, bci, 0);
-      // May have been set concurrently
-      if (dp->header() != temp.header() && !dp->atomic_set_header(temp.header())) {
-        // Allocation failure because of concurrent allocation. Try
-        // again.
-        continue;
-      }
-      assert(dp->tag() == tag, "sane");
-      assert(dp->bci() == bci, "no concurrent allocation");
-      if (tag == DataLayout::bit_data_tag) {
-        return new BitData(dp);
-      } else {
-        // If being allocated concurrently, one trap may be lost
-        SpeculativeTrapData* data = new SpeculativeTrapData(dp);
-        data->set_method(m);
-        return data;
-      }
+    assert(dp->tag() == DataLayout::no_tag || (dp->tag() == DataLayout::speculative_trap_data_tag && m != NULL), "should be free");
+    assert(next_extra(dp)->tag() == DataLayout::no_tag || next_extra(dp)->tag() == DataLayout::arg_info_data_tag, "should be free or arg info");
+    u1 tag = m == NULL ? DataLayout::bit_data_tag : DataLayout::speculative_trap_data_tag;
+    // SpeculativeTrapData is 2 slots. Make sure we have room.
+    if (m != NULL && next_extra(dp)->tag() != DataLayout::no_tag) {
+      return NULL;
+    }
+    DataLayout temp;
+    temp.initialize(tag, bci, 0);
+
+    dp->set_header(temp.header());
+    assert(dp->tag() == tag, "sane");
+    assert(dp->bci() == bci, "no concurrent allocation");
+    if (tag == DataLayout::bit_data_tag) {
+      return new BitData(dp);
+    } else {
+      SpeculativeTrapData* data = new SpeculativeTrapData(dp);
+      data->set_method(m);
+      return data;
     }
-    return NULL;
   }
   return NULL;
 }

hotspot/src/share/vm/oops/methodData.hpp

@@ -190,12 +190,6 @@ public:
   void set_header(intptr_t value) {
     _header._bits = value;
   }
-  bool atomic_set_header(intptr_t value) {
-    if (Atomic::cmpxchg_ptr(value, (volatile intptr_t*)&_header._bits, 0) == 0) {
-      return true;
-    }
-    return false;
-  }
   intptr_t header() {
     return _header._bits;
   }
@@ -2047,10 +2041,12 @@ private:
   // Cached hint for bci_to_dp and bci_to_data
   int _hint_di;
 
+  Mutex _extra_data_lock;
+
   MethodData(methodHandle method, int size, TRAPS);
 public:
   static MethodData* allocate(ClassLoaderData* loader_data, methodHandle method, TRAPS);
-  MethodData() {}; // For ciMethodData
+  MethodData() : _extra_data_lock(Monitor::leaf, "MDO extra data lock") {}; // For ciMethodData
 
   bool is_methodData() const volatile { return true; }
 
@@ -2155,7 +2151,7 @@ private:
   // What is the index of the first data entry?
   int first_di() const { return 0; }
 
-  ProfileData* bci_to_extra_data_helper(int bci, Method* m, DataLayout*& dp);
+  ProfileData* bci_to_extra_data_helper(int bci, Method* m, DataLayout*& dp, bool concurrent);
   // Find or create an extra ProfileData:
   ProfileData* bci_to_extra_data(int bci, Method* m, bool create_if_missing);