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

[Bengt Rutisson]

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?

Bengt


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 {



Why is this a method and not a static const or a #define?

   static size_t undefined_max() {
     return (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.

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.
- Move the declaration of G1GenerationCounters down to end up after 
G1MonitoringSupport? Would avoid the extra forward declaration on line 
117: class G1MonitoringSupport;

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


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



g1MonitoringSupport.cpp

- why is _non_young_collection_counters not called _old_collection_counters?
- 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);


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.








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