RFR: 8057536: Refactor G1 to allow context specific allocations

[Stefan Johansson]

Hi Bengt,

On 2014-09-04 17:07, Bengt Rutisson wrote:
>
>
> 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 for the in depth review, here's the new webrev and a diff against 
the first:
http://cr.openjdk.java.net/~sjohanss/8035057/webrev.01/
http://cr.openjdk.java.net/~sjohanss/8057536/webrev.00-01/

I also had to do a small addition in vm_operations.hpp, which was 
forgotten in the previous webrev.

Cheers,
Stefan

> Thanks,
> Bengt
>
>
> G1Allocator.java
>
>   //size_t _summary_bytes_used;
> Should probably not be there.
>
Actually this seems to be the way to show what type this field 
corresponds to in the VM, see G1CollectedHeap.java for other examples.
>
> 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?
>
Fixed.
>
> 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.
Change all to no space since that was most common already.
>
> Would it be possible to move MutatorAllocRegion, SurvivorGCAllocRegion 
> and OldGCAllocRegion into g1AllocRegion.hpp instead?
>
Done.
>
>   // 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.
>
Fixed.
>
>
>     if (hr != NULL)
>       result += hr->used();
>
> Please add {}.
>
>
Fixed.
>
> class G1ParGCAllocBuffer: public ParGCAllocBuffer {
> private:
>   bool        _retired;
>
> Alignment of single instance variable not necessary.
>
>
Fixed.
>
> 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...
>
>
Hope you like the new better.
>
> g1AllocationContext.hpp
>
> #ifndef SHARE_VM_GC_IMPLEMENTATION_G1_G1ALLOCATIONCONTEXT_HPP
>
> Should not be called ...EXT_HPP.
>
But it is, CONTEXT_HPP =) Leaving it as is, for now.
>
> typedef unsigned char AllocationContext_t;
>
> Don't think we normally call types _t.
>
>
Not sure why we/I did this, will open a bug about changing that later, 
and maybe do some more refactoring around it.
>
> g1AllocRegion.cpp
>
> G1AllocRegion::uupdate_alloc_region(). Second call to trace() should 
> probably be trace("updated").
>
Fixed.
> 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).
>
I like this, but I'm not sure what's the best way. I added a system() 
method similar to current(), which returns the system context (could not 
use default).
> 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)".)
>
>
Using AllocationContext::system() for those as well.
>
>   // 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.
>
>
Fixed.
>
> g1CollectedHeap.cpp
>
> I think the newline after this comment should be left as it was:
> // Methods for the GC alloc regions
>
>
Yes, done.
>
> 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.
>
>
Fixed.
>
> g1ParScanThreadState.cpp
>
> G1ParScanThreadState::~G1ParScanThreadState()
> Can we introduce a G1ParGCAllocator::relese_allocator() instead of 
> doing "delete _g1_par_allocator"?
>
That would require to pass the _g1_par_allocator along to release and 
then do the delete in there. I can add this if you prefer but otherwise 
keep it as is.
> 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?
>
I see your point, I shortened it to AC%4u for now since Mikael wanted a 
more compact output. This might be something to do if we re-design the 
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
>