RFR: 8064721: The card tables only ever need two covering regions

[Erik Helin]

All,

new webrevs available at:
- full:
   http://cr.openjdk.java.net/~ehelin/8064721/webrev.01/
- incremental:
   http://cr.openjdk.java.net/~ehelin/8064721/webrev.00-01/

See comments inline.

On 2014-11-13 20:33, Kim Barrett wrote:
> On Nov 13, 2014, at 11:01 AM, Erik Helin <erik.helin at oracle.com> wrote:
>>
>> On 2014-11-12 22:30, Kim Barrett wrote:
>>> src/share/vm/memory/barrierSet.hpp
>>>    52   // Some barrier sets create tables whose elements correspond to parts of
>>>    53   // the heap; the CardTableModRefBS is an example.  Such barrier sets will
>>>    54   // normally reserve space for such tables, and commit parts of the table
>>>    55   // "covering" parts of the heap that are committed. At most one covered
>>>    56   // region per generation is needed.
>>>    57   static const int _max_covered_regions = 2;
>>>
>>> Is this (now) constant slated for later elimination?
>>>
>> Sorry, I don't really follow, why would we remove this constant? Do
>> you mean we should "hard code" 2 at all places where
>> _max_covered_regions is used in CardTableModRefBS? Or that we should
>> just move the constant down the inheritance chain to CardTableModRefBS
>> where it really belongs?
>
> There's the location question, of course.  But there is also the
> question of whether this is any different from the maximum number of
> generations, e.g. use that constant rather than a separate one, or
> derived from that constant.  (Can't depend on maximum number of
> generations right now, of course, since the cleanup and simplification
> of number of generations is still in progress.)

Ah ok, now I see. Yes, once the generations cleanup has been pushed, we 
should use GenCollectedHeap::_max_gens.

>>> src/share/vm/memory/cardTableRS.hpp
>>>    86   // Number of generations (including permgen).
>>>    87   static const int _regions_to_iterate = 3;
>>> [...]
>>>
>> // The perm gen is index 0; other gens are at
>> // their level plus 1.
>> [...]
>> Do you think I should rephrase this comment?
>> [...]
>> As I mentioned above, if someone gets rid of all the PermGen legacy in
>> CardTableRS, we can probably use 2 as the value.
>
> I'd missed that perm gen was index 0 for this.  So 3 it is, at least
> for now. And yes, I think the comment needs more explanation;
> something along the lines of
>
>    Number of generations, plus one for lingering PermGen issues in
>    CardTableRS.
>
> Otherwise someone is likely to come along later, see "permgen",
> think they can subtract one and update the comment, and forget to do
> adequate testing...

Agree, I've updated the patch.

> And perhaps someday this too would be replaced by direct reference to
> a maximum number of generations constant.
>
> And there needs to be a new bug report to clean up the lingering
> permgen stuff in CardTableRS.

Agree, I've filed JDK-8064876 - "Remove all remnants of PermGen in 
CardTableRS".

>>> src/share/vm/memory/modRefBarrierSet.hpp
>>>    40   ModRefBarrierSet() : BarrierSet() { _kind = BarrierSet::ModRef; }
>>>
>>> Not sure why this change was made; the default construction of
>>> BarrierSet was already implicit.  Not that I object to the change,
>>> just surprised by it.
>
>> I didn't completely remember when in C++ a parent constructor was
>> automatically called, so I decided to add a call to BarrierSet for the
>> default constructor. If it is implicit then I can revert this part of
>> the patch if you like (although I prefer the the explicit variant).
>
> If there's no explicit constructor call for a base class, a call to
> its default constructor gets implicitly inserted at the appropriate
> point in the member initializer list.
>
> I don't have a strong preference either way for explicit vs implicit
> calls to base class default constructors.  In this case I probably
> would have left it implicit, but have no objection to explicit.  But
> if there needed to be explicit calls to other base class constructors
> (due to multiple inheritance) then I'd probably make them all
> explicit.
>
> But the initialization of _kind really should be in the member
> initialization list, and since you are touching the code anyway…

_kind is actually a member of BarrierSet, not ModRefBarrierSet, so it 
can't be set in member initialization list of a ModRefBarrierSet 
constructor. The proper way to clean this up would be to have the 
BarrierSet constructor take a parameter of type BarrierSet::Name to set 
_kind. In order to keep this patch focused on only cleaning up 
max_covered_regions, I reverted the patch to use the implicit call to 
the BarrierSet constructor in modRefBarrierSet.cpp.

Thanks,
Erik

>>> src/share/vm/runtime/vmStructs.cpp
>>> removal of
>>>   476   nonstatic_field(BarrierSet,                  _max_covered_regions,                          int)                                   \
>>>
>>> [...]
>>>
>> The entries in vmStructs are not supported …
>
> OK, so no issue here.
>
>