RFR: 8057536: Refactor G1 to allow context specific allocations

[Bengt Rutisson]

Hi Stefan,

On 2014-09-04 14:45, Stefan Johansson wrote:
> Hi,
>
> Please review these changes for:
> https://bugs.openjdk.java.net/browse/JDK-8057536
>
> Webrev:
> http://cr.openjdk.java.net/~sjohanss/8057536/webrev.00/

Overall I think this i a nice refactoring. Some comments below. Mostly 
style and code comments.

Thanks,
Bengt


G1Allocator.java

   //size_t _summary_bytes_used;
Should probably not be there.


G1CollectedHeap.java

     public long used() {
         Address g1AllocatorAddr = g1Allocator.getValue(addr);
         G1Allocator allocator =  (G1Allocator) 
VMObjectFactory.newObject(G1Allocator.class, g1AllocatorAddr);
         return allocator.getSummaryBytes();
     }

Maybe introduce a allocator() method similar to the hrm() and g1mm() 
methods?


g1Allocator.hpp

In this file we are mixing non-indented private, protected, public 
declarations with one-space-indented ones. Would be good to keep the 
same style in the file.

Would it be possible to move MutatorAllocRegion, SurvivorGCAllocRegion 
and OldGCAllocRegion into g1AllocRegion.hpp instead?


   // Outside of GC pauses, the number of bytes used in all regions other
   // than the current allocation region.
   size_t           _summary_bytes_used;

I don't think _summary_bytes_used needs to be aligned with the previous 
instance variable since the comment kind of breaks up the alignment anyway.



     if (hr != NULL)
       result += hr->used();

Please add {}.



class G1ParGCAllocBuffer: public ParGCAllocBuffer {
private:
   bool        _retired;

Alignment of single instance variable not necessary.



class G1ParGCAllocator : public CHeapObj<mtGC> {
friend class G1ParScanThreadState;
  protected:
   G1CollectedHeap*    _g1h;

   size_t           _alloc_buffer_waste;
   size_t           _undo_waste;

   void   add_to_alloc_buffer_waste(size_t waste) { _alloc_buffer_waste 
+= waste; }
   void   add_to_undo_waste(size_t waste)         { _undo_waste += waste; }

Alignment...



g1AllocationContext.hpp

#ifndef SHARE_VM_GC_IMPLEMENTATION_G1_G1ALLOCATIONCONTEXT_HPP

Should not be called ...EXT_HPP.


typedef unsigned char AllocationContext_t;

Don't think we normally call types _t.



g1AllocRegion.cpp

G1AllocRegion::uupdate_alloc_region(). Second call to trace() should 
probably be trace("updated").

G1AllocRegion::G1AllocRegion(). I would prefer if there was a special 
value in AllocationContext that could be used as a default value rather 
than using 0 explicitly in the initialization list here. 
_allocation_context(0).


g1CollectedHeap.hpp

Same as above. Calls like AllocationContext_t context = 0 kind of leaks 
what we are trying to hide with the AllocationContext_t type. Would be 
better with an explicit default value in AllocationContext. (Holds for 
this call in g1CollectedHeap.cpp too: r->set_allocation_context(0) and 
in heapRegion.hpp "AllocationContext_t context = 0" and in 
heapRegion.cpp: "set_allocation_context(0)".)



   // Allocation attempt during GC for a survivor object / PLAB.
   inline HeapWord* survivor_attempt_allocation(size_t word_size,
                                   AllocationContext_t context);

   // Allocation attempt during GC for an old object / PLAB.
   inline HeapWord* old_attempt_allocation(size_t word_size,
                              AllocationContext_t context);


Alignment.



g1CollectedHeap.cpp

I think the newline after this comment should be left as it was:
// Methods for the GC alloc regions



g1CollectedHeap.inline.hpp

inline HeapWord* G1CollectedHeap::survivor_attempt_allocation(size_t 
word_size,
                                                  AllocationContext_t 
context) {

inline HeapWord* G1CollectedHeap::old_attempt_allocation(size_t word_size,
                                             AllocationContext_t context) {
Alignment.



g1ParScanThreadState.cpp

G1ParScanThreadState::~G1ParScanThreadState()
Can we introduce a G1ParGCAllocator::relese_allocator() instead of doing 
"delete _g1_par_allocator"?

heapRegion.cpp

void HeapRegion::print_on(outputStream* st) const {
   st->print("Context: %4u", allocation_context());

Is it worth trying to hide the formatting of allocation_context?


>
> Summary:
> These changes are made to allow G1 to do context specific allocation. 
> As part of this a G1Allocator class has be refactored out of 
> G1CollectedHeap which can be extended to implement the specific 
> context code. Currently only the G1DefaultAllocator is implemented, 
> this allocator only makes use of one context to have the same behavior 
> as prior to this enhancement.
>
> Testing:
> * JPRT
> * Manual verification
> * Local ute runs of vm.quick.testlist
>
> Thanks,
> Stefan