RFR(S): 8011724: G1: Stack allocate instances of HeapRegionRemSetIterator

[Bengt Rutisson]

John, looks good!

Bengt

On 4/9/13 7:13 PM, John Cuthbertson wrote:
> Hi Bengt,
>
> Thanks for looking over the changes. As said - I was adding the 
> instrumentation code to the iterator and this looked odd.
>
> On 4/8/2013 11:05 PM, Bengt Rutisson wrote:
>>
>> Hi John,
>>
>> Thanks for doing this cleanup! It really looks much better!
>
> I expect as I look at the root scanning and RSet updating code more 
> minor cleanups/refactoring will become apparent.
>
>>
>> Changes look good.
>>
>> One minor request if you feel like it:
>>
>> Since you anyway updated a comment in the class 
>> HeapRegionRemSetIterator. Could you also update (or just remove) this 
>> comment?
>>
>>   // If true we're iterating over the coarse table; if false the fine
>>   // table.
>>   enum IterState {
>>     Sparse,
>>     Fine,
>>     Coarse
>>   };
>
> Sure. No problem. How about:
>
> "Indicates the granularity of table that we're currently iterating over"?
>
>>
>> Also, have you measured any performance impact of this? I'm not 
>> suggesting that you should put any extra work into doing that. Just, 
>> in case you have some numbers, have you seen any improvements? I 
>> could imagine that there was a fair amount of false sharing going on 
>> with the old array of iterators, so I'm guess that we might see some 
>> perf improvements with this change. Just curious.
>
> I only did a cursory (i.e. untuned) measurement using SPECjvm98 (5x 
> before and after) and didn't see any meaningful difference. The 
> variance between the results was slightly smaller but I wouldn't draw 
> any conclusion from that. This was on a system with 4 GC threads.
>
> Thanks again,
>
> JohnC