RFR (S): 8145672: Remove dependency of G1FromCardCache to HeapRegionRemSet

[Jon Masamitsu]

Thomas,

A couple of questions about code that are not part of your change (you
can ignore) and a suggestion on expanding a comment.

http://cr.openjdk.java.net/~tschatzl/8145672/webrev/src/share/vm/gc/g1/g1CollectedHeap.cpp.frames.html

Not a part of your change but is the guarantee() in the right place?

> 1936   const uint max_region_idx = (1U << (sizeof(RegionIdx_t)*BitsPerByte-1)) - 1;
> 1937   guarantee((max_regions() - 1) <= max_region_idx, "too many regions");
> 1938
> 1939 G1RemSet::init_heap(max_regions());
Meaning, isn't _max_regions initialized in init_heap() after the 
guarantee()?

http://cr.openjdk.java.net/~tschatzl/8145672/webrev/src/share/vm/gc/g1/g1FromCardCache.cpp.frames.html

Again, not part of your change but can lines  52 and 53 be lifted out of 
the loop at 51?

> 51 for (uint i = 0; i < G1RemSet::num_par_rem_sets(); i++) {
>    52     uint end_idx = (start_idx + (uint)new_num_regions);
>    53     assert(end_idx <= _max_regions, "Must be within max.");
>    54     for (uint j = start_idx; j < end_idx; j++) {
>    55       set(i, j, InvalidCard);
>    56     }
>    57   }

http://cr.openjdk.java.net/~tschatzl/8145672/webrev/src/share/vm/gc/g1/g1RemSet.hpp.frames.html

Thank you for adding this comment.

> 79 // Gives an approximation on how many threads add records to a 
> remembered set
> 80 // in parallel for sizing buffers to decrease performance losses 
> due to sharing.
> 81 // Examples are mutator threads together with the concurrent 
> refinement threads
> 82 // or GC threads.
> 83 static uint num_par_rem_sets();

Can you add what kind of buffers to "for sizing ??? buffers"?

Would this be too specific to the current implementation?  Meaning do 
you want a more
general descriptions?

// Examples are the maximum number of mutator threads plus the maximum 
number of
// concurrent refinement threads or the maximum number of GC threads, 
whichever is
// larger.

Leave out the "maximum" (in all 3 places if it is understood).

Otherwise, looks good.

None of these are critical to the patch (since mostly not part of
your change) so no need to delay your push for any of these.

Jon


On 12/18/2015 3:02 AM, Thomas Schatzl wrote:
> Hi all,
>
>    can I have review for the following change that removes the remaining
> dependency of G1FromCardCache to HeapRegionRemSet?
>
> G1FromCardCache references some static method that returns some
> configuration value for G1FromCardCache. Imo it is better to move that
> into G1RemSet as it is some value that is used for configuring parts of
> the entire remembered set.
>
> I.e. G1FromCardCache is actually pretty independent of HeapRegionRemSet.
>
> I also removed some unnecessary references to (former)
> HeapRegionRemSet::num_par_rem_sets() across the code.
>
> This allows easier hg rm heapRegionRemset* later, or at least adds some
> explanation for the methods :)
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8145672
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8145672/webrev
> Testing:
> jprt
>
> Thanks,
>    Thomas
>
>