RFR(M): 7182260: G1: Fine grain RSet freeing bottleneck
john.cuthbertson at oracle.com
Tue Jul 17 17:55:24 UTC 2012
Thanks for reviewing Thomas' code. All of the points you mention below
are valid and I can make those changes on Thomas' behalf.
On 07/16/12 13:25, Bengt Rutisson wrote:
> Hi John, Thomas and Johannes,
> I think this looks good. A few minor comments:
> 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 *
> In bulk_free() I would prefer this to be on one line:
> PerRegionTable* res =
> 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
> #endif // _MSC_VER
> It builds fine on Windows without this #ifdef.
> 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:
>> 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.
>> Thomas' testing with OpenDS and my testing with GC test suite,
>> Kitchensink, NSK (jit, gc, regression, and runtime), and jprt.
More information about the hotspot-gc-dev