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

[Tom Benson]

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 ..."


   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.


   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?


- 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.


   77   if (may_retire_region(remaining_words)) {
   78     // Immediately retire.
   79     retire_region(r);

This comment seems superfluous.


- heapRegion.hpp -

  326   HeapRegion* _next_in_retained;
  327  public:
  328   void set_next_in_retained(HeapRegion* next) {

Blank line before public: ?


- heapRegion.inline.hpp -

The change to the par_allocate_impl loop is just semi-related 'cleanup,' 
correct?



- 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?


  123   // and desired size in words, and sets that region as new active 
region.

Should be   ... and set that region...


  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...".


  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?


- 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.

Tom



On 10/2/2015 8:59 AM, Thomas Schatzl wrote:
> Hi all,
>
>    Dima made me aware of a few other minor problems in the webrev. See
> below:
>
> On Fri, 2015-10-02 at 14:56 +0300, Dmitry Fazunenko wrote:
>> Hi Thomas,
>>
>> The new version looks good to me.
>> Just a few very minor things:
>>
>> 1)
>>     28             of regions, which stresses the G1 mechanism to retain
>> regions during the whole GC.
>> -->
>>     28      *     of regions, which stresses the G1 mechanism to retain
>> regions during the whole GC.
>>
>> 2)
>>     31  * @requires (vm.gc=="G1" | vm.gc=="null")
>> -->
>>     31  * @requires vm.gc=="G1" | vm.gc=="null"
>>
>> 3) Your code doesn't use any modules but the java.base, so you don't
>> need the @modules statement.
>>
>> I don't need a new webrev for that corrections.
>>
> Thanks for the re-review. I updated the webrev in-place since these are
> minor changes.
>
>   http://cr.openjdk.java.net/~tschatzl/8067433/webrev.1/ (full)
>   http://cr.openjdk.java.net/~tschatzl/8067433/webrev.0_to_1/ (diff)
>
> Thanks,
>    Thomas
>