RFR: 8057536: Refactor G1 to allow context specific allocations

[Stefan Johansson]

On 2014-09-05 09:39, Bengt Rutisson wrote:
>
> Hi Stefan,
>
> On 2014-09-04 21:01, Stefan Johansson wrote:
>> 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.
>
> Thanks for fixing all this!
>
> I think it would be good to remove the default values for the 
> parameters to these functions:
>
>
>  454   HeapWord* humongous_obj_allocate_initialize_regions(uint first,
>  455                                                       uint 
> num_regions,
>  456                                                       size_t 
> word_size,
>  457 AllocationContext_t context = AllocationContext::system());
>  458
>  459   // Attempt to allocate a humongous object of the given size. 
> Return
>  460   // NULL if unsuccessful.
>  461   HeapWord* humongous_obj_allocate(size_t word_size, 
> AllocationContext_t context = AllocationContext::system());
>
> The declarations get very long and there is only one place where the 
> default value is actually necessary. Better to explicitly pass 
> AllocationContext::system() there.
>
Agree, here's the diff for just those changes:
http://cr.openjdk.java.net/~sjohanss/8057536/webrev.01-fixes/

Thanks for the review!
Stefan
> Other than that it looks good.
>
> Thanks,
> Bengt
>
>>
>> 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
>>>
>>
>