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

[Tony Printezis]

Bengt,

On 9/22/2011 4:57 PM, Bengt Rutisson wrote:
> Looks good. Thanks for addressing my comments so quickly. Ship it! :-)

And thanks for all the helpful feedback!

>> 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.

Normally, yes. And it is true that some of our class hierarchies are way 
too convoluted and actually counter-productive. But, in this case, the 
extra overhead from that is minimal! Oh, and now there's of course a 
second field in G1MemoryPoolSuper. ;-)

> 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.

OK.

Tony
>
>>
>> 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
>>>
>