RFR(M): 8139867: Change how startsHumongous and continuesHumongous regions work in G1.

David Lindholm david.lindholm at oracle.com
Wed Nov 4 12:01:03 UTC 2015


Thomas,

On 2015-11-04 12:20, Thomas Schatzl wrote:
> Hi,
>
> On Tue, 2015-11-03 at 14:26 +0100, David Lindholm wrote:
>> Hi Thomas,
>>
>> Thanks for looking at this! Comments inline. The new webrevs are
>> available here:
>>
>> Webrev: http://cr.openjdk.java.net/~david/JDK-8139867/webrev.02/
>> Webrev: http://cr.openjdk.java.net/~david/JDK-8139867/webrev.01-02/ (diff)
>>
>> On 2015-11-02 13:23, Thomas Schatzl wrote:
>>> Hi David,
>>>
>>> On Thu, 2015-10-29 at 14:34 +0100, David Lindholm wrote:
>>>> Hi,
>>>>
>>>> Please review the following patch that changes how startsHumongous and
>>>> continuesHumongous regions work in G1. Before this patch the top and end
>>>> of the startsHumongous regions were set so that they covered the entire
>>>> object, pointing into the last continousHumongous region for this
>>>> object. So used() and capacity() of the startsHumongous region could be
>>>> substantially larger than a normal region size.
>>>>
>>>> Now each region is "self-contained", so top and end will not point
>>>> outside the region. This makes it possible to remove many special cases
>>>> in the code, and the model is more clear. Also, one doesn't have to
>>>> figure out if G1CollectedHeap::heap_region_containing_raw() or
>>>> G1CollectedHeap::heap_region_containing() should be called (only the
>>>> latter is left in the code).
>>>>
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8139867
>>>> Webrev: http://cr.openjdk.java.net/~david/JDK-8139867/webrev.00/
>>>>
>>>> Testing: Passed JPRT.
>>>>
>>> Thanks for the change. Overall I think it makes the code more
>>> understandable.
>>>
>>> Some comments:
>>>
>>>    - in CheckBitmapClearHRClosure: while HeapRegion::end() cannot change,
>>> it would still be nice if the value would be guaranteed to be only read
>>> once. I.e. either make Space::_end volatile, or the assignment to "end"
>>> from it.
>> I started doing _end volatile, but that became a quite large change in
>> itself. I would like to add a comment that end() never changes in G1 and
>> do the _end volatile in another change.
> Ok. That's good.
>
>>> Could you also modify HeapRegion::set_end() to make sure that it can
>>> only be set to one value? (i.e. _bottom + HeapRegion::GrainBytes?) Or
>>> alternatively only called once if that is possible?
>> Ok, added an assert().
> Thanks in general for re-adding asserts.
>
>>>    - pre-existing: maybe the call to BitMap::par_at_put() in
>>> CMCountDataClosureBase::set_bit_for_region() could be changed to
>>> BitMap::par_set_bit().
>> I would like to not fix this in this change, since it is a separate issue.
> That's okay.
>
>>>    - PostCompactionPrinterClosure::doHeapRegion: what's the reason for the
>>> removal of the SingleHumongous HR printer event? While it is debatable
>>> whether there should be a difference between humongous objects that span
>>> only a single region and others, I think this should be a different
>>> change.
>> Because SingleHumongous doesn't seem to be a valid HeapRegionType, and
>> that HeapRegion::region_num() doesn't exist any more, since it is always
>> 1 now. Sure, I could check if the next hr (by index) is not a
>> ContinousHumongous or if the object is smaller than a region size, but
>> since SingleHumongous is not a concept in HeapRegionTypes, I didn't.
> I would prefer that either the current behavior is kept, or
> SingleHumongous removed completely. Either is fine for me, but it would
> be awkward to sometimes label the same region with the same content as
> SingleHumongous and sometimes different.
>
>>>    - I think the code in VerifyRegionClosure::doHeapRegion() should not
>>> skip humongous regions and the code fixed but not skipped entirely.
>> It is not skipped, r->verify is still called. The only thing that is not
>> called is the size calculation and size check. Would it be ok to leave
>> it that way?
> Okay.
>
>>> Maybe add a G1CollectedHeap::humongous_obj_size_in_regions(size_t
>>> word_size) that does that calculation?
>>>
>>>    - is it possible to (at least) wrap the code sequence:
>>>
>>> r->set_containing_set(NULL);
>>> <some_heap_regionsetcount>.increment(1u, r->capacity())
>>> g1h->free_humongous_region(r, some_free_region_list, false)
>>>
>>> return r->used();
>>>
>>> into a method? That method could at least be used in two places (if not
>>> three).
>> This function could be used at 2 places (I think), but it would have to
>> take 2 arguments (the Count and the regionlist).
>> I don't really see the point in this refactorization.
> Less need to search for uses of free_humongous_regions if something in
> that code changes.
>
>>>    - G1CollectedHeap::next_region_by_index(): I would prefer that method
>>> would be limited to the case it is used for now, i.e. humongous objects.
>>> That method as is allows iteration of the entire heap, and that is
>>> functionality that seems way too powerful to be included in this change.
>>>
>>> It is also underspecified in behavior regarding concurrent operation on
>>> the underlying memory. It does not have anything to do with an "index"
>>> as suggested by the name.
>>>
>>> I.e. some method that lets you get the next region in the regions
>>> occupied by a single humongous object would be significantly better.
>>>
>>> E.g. in pseudo code:
>>>
>>> HeapRegion::get_next_humongous_region(HeapRegion* r) {
>>>     assert(r->is_humongous() && r is part of the current humongous
>>> object*, "invariant");
>>>     if (is last region) {
>>>       return NULL;
>>>     } else {
>>>       return region[r->index + 1];
>>>     }
>>> }
>> I disagree. I don't think this HeapRegion should have any knowledge
>> about the other HeapRegions, that breaks the abstraction imho. Making
>> HeapRegion (more) self contained is one of the points of this change. If
>> we want information about other HeapRegions we should go via the
>> HeapRegionManager.
>>
>> About the name: It has everything to do with index. It is "next by
>> index", not next by next()-pointer. When doing this I first used the
>> next() but I found out that next() is something else.
> I would still prefer that it were guaranteed that the iteration cannot
> go beyond the humongous object in any case.
>
>>> * either a humongous start region, or the object in the associated
>>> humongous start region covers that region.
>>>
>>> G1 guarantees that the memory associated with a humongous object is
>>> contiguous and valid/live as long as the humongous object is live.
>>>
>>> This will also simplify the two while loops in the places we iterate
>>> over all regions in the humongous objects.
>>>
>>> Then G1CollectedHeap::next_region_by_index and the method in
>>> HeapRegionManager can go away too.
>>>
>>>    - G1PrepareCompactClosure::doHeapRegion(): I think that
>>> HeapRegion::humongous_start_region() references itself in a humongous
>>> start region, i.e. there is no need to have different paths to find out
>>> "obj".
>> Ok, fixed.
> :) I saw that this pattern has been used multiple times, great.
>
>>>    - why the removal of the assert in G1RemSet::par_write_ref()? I do
>>> think that _from cannot be NULL, because it is explicitly checked in the
>>> only caller of par_write_ref, and write_ref is never called. (I filed
>>> JDK-8141135 to remove write_ref())
>> Because this asserts fails if p is in a ContinousHumongous (from is the
>> StartsHumongous). I have modified the assert to reflect this.
> Okay, thanks.
>
>>>    - can we add an assert in HeapRegion::block_is_obj() that the new case
>>> can only occur with humongous objects, and that p must be in the same
>>> humongous object?
>> Ok, fixed.
> Looks good except for the SingleHumongous and the iteration over the
> humongous objects. Could that be fixed?

Yes, I fixed those issues:

Webrev: http://cr.openjdk.java.net/~david/JDK-8139867/webrev.03/
Webrev: http://cr.openjdk.java.net/~david/JDK-8139867/webrev.02-03/ (diff)


Thanks,
David

> Thanks,
>    Thomas
>
>



More information about the hotspot-gc-dev mailing list