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

[Thomas Schatzl]

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