RFR (XS): 8155721: Sparse remset wastes half of entry memory

[Thomas Schatzl]

Hi Mikael,

  thanks for your review.

Thomas

On Tue, 2016-05-03 at 11:30 +0200, Mikael Gerdin wrote:
> 
> On 2016-05-03 11:28, Thomas Schatzl wrote:
> > 
> > Hi Mikael,
> > 
> > On Mon, 2016-05-02 at 16:55 +0200, Mikael Gerdin wrote:
> > > 
> > > Hi Thomas,
> > > 
> > > On 2016-05-02 16:03, Thomas Schatzl wrote:
> > > > 
> > > > 
> > > > Hi Mikael,
> > > > 
> > > > On Mon, 2016-05-02 at 13:05 +0200, Mikael Gerdin wrote:
> > > > > 
> > > > > 
> > > > > Hi Thomas,
> > > > > 
> > > > > On 2016-05-02 12:17, Thomas Schatzl wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > Hi all,
> > > > > > 
> > > > > >      can I have reviews for this pretty small change that
> > > > > > (basically)
> > > > > > halves memory usage of the sparse prt?
> > > > > > 
> > > > > > Looking through the code showed that G1 allocates
> > > > > > capacity SparsePRTEntrys (the container for the cards for a
> > > > > > particular
> > > > > > region), but already expands the hash table at
> > > > > > capacity/2+1. So
> > > > > > almost
> > > > > > half of these SparsePRTEntrys are never used and need not
> > > > > > be
> > > > > > allocated/cleared/etc.
> > > > > > 
> > > > > > This also uncovered a off-by-one bug in
> > > > > > RSHashTable::alloc_entry()
> > > > > > that
> > > > > > was benign before but caused crashes when allocating just
> > > > > > what
> > > > > > is
> > > > > > required.
> > > > > > 
> > > > > > CR:
> > > > > > https://bugs.openjdk.java.net/browse/JDK-8155721
> > > > > > Webrev:
> > > > > > http://cr.openjdk.java.net/~tschatzl/8155721/webrev/
> > > > > Good catch,
> > > > > however
> > > > > 
> > > > > I'm not sure I'm fond of keeping the inverse of the occupancy
> > > > > factor.
> > > > > I think the code would be a bit clearer if we made it a
> > > > > floating
> > > > > point
> > > > > number.
> > > > > 
> > > > > Also, I think it should be CapitalizedCamelCase since it's a
> > > > > constant.
> > > > I made it a floating point number, but in return precalculate
> > > > the
> > > > num_entries value to avoid any issues due to rounding etc.
> > > > 
> > > > http://cr.openjdk.java.net/~tschatzl/8155721/webrev.0_to_1
> > > > (diff)
> > > > http://cr.openjdk.java.net/~tschatzl/8155721/webrev.1
> > > If you want to make this change neutral WRT the size of
> > > RSHashTable
> > > you can remove _capacity_mask which appears to always be
> > > (_capacity -
> > > 1) and always accessed through a getter function :)
> > I think that is not worth the effort. There is only one RSHashTable
> > per
> > region remset anyway.
> Ok.
> 
> > 
> > > 
> > > +  bool should_expand() const { return _occupied_entries ==
> > > _num_entries; }
> > > 
> > > Personally I prefer to keep this a ">" instead of having an exact
> > > check.
> > I would like to keep this, while the more imprecise >= would be as
> > fine, I am then always scratching my head why this is the case when
> > looking at it.
> > 
> > The sparse prt is only ever added to under lock.
> > 
> > So I would prefer to keep it this way unless you are really opposed
> > to
> > that.
> Ok.
> 
> /Mikael
> 
> > 
> > 
> > Thanks,
> >    Thomas
> >