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

[Thomas Schatzl]

Hi Tom,

  thanks for the review.

On Wed, 2015-08-19 at 18:19 -0400, Tom Benson wrote:
> 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

I will remove the one in 274.

New webrevs:
http://cr.openjdk.java.net/~tschatzl/8067336/webrev.1_to_2/index.html
(diff)
http://cr.openjdk.java.net/~tschatzl/8067336/webrev.2/index.html (full)

Thanks,
  Thomas

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