RFR(M): 7182260: G1: Fine grain RSet freeing bottleneck
bengt.rutisson at oracle.com
Wed Jul 18 08:11:03 UTC 2012
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
> 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