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

[John Cuthbertson]

Hi Bengt,

Thanks for reviewing Thomas' code. All of the points you mention below 
are valid and I can make those changes on Thomas' behalf.

JohnC

On 07/16/12 13:25, Bengt Rutisson wrote:
>
> 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
>
>