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

[Kim Barrett]

On Nov 14, 2014, at 8:48 AM, Erik Helin <erik.helin at oracle.com> wrote:
> 
> 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/

New webrev looks good.

> See comments inline.
> 
> On 2014-11-13 20:33, Kim Barrett wrote:
>>>   57   static const int _max_covered_regions = 2;
>> […] But there is also the
>> question of whether this is any different from the maximum number of
>> generations, […]
> 
> Ah ok, now I see. Yes, once the generations cleanup has been pushed, we should use GenCollectedHeap::_max_gens.

Do we need a bug for this?

>>>>   87   static const int _regions_to_iterate = 3;
>>>> […]

>> And yes, I think the comment needs more explanation;
> 
> Agree, I've updated the patch.

Thanks.

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

Thanks.

I added a followup comment with a pointer to _regions_to_iterate as another place to look.

> 
>>>> src/share/vm/memory/modRefBarrierSet.hpp
>>>>   40   ModRefBarrierSet() : BarrierSet() { _kind = BarrierSet::ModRef; }
>> 
>> 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.

Sorry I missed that.  Yes, the proper fix is to change the BarrierSet constructor.  And get rid of the Uninit tag in the BarrierSet::Name enum.  Assuming there is never a genuine need for a default constructed BarrierSet, which seems likely to me.  (ctor and dtor should also be protected.)
https://bugs.openjdk.java.net/browse/JDK-8064947