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

[Bengt Rutisson]

Hi John,

On 2012-07-17 19:55, John Cuthbertson wrote:
> 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.

:-) Great! The changes are really small but thanks for fixing them.

Overall this is a great improvement! Thanks to both you and Thomas for 
doing it!

Bengt

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