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

John Cuthbertson john.cuthbertson at oracle.com
Wed Feb 20 23:01:04 UTC 2013

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.


More information about the hotspot-gc-dev mailing list