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

[Bengt Rutisson]

"It would be nice if the logic could be in two places instead of one. "

I claim 'vacation brain' since I just got back from vacation. I of 
course meant that the logic should be in one place. :)

Bengt

On 2015-08-17 10:50, 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.
>
> 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?
>
> Thanks,
> Bengt
>
>>
>> Thanks,
>>    Thomas
>>
>>
>