RFR(M): 7182260: G1: Fine grain RSet freeing bottleneck

[Bengt Rutisson]

Hi John, Thomas and Johannes,

I think this looks good. A few minor comments:

heapRegionRemSet.cpp

Line 753, we don't normally prefix global functions with "::", so I 
think just "memset" will do.

::memset(_fine_grain_regions, 0, _max_fine_entries * 
sizeof(_fine_grain_regions[0]));


In bulk_free() I would prefer this to be on one line:

       PerRegionTable* res =
         (PerRegionTable*)
         Atomic::cmpxchg_ptr(prt, &_free_list, fl);



Two things that are not strictly related to your change, but that would 
be nice to fix if you feel like it:

Remove this comment? I don't really see what information it adds.

37  // OtherRegionsTable


Remove this #ifdef. I assume there used to be some code between 
"public:" and "protected:", but now it is empty.

#ifdef _MSC_VER
   // For some reason even though the classes are marked as friend they 
are unable
   // to access CardsPerRegion when private/protected. Only the windows 
c++ compiler
   // says this Sun CC and linux gcc don't have a problem with access 
when private

   public:

#endif // _MSC_VER

protected:

It builds fine on Windows without this #ifdef.

Thanks,
Bengt

On 2012-07-07 00:43, John Cuthbertson wrote:
> Hi Everyone,
>
> Can I have a couple of volunteers top review this change which was 
> contributed by Thomas Schatzl at Johannes Kepler University at Linz? 
> The webrev can be found at: 
> http://cr.openjdk.java.net/~johnc/7182260/webrev.0/
>
> Summary:
> While running some experiments with OpenDS, Thomas noticed that RSet 
> freeing was a significant contributor to freeing the collection set 
> after a GC pause. The main culprit was freeing the PerRegionTables 
> that are the entries in the fine grain table - and placing them on a 
> global free list. Thomas' change is to chain the PerRegionTables in an 
> individual RSet together (using a doubly linked list and utilizing the 
> same link field as the free list) so that the fine grain entries could 
> be added to the free list in a single operation. In order to preserve 
> the integrity of the value of the PerRegionTable::_next field, he 
> added a new field for walking the collision chains.
>
> I've already reviewed the changes and after tweaking some of the 
> formatting and comments, they look OK to me.
>
> Testing:
> Thomas' testing with OpenDS and my testing with GC test suite, 
> Kitchensink, NSK (jit, gc, regression, and runtime), and jprt.
>
> Thanks,
>
> JohnC