RFR (M): 8067336: Allow that PLAB allocations at the end of regions are flexible

[Tom Benson]

Hi Thomas -
Looks good to me.   Though there was one odd thing I noticed, not in the 
changes but adjacent to one - this comment in g1Allocator.cpp:

  273     if (buf != NULL) {
  274       // Otherwise.
  275       alloc_buf->set_buf(buf, actual_plab_size);

There is another "Otherwise." comment just afterward at line 283, where 
it makes sense, but the one at 274 looks left over from some past 
restructuring.  Your call.  8^)
Tom

On 8/19/2015 5:18 PM, Thomas Schatzl wrote:
> Hi Tom,
>
> On Tue, 2015-08-18 at 11:03 -0400, Tom Benson wrote:
>> Hi Thomas,
>> It looks like the actual plab allocation changes are missing from
>> g1Allocator.cpp - only an include was added...   ?
>>
>> Aside from that, just a couple of minor comments.  I think the comment
>> at line 193-194 in g1AllocRegion.hpp should say "...maximum word size of
>> the allocation in desired_word_size" rather than "... in word_size".
>>
>> In g1AllocRegion.inline.hpp, attempt_allocation and
>> attempt_allocation_locked both have an error path that includes:
>> trace("alloc failed", *actual_word_size);
>>
>> In the old version, the value handed to trace was the requested size, so
>> here it should probably be min_word_size, since *actual_word_size is
>> likely to be uninitialized.
>    thanks for the review, even if it was for an unintentionally broken
> change. I seem to have removed the actual change, the use of the new
> methods in G1PLABAllocator::allocate_direct_or_new_plab(), while merging
> these series of PLAB changes and the evac failure changes.
>
> I also fixed the G1AllocRegion::trace() method, adding the new sizes
> (desired/actual size) as needed.
>
> Then I decided to move making G1OffsetTableContigSpace::_top volatile
> (this is a _potential_ pre-existing bug - I am sure there is some weird
> compiler that notices that it could hoist this access out of the loop in
> G1OffsetTableContigSpace::par_allocate_impl()) into this CR. I am sure
> you would have noticed anyway.
>
> I think I also covered the other problems you mentioned. Thanks again.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8067336
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8067336/webrev.1 (full)
> http://cr.openjdk.java.net/~tschatzl/8067336/webrev.0_to_1 (diff)
> Testing:
> jprt, vm.gc testlist, perf testing benchmarks, locally checked
> allocation tracing (enabled by define only)
>
> Thanks,
>    Thomas
>