8015270: @Contended: fix multiple issues in the layout code

Summary: field count handling fixed, has_nonstatic_fields invariant fixed, oop map overrun fixed; new asserts
Reviewed-by: kvn, dcubed, coleenp

src/share/vm/classfile/classFileParser.cpp

@@ -3152,7 +3152,6 @@ void ClassFileParser::layout_fields(Handle class_loader,
       }
     }
   }
-  int contended_count = nonstatic_contended_count;
 
 
   // Calculate the starting byte offsets
@@ -3177,35 +3176,52 @@ void ClassFileParser::layout_fields(Handle class_loader,
 
   next_nonstatic_field_offset = nonstatic_fields_start;
 
+  bool is_contended_class     = parsed_annotations->is_contended();
+
   // Class is contended, pad before all the fields
-  if (parsed_annotations->is_contended()) {
+  if (is_contended_class) {
     next_nonstatic_field_offset += ContendedPaddingWidth;
   }
 
-  // Compute the non-contended fields count
+  // Compute the non-contended fields count.
+  // The packing code below relies on these counts to determine if some field
+  // can be squeezed into the alignment gap. Contended fields are obviously
+  // exempt from that.
   unsigned int nonstatic_double_count = fac->count[NONSTATIC_DOUBLE] - fac_contended.count[NONSTATIC_DOUBLE];
   unsigned int nonstatic_word_count   = fac->count[NONSTATIC_WORD]   - fac_contended.count[NONSTATIC_WORD];
   unsigned int nonstatic_short_count  = fac->count[NONSTATIC_SHORT]  - fac_contended.count[NONSTATIC_SHORT];
   unsigned int nonstatic_byte_count   = fac->count[NONSTATIC_BYTE]   - fac_contended.count[NONSTATIC_BYTE];
   unsigned int nonstatic_oop_count    = fac->count[NONSTATIC_OOP]    - fac_contended.count[NONSTATIC_OOP];
 
+  // Total non-static fields count, including every contended field
+  unsigned int nonstatic_fields_count = fac->count[NONSTATIC_DOUBLE] + fac->count[NONSTATIC_WORD] +
+                                        fac->count[NONSTATIC_SHORT] + fac->count[NONSTATIC_BYTE] +
+                                        fac->count[NONSTATIC_OOP];
+
   bool super_has_nonstatic_fields =
           (_super_klass() != NULL && _super_klass->has_nonstatic_fields());
-  bool has_nonstatic_fields = super_has_nonstatic_fields ||
-          ((nonstatic_double_count + nonstatic_word_count +
-            nonstatic_short_count + nonstatic_byte_count +
-            nonstatic_oop_count) != 0);
+  bool has_nonstatic_fields = super_has_nonstatic_fields || (nonstatic_fields_count != 0);
 
 
   // Prepare list of oops for oop map generation.
+  //
+  // "offset" and "count" lists are describing the set of contiguous oop
+  // regions. offset[i] is the start of the i-th region, which then has
+  // count[i] oops following. Before we know how many regions are required,
+  // we pessimistically allocate the maps to fit all the oops into the
+  // distinct regions.
+  //
+  // TODO: We add +1 to always allocate non-zero resource arrays; we need
+  // to figure out if we still need to do this.
   int* nonstatic_oop_offsets;
   unsigned int* nonstatic_oop_counts;
   unsigned int nonstatic_oop_map_count = 0;
+  unsigned int max_nonstatic_oop_maps  = fac->count[NONSTATIC_OOP] + 1;
 
   nonstatic_oop_offsets = NEW_RESOURCE_ARRAY_IN_THREAD(
-            THREAD, int, nonstatic_oop_count + 1);
+            THREAD, int, max_nonstatic_oop_maps);
   nonstatic_oop_counts  = NEW_RESOURCE_ARRAY_IN_THREAD(
-            THREAD, unsigned int, nonstatic_oop_count + 1);
+            THREAD, unsigned int, max_nonstatic_oop_maps);
 
   first_nonstatic_oop_offset = 0; // will be set for first oop field
 
@@ -3392,9 +3408,11 @@ void ClassFileParser::layout_fields(Handle class_loader,
             int(nonstatic_oop_counts[nonstatic_oop_map_count - 1]) *
             heapOopSize ) {
           // Extend current oop map
+          assert(nonstatic_oop_map_count - 1 < max_nonstatic_oop_maps, "range check");
           nonstatic_oop_counts[nonstatic_oop_map_count - 1] += 1;
         } else {
           // Create new oop map
+          assert(nonstatic_oop_map_count < max_nonstatic_oop_maps, "range check");
           nonstatic_oop_offsets[nonstatic_oop_map_count] = real_offset;
           nonstatic_oop_counts [nonstatic_oop_map_count] = 1;
           nonstatic_oop_map_count += 1;
@@ -3452,12 +3470,10 @@ void ClassFileParser::layout_fields(Handle class_loader,
   //
   // Additionally, this should not break alignment for the fields, so we round the alignment up
   // for each field.
-  if (contended_count > 0) {
+  if (nonstatic_contended_count > 0) {
 
     // if there is at least one contended field, we need to have pre-padding for them
-    if (nonstatic_contended_count > 0) {
-      next_nonstatic_padded_offset += ContendedPaddingWidth;
-    }
+    next_nonstatic_padded_offset += ContendedPaddingWidth;
 
     // collect all contended groups
     BitMap bm(_cp->size());
@@ -3518,6 +3534,7 @@ void ClassFileParser::layout_fields(Handle class_loader,
             next_nonstatic_padded_offset += heapOopSize;
 
             // Create new oop map
+            assert(nonstatic_oop_map_count < max_nonstatic_oop_maps, "range check");
             nonstatic_oop_offsets[nonstatic_oop_map_count] = real_offset;
             nonstatic_oop_counts [nonstatic_oop_map_count] = 1;
             nonstatic_oop_map_count += 1;
@@ -3554,18 +3571,17 @@ void ClassFileParser::layout_fields(Handle class_loader,
     // handle static fields
   }
 
-  // Size of instances
-  int notaligned_offset = next_nonstatic_padded_offset;
-
   // Entire class is contended, pad in the back.
   // This helps to alleviate memory contention effects for subclass fields
   // and/or adjacent object.
-  if (parsed_annotations->is_contended()) {
-    notaligned_offset += ContendedPaddingWidth;
+  if (is_contended_class) {
+    next_nonstatic_padded_offset += ContendedPaddingWidth;
   }
 
-  int nonstatic_fields_end      = align_size_up(notaligned_offset, heapOopSize);
-  int instance_end              = align_size_up(notaligned_offset, wordSize);
+  int notaligned_nonstatic_fields_end = next_nonstatic_padded_offset;
+
+  int nonstatic_fields_end      = align_size_up(notaligned_nonstatic_fields_end, heapOopSize);
+  int instance_end              = align_size_up(notaligned_nonstatic_fields_end, wordSize);
   int static_fields_end         = align_size_up(next_static_byte_offset, wordSize);
 
   int static_field_size         = (static_fields_end -
@@ -3579,6 +3595,14 @@ void ClassFileParser::layout_fields(Handle class_loader,
          (instanceOopDesc::base_offset_in_bytes() + nonstatic_field_size*heapOopSize),
           wordSize) / wordSize), "consistent layout helper value");
 
+  // Invariant: nonstatic_field end/start should only change if there are
+  // nonstatic fields in the class, or if the class is contended. We compare
+  // against the non-aligned value, so that end alignment will not fail the
+  // assert without actually having the fields.
+  assert((notaligned_nonstatic_fields_end == nonstatic_fields_start) ||
+         is_contended_class ||
+         (nonstatic_fields_count > 0), "double-check nonstatic start/end");
+
   // Number of non-static oop map blocks allocated at end of klass.
   const unsigned int total_oop_map_count =
     compute_oop_map_count(_super_klass, nonstatic_oop_map_count,

test/runtime/contended/HasNonStatic.java

+/*
+ * Copyright (c) 2013, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+import java.io.BufferedReader;
+import java.io.InputStreamReader;
+import java.lang.Class;
+import java.lang.String;
+import java.lang.System;
+import java.lang.management.ManagementFactory;
+import java.lang.management.RuntimeMXBean;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.CyclicBarrier;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import java.lang.reflect.Field;
+import java.lang.reflect.Modifier;
+import sun.misc.Unsafe;
+import sun.misc.Contended;
+
+/*
+ * @test
+ * @bug     8015270
+ * @summary \@Contended: fix multiple issues in the layout code
+ *
+ * @run main/othervm -XX:-RestrictContended HasNonStatic
+ */
+public class HasNonStatic {
+
+    public static void main(String[] args) throws Exception {
+        R1 r1 = new R1();
+        R2 r2 = new R2();
+        R3 r3 = new R3();
+        R4 r4 = new R4();
+    }
+
+    public static class R1 {
+        @Contended
+        Object o;
+    }
+
+    @Contended
+    public static class R2 {
+        Object o;
+    }
+
+    @Contended
+    public static class R3 {
+    }
+
+    public static class R4 extends R3 {
+    }
+
+}
+

test/runtime/contended/OopMaps.java

+/*
+ * Copyright (c) 2013, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+import java.io.BufferedReader;
+import java.io.InputStreamReader;
+import java.lang.Class;
+import java.lang.String;
+import java.lang.System;
+import java.lang.management.ManagementFactory;
+import java.lang.management.RuntimeMXBean;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.CyclicBarrier;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import java.lang.reflect.Field;
+import java.lang.reflect.Modifier;
+import sun.misc.Unsafe;
+import sun.misc.Contended;
+
+/*
+ * @test
+ * @bug     8015270
+ * @summary \@Contended: fix multiple issues in the layout code
+ *
+ * @run main/othervm -XX:-RestrictContended OopMaps
+ */
+public class OopMaps {
+
+    public static void main(String[] args) throws Exception {
+        Object o01 = new Object();
+        Object o02 = new Object();
+        Object o03 = new Object();
+        Object o04 = new Object();
+        Object o05 = new Object();
+        Object o06 = new Object();
+        Object o07 = new Object();
+        Object o08 = new Object();
+        Object o09 = new Object();
+        Object o10 = new Object();
+        Object o11 = new Object();
+        Object o12 = new Object();
+        Object o13 = new Object();
+        Object o14 = new Object();
+
+        final int COUNT = 100000;
+        R1[] rs = new R1[COUNT];
+
+        for (int i = 0; i < COUNT; i++) {
+           R1 r1 = new R1();
+           r1.o01 = o01;
+           r1.o02 = o02;
+           r1.o03 = o03;
+           r1.o04 = o04;
+           r1.o05 = o05;
+           r1.o06 = o06;
+           r1.o07 = o07;
+           r1.o08 = o08;
+           r1.o09 = o09;
+           r1.o10 = o10;
+           r1.o11 = o11;
+           r1.o12 = o12;
+           r1.o13 = o13;
+           r1.o14 = o14;
+           r1.i1 = 1;
+           r1.i2 = 2;
+           r1.i3 = 3;
+           r1.i4 = 4;
+           rs[i] = r1;
+        }
+
+        System.gc();
+
+        for (int i = 0; i < COUNT; i++) {
+           R1 r1 = rs[i];
+           if (r1.o01 != o01) throw new Error("Test Error: o01");
+           if (r1.o02 != o02) throw new Error("Test Error: o02");
+           if (r1.o03 != o03) throw new Error("Test Error: o03");
+           if (r1.o04 != o04) throw new Error("Test Error: o04");
+           if (r1.o05 != o05) throw new Error("Test Error: o05");
+           if (r1.o06 != o06) throw new Error("Test Error: o06");
+           if (r1.o07 != o07) throw new Error("Test Error: o07");
+           if (r1.o08 != o08) throw new Error("Test Error: o08");
+           if (r1.o09 != o09) throw new Error("Test Error: o09");
+           if (r1.o10 != o10) throw new Error("Test Error: o10");
+           if (r1.o11 != o11) throw new Error("Test Error: o11");
+           if (r1.o12 != o12) throw new Error("Test Error: o12");
+           if (r1.o13 != o13) throw new Error("Test Error: o13");
+           if (r1.o14 != o14) throw new Error("Test Error: o14");
+           if (r1.i1 != 1)    throw new Error("Test Error: i1");
+           if (r1.i2 != 2)    throw new Error("Test Error: i2");
+           if (r1.i3 != 3)    throw new Error("Test Error: i3");
+           if (r1.i4 != 4)    throw new Error("Test Error: i4");
+        }
+    }
+
+    public static class R0 {
+        int i1;
+        int i2;
+
+        Object o01;
+        Object o02;
+
+        @Contended
+        Object o03;
+
+        @Contended
+        Object o04;
+
+        @Contended
+        Object o05;
+
+        @Contended
+        Object o06;
+
+        @Contended
+        Object o07;
+   }
+
+   public static class R1 extends R0 {
+        int i3;
+        int i4;
+
+        Object o08;
+        Object o09;
+
+        @Contended
+        Object o10;
+
+        @Contended
+        Object o11;
+
+        @Contended
+        Object o12;
+
+        @Contended
+        Object o13;
+
+        @Contended
+        Object o14;
+   }
+
+}
+