CRR (re-opened / M): 7075646: G1: fix inconsistencies in the monitoring data

[Bengt Rutisson]

Tony,

On 2011-09-22 22:02, Tony Printezis wrote:
> Hi all,
>
> Latest webrev based on Bengt's changes:
>
> http://cr.openjdk.java.net/~tonyp/7075646/webrev.4/

Looks good. Thanks for addressing my comments so quickly. Ship it! :-)

> Due to popular demand :-) I changed the #define for the undefined max 
> for the G1 memory pools to a static const.

I like it! Thanks!

Finally, a comment regarding the almost empty super classes 
G1MemoryPoolSuper and G1GenerationCounters. I see your point about 
collecting the instance variable for the G1MonitoringSupport instance in 
a super class, but I don't agree that having an extra super class "does 
not cost us anything". It might not cost us any performance, but it does 
cost us in code complexity and speed of figuring call hierarchies out.

Clearly there is no right or wrong here. Just a matter of balancing 
different code quality values against each other. So, I am fine with 
leaving these extra super classes in.

Bengt


>
> Tony
>
> On 09/21/2011 06:34 PM, Tony Printezis wrote:
>> (resending to the correct list this time....)
>>
>> Hi all,
>>
>> I'm re-opening this change for code review:
>>
>> http://cr.openjdk.java.net/~tonyp/7075646/webrev.3/
>>
>> Here's a quick recap of the changes:
>>
>> a) The jstat tool assumes that none of the spaces have 0 capacity. In 
>> G1 the assumption does not always hold (i.e., where no survivor 
>> regions are allocated, the survivor space has 0 capacity). This 
>> causes jstat to display an unexpected character (?) instead of the 
>> correct occupancy percentage of a space. To get round this problem I 
>> now artificially pad all capacities, when reported through jstat, to 
>> ensure that no capacity is 0.
>>
>> b) The jstat counters for the young / old gen capacity were not 
>> updated. Those counters are now updated correctly.
>>
>> Unfortunately, I had to re-architect the way G1 calculates the 
>> various sizes reported by the monitoring frameworks to ensure that we 
>> do minimal work when a new eden region is allocated (e.g., update 
>> exactly the eden occupancy counter, not having to update all the 
>> sizes and counters). The new version calculates all the sizes 
>> synchronously in well-defined places (all sizes: end of GC, end of 
>> hum object allocation, eden occupancy: after an eden region 
>> allocation) and stores them in fields. The consumers of the data only 
>> read the fields when they need the values, i.e., they do not need to 
>> recalculate anything.
>>
>> I also tidied up the GenerationCounters class (there are now two 
>> clear constructors, one for uses with a VirtualSpace, one without) 
>> and the G1 memory pool classes (we had several static wrapper methods 
>> that calculated some of the sizes, but these are not needed any more 
>> as the calculations are now done elsewhere).
>>
>> Tony
>>