RFR (M): 8035406: Improve data structure for Code Cache remembered sets

[Mikael Gerdin]

Hi,

On Tuesday 25 February 2014 10.42.35 Thomas Schatzl wrote:
> Hi,
> 
>   thanks for the review...
> 
> On Mon, 2014-02-24 at 16:57 +0100, Mikael Gerdin wrote:
> > Thomas,
> > 
> > On Monday 24 February 2014 12.40.10 Thomas Schatzl wrote:
> > > Hi all,
> > > 
> > >   can I have reviews for this split-out from 8027295? It changes the
> > > 
> > > code cache remembered set data structure from a GrowableArray to a set
> > > of chunks of nmethods.
> > > 
> > > 
> > > Bug CR:
> > > https://bugs.openjdk.java.net/browse/JDK-8035406
> > > 
> > > Webrev:
> > > http://cr.openjdk.java.net/~tschatzl/8035406/webrev/
> >  
> >  845 HeapRegionRemSet::HeapRegionRemSet(G1BlockOffsetSharedArray* bosa,
> >  846                                    HeapRegion* hr)
> >  847   : _bosa(bosa), _m(Mutex::leaf, "An OtherRegionsTable lock", true),
> >  848     _code_roots(), _other_regions(hr, &_m) {
> >  849   reset_for_par_iteration();
> >  850 }
> > 
> > Since the mutex is no longer only covering the OtherRegionsTable, should
> > it be renamed to "HeapRegionRemSet lock"?
> > Also, it appears that the Mutex constructor actually copies the name
> > string
> > into the Mutex object, so we can format the string with whatever contents
> > we want, such as "HeapRegionRemSet lock for region #4711" without
> > increasing footprint.
> 
> Fixed. That required some minor changes to other code though that
> manages the mutex.

Ok, I was thinking something more along the lines of:

_m(Mutex::leaf, FormatBuffer("HeapRegionRemSet lock #"UINT32_FORMAT, hr-
>hrs_index()), true),

FormatBuffer is the same class being used by err_msg used in asserts and has a 
char* conversion operator, it encapsulates the char[] and snprintf call so you 
don't need to worry about it.

> 
> >   34 class G1CodeRootChunk : public CHeapObj<mtGC> {
> >   35 private:
> >   36   static const int num_entries = 32;
> > 
> >   37 public:
> > The private/public declarations are incorrectly indented.
> 
> A small survey through the code reveals:
> 
> 332:43 in the g1 directory for no space/one space
> 430:467 across the gc_implementation directory and
> 3383:1861 across the VM
> 
> (numbers are approximate)
> 
> I added the space. I do not really care although my IDE prefers to use
> zero spaces.

In that case I don't really care either way, I thought it was more consistent.

> 
> >   49   nmethod** top() const {
> >   50     return (nmethod**) &(_data[num_entries]);
> >   51   }
> > 
> > Top usually denotes the "current allocation point" in the rest of the GC
> > code, I'm fine with you calling that variable _alloc_next instead but
> > please rename this to end() to be consistent with the terminology in the
> > rest of the code.
> I changed the terminology to bottom()/top()/end() as in the other places
> (I think).

Thanks

/Mikael

> 
> >  133 class G1CodeRootSet {
> > 
> > Add VALUE_OBJ_CLASS_SPEC to inherit ValueObj.
> 
> Done
> 
> >  174
> >  175  void nmethods_do(CodeBlobClosure* blk) const;
> >  176
> > 
> > The indent is just one space here.
> 
> Fixed
> 
> >  288   experimental(uintx, G1CodeRootsChunkCacheKeepRatio, 10,
> >  289           "The amount of code root chunks that should be kept at most
> >  "
> >  290           "as percentage of already allocated.")
> > 
> > Shouldn't this be Percent instead of Ratio if it's a percent value? I know
> > we've other flags which are incorrectly named but "the real" ratios such
> > as
> > NewRatio and SurvivorRatio are not percent values at all.
> 
> Fixed
> 
> New webrev at
> http://cr.openjdk.java.net/~tschatzl/8035406/webrev.1/
> 
> Testing:
> jprt
> 
> Thanks,
>   Thomas