RFR: 8202994: Add support for undoing last TLAB allocation

[Per Liden]

Hi,

I don't really have a strong opinion on this. There's some comfort in 
asserting on things we should never be doing (i.e. undo something that 
is in the TLAB but it's not the last allocation). On the other hand, I 
think you proposal is also fine, and I _think_ it can be simplified even 
further with a single pointer_delta call. Like this:

http://cr.openjdk.java.net/~pliden/8202994/webrev.2

In any case, I'm fine with going with either of
http://cr.openjdk.java.net/~pliden/8202994/webrev.1
and
http://cr.openjdk.java.net/~pliden/8202994/webrev.2

Any strong opinions from others?

cheers,
Per

On 05/11/2018 07:06 PM, JC Beyler wrote:
> And because I do things too fast sometimes and change code in an email at the last minute, here would be the correct test:
> 
> 
> +inline bool ThreadLocalAllocBuffer::undo_allocate(HeapWord* obj, size_t 
> size) {
> + invariants();
> +
> + // The object might not be allocated in this TLAB
> + if (obj < start() || pointer_delta(top(), obj) != size) {
> 
> + return false;
> 
> +  }
> 
> +    set_top(obj);
> 
> + invariants();
> + return true;
> 
> +}
> 
> 
> 
> On Fri, May 11, 2018 at 10:04 AM JC Beyler <jcbeyler at google.com 
> <mailto:jcbeyler at google.com>> wrote:
> 
>     Hi Per,
> 
>     It looks kind of weird to me that the method now checks if the obj
>     is contained in the TLAB but might undo it even if it is not the
>     last allocation (though it would fail the assertion). Why not just
>     change the contains to being something like : is_last_allocation and
>     then the test can be at the top of the method without the need of
>     the second test.
> 
>     I wonder if you even need a new method then and not just do:
> 
> 
>     +inline bool ThreadLocalAllocBuffer::undo_allocate(HeapWord* obj,
>     size_t size) {
>     + invariants();
>     +
>     + // The object might not be allocated in this TLAB
>     + if (obj >= start() && pointer_delta(top(), obj) != size) {
> 
>     + return false;
> 
>     + }
> 
>     +
>     + set_top(obj);
>     +
>     + invariants();
>     + return true;
>     +}
> 
> 
>     Just thought I'd mention it :),
>     Jc
> 
>     On Fri, May 11, 2018 at 8:28 AM Aleksey Shipilev <shade at redhat.com
>     <mailto:shade at redhat.com>> wrote:
> 
>         On 05/11/2018 05:17 PM, Per Liden wrote:
>          > Updated webrev:
>         http://cr.openjdk.java.net/~pliden/8202994/webrev.1
> 
>         Looks fine to me.
> 
>          >> *) Curiosity: why ZGC uses TLABs, not PLABs for these
>         relocations to begin with? Shenandoah needs
>          >> rewinds like that too, for same reason, and we use
>         PLAB(-like) facilities for that, leaving TLABs
>          >> for "real" mutator allocations.
>          >
>          > We haven't really seen the need to introduced the concept of
>         PLABs, neither for mutators nor for GC
>          > workers. For the most part, an allocation is just an
>         allocation, so we naturally just use a
>          > mutator's TLAB if there is one. GC workers allocate straight
>         into a CPU-local ZPage (same page a
>          > mutator on that CPU would allocate a TLAB from). An
>         allocation request in ZGC also comes with a set
>          > of "allocation flags", which, when needed, can tell us what
>         the allocation is for, etc.
>          >
>          > I suspect it might also help reduce fragmentation a bit (i.e.
>         no PLAB waste), but that's secondary
>          > and might not even matter much in the end.
> 
>         Oh yes, fragmentation is something that is helped with
>         separating TLABs and PLABs: young objects are
>         mostly dead, and relocated objects are usually alive. In
>         Shenandoah, we try to separate the two. It
>         might not matter for ZGC.
> 
>         -Aleksey
>