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

[Tony Printezis]

Bengt,

Many thanks for looking at the webrev! Please see inline for answers to 
your comments.

On 09/22/2011 07:51 AM, Bengt Rutisson wrote:
>
> Hi Tony,
>
> I think it looks good and the updates of the values are much more 
> structured this way. Thanks for cleaning this up.
>
> Some comments below. Mostly code issues.
>
> There is one more functional issue that bothers me. It is the 
> pad_capacity() solution. I don't have a better solution in mind if we 
> are to solve this within the VM, but I think it is (like you put it) 
> very unfortunate that it is needed. Would it be possible to try to get 
> jstat to handle 0 capacities properly instead?

I'm not really proud of it, but I'd rather keep the fixes local to G1 
for the moment than requiring jstat to be fixed (even though, jstat 
should really handle the 0 size case; also note that users might end up 
using an older jstat without the appropriate fixes to monitor the latest 
JVM). But also let me defend the change a bit: it's localized to the 
capacity values reported by the jstat counters and, given that jstat 
(the tool) reports everything in Ks, the values reported are actually 
correct.

> g1MemoryPool.hpp
>
> The last sentence of this comment is a little off now that the 
> calculations have moved.
>
> // This class is shared by the three G1 memory pool classes
> // (G1EdenPool, G1SurvivorPool, G1OldGenPool). Given that the way we
> // calculate used / committed bytes for these three pools is related
> // (see comment above), we put the calculations in this class so that
> // we can easily share them among the subclasses.
> class G1MemoryPoolSuper : public CollectedMemoryPool {

Indeed, I removed it.

> Why is this a method and not a static const or a #define?
>
>   static size_t undefined_max() {
>     return (size_t) -1;
>   }

I suppose when I had originally implemented G1MemoryPoolSuper all the 
sizes reported by the subclasses were accessed by methods on the 
superclass. So having a method for the undefined max too was consistent 
with that approach. I changed the code to use:

#define G1_MEMORY_POOL_UNDEFINED_MAX ((size_t) -1)


> Can we remove G1MemoryPoolSuper? It does not do much. Assuming 
> undefinied_max() is instead "#define UNDEFINED_MAX (size_t)-1" it 
> would make the G1EdenPool class look like this:
>
> class G1EdenPool : public CollectedMemoryPool {
>   G1MonitoringSupport* _g1mm;
> public:
>   G1EdenPool(G1CollectedHeap* g1h) :
>   _g1mm(g1h->g1mm()), CollectedMemoryPool("G1 Eden Space",
>                                           MemoryPool::Heap,
>                                           
> g1h->g1mm()->eden_space_committed(),
>                                           UNDEFINED_MAX,
>                                           false) { }
>
>   size_t used_in_bytes() {
>     return _g1mm->eden_space_used();
>   }
>   size_t max_size() const {
>     return UNDEFINED_MAX;
>   }
>   MemoryUsage get_memory_usage();
> };
>
> I realize that G1SurvivorPool and G1OldGenPool will be very similar. 
> It is about the same amount of code duplication as there is now, but 
> we would get rid of one level of inheritance.

It is true that, given the latest changes, we don't do much in the super 
class. However, I'm inclined to leave it in. It does have the 
functionality of locally storing the G1MonitoringSupport object which 
can be used in the subclasses and it's nice not to have to replicate 
that three times. Also, we might be able to take advantage of having the 
super class in the future in case we need to share some code among the 
subclasses. If it was empty, I'd be OK removing it. But it does cut down 
a bit of replication and having it doesn't really cost us anything.

> g1MonitoringSupport.hpp
>
> - min capacity. Could we not use 1 region for the Eden memory pool? 
> Not a big deal, but I think we will never have a smaller eden.

Yeah, and we could use the min heap size as the min capacity of the old 
pool. I don't think it really matters though. The min capacity is only 
reported through jstat IIRC. And it might confuse folks if we have 1 
region for eden and 0 for survivors. I think I'll leave it as is to be 
consistent among the three pools. BTW, there's already an open CR to 
look at a min / max capacity for the old pool that makes more sense.

> - Move the declaration of G1GenerationCounters down to end up after 
> G1MonitoringSupport? Would avoid the extra forward declaration on line 
> 117: class G1MonitoringSupport;

Done.

Incidentally, I also found this declaration but I can't find a 
corresponding class. ;-) I removed it too.

class G1SpaceMonitoringSupport;

> - Similar to the discussion about G1MemoryPoolSuper above. Can we 
> remove the G1GenerationCounters super class? It does not do much. That 
> would make G1YoungGenerationCounters the look like this:
>
> G1YoungGenerationCounters::G1YoungGenerationCounters(G1MonitoringSupport* 
> g1mm,
>                                                      const char* name)
>   : GenerationCounters(), _g1mm(g1mm) {
>   initialize(name, 0, 3,
>              G1MonitoringSupport::pad_capacity(0, 3),
>              G1MonitoringSupport::pad_capacity(g1mm->young_gen_max()),
>              G1MonitoringSupport::pad_capacity(0, 3));
>   update_all();
> }
>
> Again, G1OldGenerationCounters would be very similar. But it is about 
> the same amount of code duplication as there is now.

See my comments above which also apply here re: having a shared way in 
storing the G1MonitoringSupport object and the extra class not costing 
us anything.

> - If we don't want to remove G1GenerationCounters. Can we move the 
> update_all() call into the G1GenerationCounters constructor. Then the 
> constructors in the subclasses don't have to call it.
>

We want to call update_all() after the subclass constructor has been 
executed. If we call update_all() in the superclass constructor, the 
above is not guaranteed. In fact, I tried this and it indeed does not work.

> g1MonitoringSupport.cpp
>
> - why is _non_young_collection_counters not called 
> _old_collection_counters?

This is what we had before so I had left it in... I changed non_young to 
old.

> - recalculate_eden_size() why is this necessary:
>     // Somewhat defensive: cap the eden used size to make sure it
>     // never exceeds the committed size.
>     _eden_used = MIN2(_eden_used, _eden_committed);

For the survivor and old pool, I could easily prove that used <= 
committed. For the eden pool I was not 100% sure, given the inherent 
inaccuracy in the way we do the used / committed calculations. So, I 
decided to be defensive instead of adding an assert. FWIW, after we push 
the changeset that introduces the old region set that keeps track of all 
the allocated old regions, this calculations will become much saner. 
We'll have to wait for a few weeks for that though...

> generationCounters.hpp
>
> - Why expose the initialize() method? Why not just change the 
> protected default constructor to take the parameters that initialize() 
> takes? initialize() is always called right after the call to the 
> default constructor. I guess you want to share the implementation with 
> the constuctor that takes a VirtualSpace, but both these consturctors 
> could call initialize() internally.

I thought it was a bit pointless creating a constructor which just 
passes the values to initialize(). But, OK, you're right: it's nice to 
have all values initialized in the constructors. I added the protected 
constructor you mentioned and made initialize() private. BTW, the 
GenerationCounters class is a bit awkward given that it's trying to do 
two things: either work with a VirtualSpace or without. Ideally, we'd 
separate the former into a separate subclass. FWIW.

Tony

> On 2011-09-22 00:34, 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
>