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

[Thomas Schatzl]

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.

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

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

>  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