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

[Bengt Rutisson]

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