RFR: 8057536: Refactor G1 to allow context specific allocations
Bengt Rutisson
bengt.rutisson at oracle.com
Fri Sep 5 08:01:25 UTC 2014
On 2014-09-05 10:02, Stefan Johansson wrote:
> 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/
Looks good.
Bengt
>
> 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
>>>>
>>>
>>
>
More information about the hotspot-gc-dev
mailing list