RFR (S): 8067339: PLAB reallocation might result in failure to allocate object in that recently allocated PLAB

[Bengt Rutisson]

Hi Thomas,

On 17/08/15 17:09, Thomas Schatzl wrote:
> Hi Bengt,
>
> On Mon, 2015-08-17 at 10:50 +0200, Bengt Rutisson wrote:
>> Hi Thomas,
>>
>> On 2015-08-14 10:29, Thomas Schatzl wrote:
>>> Hi all,
>>>
>>>     can I have reviews for the following small fix: if G1 reallocates a
>>> PLAB for an object of size X, it allocates a new PLAB of size Y.
>>>
>>> PLABs are required to contain enough space for the object plus some
>>> alignment reserve; so if X >= Y - alignment-reserve, G1 failed to
>>> allocate this object it just allocated the PLAB for into that PLAB,
>>> resulting in an evacuation failure although there is actually enough
>>> space.
>>>
>>> As far as I know this has been a rare occurrence until now because
>>> current PLAB sizing tended to maximize PLAB sizes, and it is rare to
>>> have such large objects, but with future changes that actually try to
>>> size the PLABs so that the expected waste is kept, I noticed a few cases
>>> of that issue occurring. So this needs to be fixed.
>>>
>>> The fix is to consider the required alignment reserve in the PLAB
>>> allocation request.
>>>
>>> This is a day one G1 bug.
>>>
>>> CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8067339
>>> Webrev:
>>> http://cr.openjdk.java.net/~tschatzl/8067339/webrev/
>>> Testing:
>>> JPRT, vm.gc testlist, lots of testing.
>> Interesting bug. Nice find!
>>
>> I agree that you fix solves the problem. I'm just wondering if we could
>> make the dependency on the alignment reserve between G1PLABAllocator and
>> PLAB more explicit. With your fix,
>> G1PLABAllocator::allocate_direct_or_new_plab() has to know exactly what
>> PLAB::set_buf() does to fix the sizes correctly. It would be nice if the
>> logic could be in two places instead of one.
> We could ask the PLAB for a given allocation, how much space you need.
> I.e. basically move line 236 in g1Allocator.cpp to the PLAB class.
>
> New changes at
> http://cr.openjdk.java.net/~tschatzl/8067339/webrev.0_to_1 (diff)
> http://cr.openjdk.java.net/~tschatzl/8067339/webrev.1 (full)

I like this better. Thanks for fixing it!

>
>> Not sure exactly how to do that. One way would be to move the alignment
>> reserve fixup up to G1PLABAllocator::allocate_direct_or_new_plab(). It
>> could add the alignment reserve to the size it passes to
>> par_allocate_during_gc(). Then PLAB::set_buf() could just assert that
>> the size if the PLAB is at least the size of the alignment reserve.
>> Potentially this would look nicer if  PLAB::set_buf() and
>> PLAB::set_word_size() were combined into one method that also takes the
>> alignment reserve.
>>
>> I realize that this would affect CMS also, unless the new changes are
>> contained in G1PLAB. But looking at
>> ParScanThreadState::alloc_in_to_space_slow(), I think that CMS may have
>> the same issue, right? So, maybe it is good to fix it for both collectors?
> CMS has the same issue, but it simply promotes the object, having a more
> loose assert in that place. That would be an option in G1 too.
>
> Parallel GC does not get into this issue because it simply inline
> allocates any object less than half of the PLAB size. Parallel just
> assumes that (PLAB-size / 2) > PLAB::alignment_reserve; besides, it has
> a completely different PLAB implementation.
>
> To me it seems what Parallel GC does is the best way of action. The only
> problem is that in G1 this would almost certainly increase waste at the
> end of regions significantly.
> There will be some change in the future that fixes this, so for now I
> would like to postpone this problem.
>
> I created JDK-8133724 to look at this in more detail if you do not mind.

Yes, that sounds like a good approach.

Ship it!
Bengt

>
> Thanks,
>    Thomas
>
>