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

[Kim Barrett]

On Nov 12, 2014, at 1:57 PM, Erik Helin <erik.helin at oracle.com> wrote:
> 
> this patch removes the max_covering_regions argument from the BarrierSet constructor, since all the code using a BarrierSet only needs two covering regions. Having the maximum number of covering regions as a constant is desirable because it makes it much easier to reason about the code in all the various card tables (CardTableRS, CardTableModRefBS, G1SATBCardTableModRefBS, CardTableExtension).
> 
> […]

> Webrev:
> http://cr.openjdk.java.net/~ehelin/8064721/webrev.00/
> 
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8064721

Thanks for the thorough explanation in the covering email for this
RFR.

------------------------------------------------------------------------------

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?

------------------------------------------------------------------------------

src/share/vm/memory/cardTableRS.hpp
  86   // Number of generations (including permgen).
  87   static const int _regions_to_iterate = 3;

And from the covering email:
  The other variable that was made static const is
  CardTableRS::_regions_to_iterate. This was previously set to
  max_covered_regions - 1 by the framework collectors, so 4 - 1 = 3. I
  also added a comment explaining why it needs to be 3 and not 2.

Sorry, but I'm not finding that comment on line 86 very informative,
especially in light of past removal of permgen.

I don't yet understand why this isn't 1 if it used to be
max_covered_regions - 1 and max_covered_regions is now a constant 2.
But I suspect that value is wrong, based on its usage.

Maybe it should instead be 2 because the now removed increment of
max_covered_regions by 2 was really a workaround for the calculation
here, and needed this to include permgen (e.g. be 3).  (This actually
seems kind of plausible, since the offending increment had a comment
indicating it was a workaround for a needed cardtable fix.)

_regions_to_iterate is used as a bound on the iteration through the
_last_cur_val_in_gen table, whose size is
GenCollectedHeap::max_gens+1.  I'm not sure what the +1 is for; I
wonder if this might be another permgen leftover.

In any case, it's not apparent to me why 3 is the right value now.  I
think 3 will work, but I think there may be further followup work to
be done in this vicinity.  I would be fine with having that be spawned
off as a separate task, rather than being rolled into here; the
analysis of this change set is already complicated enough.

But perhaps there is there a plan to change the value later, or
eliminate this constant altogether?  If so, it would be helpful to
mention that.

------------------------------------------------------------------------------

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.

If any change is going to be made here, _kind should be initialized
via a member initializer rather than assignment in the constructor
body.

------------------------------------------------------------------------------

src/share/vm/runtime/vmStructs.cpp
removal of
 476   nonstatic_field(BarrierSet,                  _max_covered_regions,                          int)                                   \

This is removing an entry that was being made available to Java code.
Are we sure there aren't any users of it?  [I did a grep over a full
JDK tree and found no occurrences of "max_covered_regions" outside
hotspot C++ code, so there don't appear to be any Java uses in the
JDK.  Could there be uses outside that tree?]  It might be safer to
change it to a static_field, rather than removing it entirely.  Hm,
and does removal (or even change?) of this require CCC approval?