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

[Mikael Gerdin]

Hi Thomas,

On 2015-08-24 12:36, Thomas Schatzl wrote:
> 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)

Overall I think this change is good.
I'm somewhat paranoid about output parameters, can you please add code 
to initialize the value to be set by the out parameter to a dummy value 
(-1?) and assert that it was properly set on the code paths where the 
actual size is being used.

The alignment of the \ in vmStructs_g1.hpp is wrong, I don't need to see 
that in a new webrev though :)

/Mikael

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