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

[Erik Helin]

On 2014-11-12 22:30, Kim Barrett wrote:
> 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.

Thanks for taking your time to review!

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

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

So, CardTableRS is used by the framework collectors (and G1) and is set 
up via GenCollectedHeap::initialize which calls 
collector_policy()->create_rem_set(...) which creates a new CardTableRS. 
The framework collectors (CMS, ParNew, DefNew, Serial) passed 4 as 
max_covered_regions, G1 passes 2. This is why _regions_to_iterate must 
be max(4-1, 2-1) = 3.

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

Yes, 2 should be enough, but as you correctly suspect, CardTableRS was 
not updated when PermGen was removed. The comment above the declaration 
of _last_cur_val_in_gen reads:

   // An array that contains, for each generation, the card table value last
   // used as the current value for a younger_refs_do iteration of that
   // portion of the table.  (The perm gen is index 0; other gens are at
   // their level plus 1.  They youngest gen is in the table, but will
   // always have the value "clean_card".)
   jbyte* _last_cur_val_in_gen;

So CMS (which will get index 2) needs to access index 2 in this array, 
therefore _regions_to_iterate must be 3 (permgen + young gen + old gen). 
If we remove all traces of PermGen in CardTableRS, we can probably use 
2, but thas is another change.

This is why I tried to add the comment, to help someone understand why 
we use the number 3 for _regions_to_iterate.

   // Number of generations (including permgen).
   static const int _regions_to_iterate = 3;

Do you think I should rephrase this comment?

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

The +1 here is because PermGen gets index 0, so every generation in 
GenCollectedHeap will get their generation index + 1. I don't think 
GenCollectedHeap::max_gens included perm gen in the count, PermGen was 
never part of GenCollectedHeap::_gens (I believe it was created separately).

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

Since I didn't trust my reasoning that I outlined in the review request 
email, I run a lot of tests :) The JVM crashes for values 1 and 2, so we 
better use 3. As I mentioned above, if someone gets rid of all the 
PermGen legacy in CardTableRS, we can probably use 2 as the value.

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

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 any change is going to be made here, _kind should be initialized
> via a member initializer rather than assignment in the constructor
> body.

Yes, if you agree that we should keep the explicit constructor call to 
BarrierSet then I will definitely update the patch to use _kind in the 
initialization list.

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

The entries in vmStructs are not supported, the only supported user of 
contents in vmStrcuts is the SA agent (hotspot/agent). We do not need a 
CCC request for changing vmStructs, as long the SA agent works, any 
change is fine. I also grepped for any usage of "max_covered_regions" in 
the SA code and didn't find any references (I also run the SA tests and 
they all pass), so the "max_covered_regions" field can safely be removed.

Thanks,
Erik