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

[Kim Barrett]

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.)

>> 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...

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.

>> 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…

>> 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.