RFR: 8057536: Refactor G1 to allow context specific allocations

Bengt Rutisson bengt.rutisson at oracle.com
Fri Sep 5 07:39:24 UTC 2014


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.

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




More information about the hotspot-gc-dev mailing list