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

[John Cuthbertson]

Hi Bengt,

On 2/20/2013 4:59 AM, Bengt Rutisson wrote:
>
> Hm. I don't find the comment about the HeapRegionSeq::at() bug further 
> down. Did I miss it somehow?

Sorry I didn't include it. It's obvious when looking at your patch:

--- 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;
  }

You didn't return hr in the if-class.

> I think Thomas only answered to you and not the list (he did this 
> before he had a usable Oracle email) so I actually don't know what 
> Thomas suggested. But a virtual default_value() function would 
> probably work. On the other hand if we remove the split between 
> G1HeapSpanningTableBase and G1HeapSpanningTable, as suggested further 
> down, I think we could just have the default value be of the template 
> type.

OK. I didn't realize that Thomas had only sent his comments to me. I 
have removed the split and moved the routines from the .cpp into the 
combined class. I'll define DefaultValue as of the template type.

> Yes, this is what I meant.

OK. Let me think about that and how it plays into fast cset arrays 
patch. I might want to defer this for another CR if it makes merging the 
fast cset arrays patch a pain.

> Great. Thanks!

No problem.

> OK, thanks for this background information. Do you have follow up 
> patches for these changes too? I mean, do we think we will do them in 
> the near future?

Yes I have the patch that extends this to the fast cset test arrays. It 
needs to be brought up to date and, obviously, depends on this change.

JohnC