RFR(S): 7143490: G1: Remove HeapRegion::_top_at_conc_mark_count

[Tony Printezis]

John,

Apologies for taking forever to look at this. Thanks for doing this cleanup.

concurrentMark.cpp

1784 class FinalCountDataUpdateClosure: public CMCountDataClosureBase {

Could you please add a comment on where this class is used (there are 
such comments for CMCountDataClosureBase and CalcLiveObjectsClosure, 
it's be good to be consistent)?


1484   // Debugging
1485   size_t _tot_words_done;
1486   size_t _tot_live;
1487   size_t _tot_used;

(Picky) Given that the class has a comment "Used to calculate the # live 
objects per regionfor verification purposes" does the "Debugging" 
comment above still apply (i.e., isn't it redundant)?

Also, related to this, compare (from CalcLiveObjectsClosure)

1484   // Debugging
1485   size_t _tot_words_done;
1486   size_t _tot_live;
1487   size_t _tot_used;

with (from FinalCountDataUpdateClosure)

1785   size_t _total_live_bytes;
1786   size_t _total_used_bytes;
1787   size_t _total_words_done;

Do you notice any similarities? :-) Maybe you can move those fields to 
the superclass?

1435     // Set the inclusive bit range [start_idx, last_idx].
1436     // For small ranges (up to 8 cards) use a simple loop; otherwise
1437     // use par_at_put_range.
1438     if ((last_idx - start_idx)<= 8) {

Given that last_idx is inclusive:

1439       for (BitMap::idx_t i = start_idx; i<= last_idx; i += 1) {

won't (last_idx - start_idx) <= 8 actually return true for up to 9 cards 
(instead of 8 that it says on the comment)? Maybe change it to < 8? This 
is really not important, but it's again good to make sure the comment 
actually reflects what's in the code.

1535       HeapWord* obj_last = start + obj_sz - 1;
...
1547       start = _bm->getNextMarkedWordAddress(obj_last, nextTop);

Does it make sense to replace the second line with:

1547       start = _bm->getNextMarkedWordAddress(obj_last + 1, nextTop);

given that we know that the bit for obj_last will not be marked given 
that it's the last word of an object and definitely not a header?


(on the same code) Please compare the two code fragments:

1535       HeapWord* obj_last = start + obj_sz - 1;
...
1538       BitMap::idx_t last_idx = _cm->card_bitmap_index_for(obj_last);
...
1541       set_card_bitmap_range(start_idx, last_idx);


and

1554       BitMap::idx_t last_idx = _cm->card_bitmap_index_for(top);
...
1556       set_card_bitmap_range(start_idx, last_idx);


This looks kinda fishy to me. In the former code fragment last_idx is 
calculated from an "inclusive" end-of address (obj_last) whereas in the 
latter code fragment is calculated from an "exclusive" end-of address 
(top). Shouldn't line 1554 be:

1554       BitMap::idx_t last_idx = _cm->card_bitmap_index_for(top - 1);


General comment on this: the code might be simpler if 
set_card_bitmap_range() accepted a last_idx that was exclusive?

g1CollectedHeap.cpp

371       gclog_or_tty->print_cr("  [%08x-%08x], t: %08x, P: %08x, N: %08x, "

g1CollectorPolicy.cpp

2591     st->print_cr("  [%08x-%08x], t: %08x, P: %08x, N: %08x, "


Since you're changing those lines, can you add a bit of additional cleanup?

%08x -> PTR_FORMAT so that it's 64-bit-friendly.
[%08x-%08x] -> could you please use the HR_FORMAT style? Note that the 
HR_FORMAT includes top as well as "eden" / "survivor" so you can remove 
that extra information from the print_cr() statement.

On 04/02/2012 07:45 PM, John Cuthbertson wrote:
> Hi Everyone,
>
> Here's my contribution to the cleanup week.
>
> Can I have a couple of volunteers review the changes for this CR? The 
> webrev can be found at: 
> http://cr.openjdk.java.net/~johnc/7143490/webrev.0/
>
> Summary:
> As a result of Tony's recent marking changes (associated with the 
> marking of survivors) the field HeapRegion::_top_at_conc_mark_count is 
> no longer required. The value of _top_at_conc_mark_count is now the 
> same as NTAMS. Additionally I simplified the verification code that 
> sets the bits in the live card bitmap and refactored the closures that 
> finalize and verify the liveness counting data to avoid code duplication.
>
> Testing: I inserted a 500ms sleep in the marking cycle between the 
> remark and cleanup pauses to ensure that at least one evacuation pause 
> was seen during this phase, and ran the GC test suite with several 
> marking thresholds (2, 5, and 10%) and verification enabled.
>
> Thanks,
>
> JohnC