RFR (L): 8067433: Keep waste at end of regions for further allocation during GC

[Tom Benson]

Hi Thomas,
All looks good to me!  Thanks,
Tom

On 10/15/2015 9:02 AM, Thomas Schatzl wrote:
> Hi Tom,
>
>    thanks for the review...
>
> On Wed, 2015-10-14 at 16:00 -0400, Tom Benson wrote:
>> Hi Thomas,
>> Here are some review comments on the code, mostly superficial.  I think
>> it's a great addition.
>>
>> - retainedRegionsList.hpp -
>>
>>     37 // considered not worth further allocating into are automatically
>> retired, while
>>     38 // we keep onto regions that still contain a reasonable amount of
>> bytes.
>>
>> Perhaps should be "... we keep holding onto ..."
>>
> Fixed.
>
>>     43 // Note that after appending a region to this list, threads might
>> still allocate
>>     44 // from these regions.
>>
>> Can a little more of the comments you made in your mail message be
>> included here?  That comment talks about threads that already have refs
>> to this region continuing to try to use it, while the above sounds like
>> it means threads traversing the list and and trying to allocate from
>> regions on it in place.
>>
> Fixed.
>
>>     74   // Add the given region to this list.
>>     75   void append(HeapRegion* r);
>>     76   // Get a region with at least min_size available space and
>> allocate up to
>>
>> min_size should be min_word_size.  Do you want a blank line at 76?
>>
> Fixed.
>
>> - retainedRegionsList.cpp -
>>
>>     41 void RetainedRegionsList::add_to_list(HeapRegion** list,
>> HeapRegion* region) {
>>
>> The assertion here that walks the list to make sure the count is right
>> after adding a new element seems like overkill to me... but OK.  But as
>> long as you're doing that, you could also check that the element you're
>> about to add isn't already there.  If it was the last element, its
>> next_in_retained would be null, so there would be no assertion failure
>> when it was then added to the head, creating a circular list.
> Fixed.
>
>>     77   if (may_retire_region(remaining_words)) {
>>     78     // Immediately retire.
>>     79     retire_region(r);
>>
>> This comment seems superfluous.
> Removed.
>
>> - heapRegion.hpp -
>>
>>    326   HeapRegion* _next_in_retained;
>>    327  public:
>>    328   void set_next_in_retained(HeapRegion* next) {
>>
>> Blank line before public: ?
> Added
>
>> - heapRegion.inline.hpp -
>>
>> The change to the par_allocate_impl loop is just semi-related 'cleanup,'
>> correct?
>>
> I can split it out, it's just a two-line change though... Basically the
> problem is that it is preferable to reuse the result from any cmpxchg as
> this avoids you to consider whether you need to use volatile.
>
>> - g1AllocRegion.hpp -
>>
>>     96 public:
>>     97   // Perform a MT-safe allocation out of the given region.
>>     98   static inline HeapWord* par_allocate(HeapRegion* alloc_region,
>>
>> Move the now-public par_allocate entries down with the other public entries?
> Done.
>
>>    123   // and desired size in words, and sets that region as new active
>> region.
>>
>> Should be   ... and set that region...
> Done.
>
>>    139   // Allocate a new region that is large enough to fit word_size
>> into it.
>>    140   virtual HeapRegion* allocate_new_region(size_t word_size, bool
>> force) = 0;
>>    141   // Allocate a new region and allocate word_size words into it,
>> returning both the
>>
>> word_size should be min_word_size. Perhaps say "allocate at least
>> min_word_size words...".
> Done. Thanks for looking through the comments btw.
>
>>    205 void G1AllocRegion::release_during_gc() {
>>
>> This name seems confusing to me.  G1AllocRegions's release_during_gc
>> always does release, while G1GCAllocRegion's release_during_gc will
>> either add to the retained list, or retire if the remaining space is
>> below threshold.  new_alloc_region_and_allocate invokes one or the other
>> of these.  Maybe "release_or_retain" would be a better name?
> I changed it to release_or_retain_during_gc and added some hopefully
> better comment.
>
>> - g1_globals.hpp -
>>
>> Is the new option really necessary? It doesn't seem there would be a
>> benefit from disabling it, other than working around a bug.
> That's the intention. If you think it is not necessary I can remove it,
> I think the change is good.
>
> New webrevs at
> http://cr.openjdk.java.net/~tschatzl/8067433/webrev.2/ (full)
> http://cr.openjdk.java.net/~tschatzl/8067433/webrev.1_to_2/ (diff)
>
> Thanks,
>    Thomas
>
>