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