RFR(M): 7163191: G1: introduce a "heap spanning table" abstraction

[Bengt Rutisson]

Hi John,

Thanks for putting this together! I definitely think it is a good idea 
to break some of the logic out of HeapRegionSeq!

I have a couple of minor things that I would like to clean up. I figured 
the easiest way was to send you a patch. Please try out the attached 
patch and see what you think. It should be applied on top of the patch 
from your webrev.

I also have a couple of design questions:

The G1HeapSpanningTableBase::DefaultValue has this comment:

   // Currently, in all the uses of this class the default value of
   // array's elements is 0. If this changes in the future we can
   // allow the subclasses to parametarize it.

I don't think this will work unless we make the DefaultValue of type T 
and thus move it into G1HeapSpanningTable. The DefalutValue now is 0 
which works fine. But when we memset we will write DefalutValue which I 
guess is int in the enum case (and char in my patch). But when we read 
we do:

array[index]

where array is T* array, so on 64 bit we probably read 64 bits but we 
only write 32 or less with DefaultValue. So, setting it to anything else 
than 0 probably won't work.



How do you feel about changing the relationship between HeapRegionSeq 
and G1HeapSpanningTable a bit? Right now HeapRegionSeq inherits from 
G1HeapSpanningTable but I kind of think that makes the distinction 
between HeapRegionSeq and G1HeapSpanningTable hard to find. I think it 
would be clearer if HeapRegionSeq just had an instance of 
G1HeapSpanningTable and delegated to it.

That way HeapRegionSeq can be more focused on its responsibilities and 
G1HeapSpanningTable can be more self contained.


Another thing, which is not only related to this change, is the use of 
initilize methods. It's a bit strange to me that we use empty 
constructors and then have to remember to call the initilize methods in 
the right order. I think it would be good to break the initialize-chain 
somewhere and go over to using constructors instead. If HeapRegionSeq 
would not inherit from G1HeapSpanningTable we could set the 
G1HeapSpanningTable instance in HeapRegionSeq up in its intialize 
method. That would allow us to use the constructor instead of another 
level of initialize methods for G1HeapSpanningTable.


Finally, there is a comment about why we have the division between 
G1HeapSpanningTableBase and G1HeapSpanningTable:

// ... The reason for the
// split is that we found that compilers like all the methods of a
// parametarized class to be in the .hpp file and this was the only
// way to put some of the methods in the .cpp file.

This is a downside of templates but the .cpp file now only contains 4 
relevant methods and they are not very large. Does this still motivate 
the extra complexity introduced by the split into two classes? Currently 
we also only have one instantation of G1HeapSpanningTable. Will there be 
more of other types than HeapRegion* or could we just drop the templates?

Thanks,
Bengt

On 2/14/13 11:16 PM, John Cuthbertson wrote:
> Hi Everyone,
>
> Here's another change from Tony that I've been maintaining since he 
> left Oracle. Can I have a volunteer look over the changes? They look 
> good to me. The webrev can be found at: 
> http://cr.openjdk.java.net/~johnc/7163191/webrev.0/
>
> Summary:
> Currently in G1 there are several tables where a each element is used 
> to hold some information about a fixed size subdivision of the heap. 
> For example the HeapRegion table and the fast CSet test tables. This 
> change encapsulates the base properties of these tables in a generic 
> heap spanning table and uses the abstraction for the heap region 
> sequence table. A future change will extend this to the fast cset test 
> arrays.
>
> Testing consisted of the GC test suite with a low marking threshold 
> and heap verification. Testing did catch an assertion failure that I 
> have included a small fix for. As with the additional marking 
> validation changes, if no-one objects, I will push this change with 
> Tony as the author and myself as a reviewer.
>
> JohnC

-------------- next part --------------
diff --git a/src/share/vm/gc_implementation/g1/g1HeapSpanningTable.cpp b/src/share/vm/gc_implementation/g1/g1HeapSpanningTable.cpp
--- a/src/share/vm/gc_implementation/g1/g1HeapSpanningTable.cpp
+++ b/src/share/vm/gc_implementation/g1/g1HeapSpanningTable.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012, Oracle and/or its affiliates. All rights reserved.
+ * 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
@@ -30,16 +30,6 @@
   memset(array, DefaultValue, array_size_bytes());
 }
 
-bool G1HeapSpanningTableBase::check_cleared(void* array) {
-  char* carray = (char*) array;
-  for (size_t i = 0; i < array_size_bytes(); i += 1) {
-    if (carray[i] != (char) DefaultValue) {
-      return false;
-    }
-  }
-  return true;
-}
-
 void* G1HeapSpanningTableBase::create_new_array_base() {
   void* array = (void*) NEW_C_HEAP_ARRAY(char, array_size_bytes(), mtGC);
   clear(array);
diff --git a/src/share/vm/gc_implementation/g1/g1HeapSpanningTable.hpp b/src/share/vm/gc_implementation/g1/g1HeapSpanningTable.hpp
--- a/src/share/vm/gc_implementation/g1/g1HeapSpanningTable.hpp
+++ b/src/share/vm/gc_implementation/g1/g1HeapSpanningTable.hpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012, Oracle and/or its affiliates. All rights reserved.
+ * 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
@@ -48,12 +48,10 @@
 
 class G1HeapSpanningTableBase VALUE_OBJ_CLASS_SPEC {
 protected:
-  enum {
     // Currently, in all the uses of this class the default value of
     // array's elements is 0. If this changes in the future we can
     // allow the subclasses to parametarize it.
-    DefaultValue = 0
-  };
+  static const char DefaultValue = 0;
 
   // The address of the first reserved word in the heap.
   HeapWord* _heap_bottom;
@@ -98,10 +96,6 @@
   // Set all elements of the given array to the default value.
   void clear(void* array);
 
-  // Returns true if all the entries of the given array are the default
-  // value, false otherwise.
-  bool check_cleared(void* array);
-
   // Allocate, clear, and return a new array.
   void* create_new_array_base();
 
@@ -173,17 +167,6 @@
   }
 
   // Return the element of the given array that corresponds to the
-  // given address. The array should be the biased version. If the
-  // address is not valid, return the default value.
-  T get(T* array_biased, HeapWord* addr) const {
-    if (in_g1_heap(addr)) {
-      return get_unsafe(array_biased, addr);
-    } else {
-      return (T) DefaultValue;
-    }
-  }
-
-  // Return the element of the given array that corresponds to the
   // given address. The array should be the biased version. Assume
   // that the address is valid.
   T get_unsafe(T* array_biased, HeapWord* addr) const {
diff --git a/src/share/vm/gc_implementation/g1/heapRegionSeq.inline.hpp b/src/share/vm/gc_implementation/g1/heapRegionSeq.inline.hpp
--- a/src/share/vm/gc_implementation/g1/heapRegionSeq.inline.hpp
+++ b/src/share/vm/gc_implementation/g1/heapRegionSeq.inline.hpp
@@ -36,13 +36,12 @@
 }
 
 inline HeapRegion* HeapRegionSeq::at(HeapWord* addr) const {
-  HeapRegion* hr = get(_regions_biased, addr);
   if (in_g1_heap(addr)) {
+    HeapRegion* hr = get_unsafe(_regions_biased, addr);
     assert(hr != NULL, "if the address is valid, hr should not be NULL");
   } else {
-    assert(hr == NULL, "if the address is not valid, hr should be NULL");
+    return NULL;
   }
-  return hr;
 }
 
 inline HeapRegion* HeapRegionSeq::at_unsafe(HeapWord* addr) const {