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

[Tony Printezis]

PS Is anyone else looking at this?

On 9/23/2011 11:19 AM, Tony Printezis wrote:
> 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
>>>>
>>