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

[John Cuthbertson]

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