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

[Thomas Schatzl]

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)

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

Thanks,
  Thomas