RFR(M): 6921087: G1: remove per-GC-thread expansion tables from the fine-grain remembered sets

[Bengt Rutisson]

John and Thomas,

I think we should try to remove dead code and I consider code that has 
been #if-deffed out for several years dead. So, I'm all for removing 
both PRT_COUNT_OCCUPIED and SAMPLE_FOR_EVICTION. All our testing and 
performance measurements have been based on these being enabled. Let's 
remove the unused code for having those flags disabled.

A bit more tricky question is HRRS_VERBOSE. I am not sure why this is a 
#define and not a develop flag. In my opinion we can remove that too 
along with the code it guards since HRRS_VERBOSE is disabled. On the 
other hand this is just logging so it would also be fine to turn 
HRRS_VERBOSE into a develop flag. Personally I prefer to remove the code 
and add it in again later if we see the need for it.

Bengt


On 2012-06-20 20:12, John Cuthbertson wrote:
> Hi Bengt,
>
> A similar question regarding PRT_COUNT_OCCUPIED was asked by Thomas.
>
> It looks like the code under SAMPLE_FOR_EVICTION describe alternative 
> strategies for determining which PerRegionTable to evict from the fine 
> grain entries. If SAMPLE_FOR_EVICTION is defined TRUE then the PRT 
> selected for eviction is chosen from a small subset of the PRTs; 
> otherwise we look at all the PRTs and select the one with most number 
> of entries. Using the non SAMPLE_FOR_EVICTION code will most likely 
> lead to regressions during RSet updating except when _n_fine_entries 
> is less than _fine_eviction_sample_size.
>
> I'm OK with removing the #if-s  and associated dead code - especially 
> since the sample code has been getting exercised for quite a while.
>
> What about PRT_COUNT_OCCUPIED - should we remove those and have the 
> code unconditional too?
>
> Thanks,
>
> JohnC
>
> On 06/20/12 05:03, Bengt Rutisson wrote:
>>
>> Hi John and Thomas,
>>
>> This looks good to me.
>>
>> One minor thing:
>>
>> Not strictly related to this change, but why do we have 
>> SAMPLE_FOR_EVICTION defined as 1? I guess this means that it is 
>> always been on and thus we won't get any performance regressions from 
>> removing the #if SAMPLE_FOR_EVICTION guards. So, why don't we just go 
>> ahead and do that? It would simplify the code a bit.
>>
>> Maybe this should be done as a separate change. It just looks kind of 
>> strange to me.
>>
>> Bengt
>>
>> P.S. Copyright year in heapRegionRemSet.hpp... ;-)
>>
>>
>>
>> On 2012-06-19 21:21, John Cuthbertson wrote:
>>> Hi Everyone,
>>>
>>> Can I have a review for the cleanup changes, supplied by Thomas 
>>> Schatzl (Johannes Kepler University at Linz), for this CR? The 
>>> webrev can be found at: 
>>> http://cr.openjdk.java.net/~johnc/6921087/webrev.0/
>>>
>>> Summary:
>>> Thomas has removed the per thread expansion tables (PosParPRT) and 
>>> associated, unused, expansion and compaction code along with some 
>>> additional unused code. This change significantly reduces the code 
>>> complexity of the fine grained RSet entries. These changes are a 
>>> prerequisite for some other changes that Thomas has been working on 
>>> to optimize the freeing of RSets' fine grained entries.
>>>
>>> I've looked at the changes and they have a thumbs up from me.
>>>
>>> Testing: OpenDS (performed by Thomas), the GC test suite with heap 
>>> verification enabled (me), and jprt (me).
>>>
>>> Thanks,
>>>
>>> JohnC
>>
>