Small issue with array chunking in G1

[Tony Printezis]

Thomas

I attached the diffs (note that our repo is a bit behind yours so the 
change might need to be reworked a bit; but you'll get the idea).

Tony

On 1/31/14, 8:28 AM, Thomas Schatzl wrote:
> Hi Tony,
>
> On Thu, 2014-01-30 at 11:29 -0500, Tony Printezis wrote:
>> (looking at Thomas' recent change reminded me that I've been meaning to
>> ask about this)
>>
>> Talking about array chunking in G1, we noticed a small difference in
>> that code compared to what the other GCs (ParNew / PS) do. G1 uses the
>> to-space length field to encode how much of the array has been scanned
>> (i.e., the from-space length field is always correct). ParNew and PS use
>> the from-space length field (and the to-space length field is always
>> correct). I came across that issue when I was working on a change to
>> periodically scan PLABs (for some profiling stuff I was working on) and
>> in G1 some PLABs were unparseable because the length field of arrays
>> could be incorrect (even if the PLAB is retired, it's possible that the
>> length field is still incorrect given that other threads might still be
>> working on that array).
>>
>> The patch to change G1 to do what ParNew / PS also do is very small (and
>> we've had it for a few months in our repo without any issues). Any
>> chance of convincing you to also take it? I should be clear that the
>> patch doesn't fix a correctness issue in G1; the code is correct as is.
>> But it will bring G1 in line with the other two GCs and allow PLAB
>> scanning during GC, if you want to do that...
> Where is the patch to look at?
>
> Thomas
>
>

-- 
Tony Printezis | JVM/GC Engineer / VM Team | Twitter

@TonyPrintezis
tprintezis at twitter.com

-------------- next part --------------
diff --git a/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp b/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp
index 986384c..ba2f3d0 100644
--- a/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp
+++ b/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp
@@ -4756,9 +4756,9 @@ oop G1ParCopyClosure<do_gen_barrier, barrier, do_mark_object>
 
     if (obj->is_objArray() && arrayOop(obj)->length() >= ParGCArrayScanChunk) {
       // We keep track of the next start index in the length field of
-      // the to-space object. The actual length can be found in the
-      // length field of the from-space object.
-      arrayOop(obj)->set_length(0);
+      // the from-space object. The actual length can be found in the
+      // length field of the to-space object.
+      arrayOop(old)->set_length(0);
       oop* old_p = set_partial_array_mask(old);
       _par_scan_state->push_on_queue(old_p);
     } else {
@@ -4832,16 +4832,16 @@ template <class T> void G1ParScanPartialArrayClosure::do_oop_nv(T* p) {
   assert(Universe::heap()->is_in_reserved(from_obj), "must be in heap.");
   assert(from_obj->is_objArray(), "must be obj array");
   objArrayOop from_obj_array = objArrayOop(from_obj);
-  // The from-space object contains the real length.
-  int length                 = from_obj_array->length();
+  // We keep track of the next start index in the length field of the
+  // from-space object.
+  int next_index             = from_obj_array->length();
 
   assert(from_obj->is_forwarded(), "must be forwarded");
   oop to_obj                 = from_obj->forwardee();
   assert(from_obj != to_obj, "should not be chunking self-forwarded objects");
   objArrayOop to_obj_array   = objArrayOop(to_obj);
-  // We keep track of the next start index in the length field of the
-  // to-space object.
-  int next_index             = to_obj_array->length();
+   // The to-space object contains the real length.
+  int length                 = to_obj_array->length();
   assert(0 <= next_index && next_index < length,
          err_msg("invariant, next index: %d, length: %d", next_index, length));
 
@@ -4851,7 +4851,7 @@ template <class T> void G1ParScanPartialArrayClosure::do_oop_nv(T* p) {
   // We'll try not to push a range that's smaller than ParGCArrayScanChunk.
   if (remainder > 2 * ParGCArrayScanChunk) {
     end = start + ParGCArrayScanChunk;
-    to_obj_array->set_length(end);
+    from_obj_array->set_length(end);
     // Push the remainder before we process the range in case another
     // worker has run out of things to do and can steal it.
     oop* from_obj_p = set_partial_array_mask(from_obj);
@@ -4860,7 +4860,7 @@ template <class T> void G1ParScanPartialArrayClosure::do_oop_nv(T* p) {
     assert(length == end, "sanity");
     // We'll process the final range for this object. Restore the length
     // so that the heap remains parsable in case of evacuation failure.
-    to_obj_array->set_length(end);
+    from_obj_array->set_length(end);
   }
   _scanner.set_region(_g1->heap_region_containing_raw(to_obj));
   // Process indexes [start,end). It will also process the header