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

[Mikael Gerdin]

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.
> 
> This has two purposes:
> - make code cache remembered set reclamation fast, i.e. with the current
> implementation instead of malloc/free() just a few pointer operations
> are required that are independent of size.
> 
> - further in the future, these chunks of nmethods can be used as unit of
> distribution across threads to make code root scan less unbalanced
> (JDK-8025813).
> 
> Further implementation details
> 
> - caches a number of free chunks in a global free list to avoid
> malloc/free overhead of freeing chunks (which is a lot smaller too
> because these chunks are evenly sized and small unlike the
> GrowableArrays before)
> 
> - there is a new gc log section that prints code root chunk purge time
> to diagnose future problem with that anyway.
> 
> - fix a problem that code root freeing was not protected by the
> HeapRegionRemSet lock; for that I moved that lock up from
> OtherRegionsTable to HeapRegionRemSet.
> 
> Alternatives like hash tables did not seem to have advantages: contrary,
> they may suffer from the same issues when disposing the hash table array
> itself.

I agree. At some point we should rename the class template FreeList to 
LinkedList since that's what it is, essentially.
Or we could have a FreeList which extends LinkedList with a getter for size.
(You forgot to include the rant about adding yet-another linked list class to 
the VM but I'll say +1 to that here anyway :)

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

  34 class G1CodeRootChunk : public CHeapObj<mtGC> {
  35 private:
  36   static const int num_entries = 32;
  37 public:
The private/public declarations are incorrectly indented.

  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.

 133 class G1CodeRootSet {
Add VALUE_OBJ_CLASS_SPEC to inherit ValueObj.

 174 
 175  void nmethods_do(CodeBlobClosure* blk) const;
 176 
The indent is just one space here.

 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.

/Mikael

> 
> Testing:
> jprt
> 
> Thanks,
>   Thomas