<div dir="ltr">Hi Per,<div><br></div><div>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.</div><div><br></div><div>I wonder if you even need a new method then and not just do:</div><div><pre style="color:rgb(0,0,0);font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial"><span class="gmail-new" style="color:blue;font-weight:normal"><br class="gmail-Apple-interchange-newline">+inline bool ThreadLocalAllocBuffer::undo_allocate(HeapWord* obj, size_t size) {</span>
<span class="gmail-new" style="color:blue;font-weight:normal">+  invariants();</span>
<span class="gmail-new" style="color:blue;font-weight:normal">+</span>
<span class="gmail-new" style="color:blue;font-weight:normal">+  // The object might not be allocated in this TLAB</span>
<span class="gmail-new" style="color:blue;font-weight:normal">+  if (obj >= start() && </span><span class="gmail-new" style="color:blue;font-weight:normal">pointer_delta(top(), obj) != size) {</span></pre><pre style="color:rgb(0,0,0);font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial"><span class="gmail-new" style="color:blue;font-weight:normal">+     return false;<br></span></pre><pre style="color:rgb(0,0,0);font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial"><span class="gmail-new" style="color:blue;font-weight:normal">+  }<br></span></pre><pre style="color:rgb(0,0,0);font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial"><span class="gmail-new" style="color:blue;font-weight:normal">+</span>
<span class="gmail-new" style="color:blue;font-weight:normal">+  set_top(obj);</span>
<span class="gmail-new" style="color:blue;font-weight:normal">+</span>
<span class="gmail-new" style="color:blue;font-weight:normal">+  invariants();</span>
<span class="gmail-new" style="color:blue;font-weight:normal">+  return true;</span>
<span class="gmail-new" style="color:blue;font-weight:normal">+}</span></pre><br></div><div>Just thought I'd mention it :),</div><div>Jc</div></div><br><div class="gmail_quote"><div dir="ltr">On Fri, May 11, 2018 at 8:28 AM Aleksey Shipilev <<a href="mailto:shade@redhat.com">shade@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 05/11/2018 05:17 PM, Per Liden wrote:<br>
> Updated webrev: <a href="http://cr.openjdk.java.net/~pliden/8202994/webrev.1" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~pliden/8202994/webrev.1</a><br>
<br>
Looks fine to me.<br>
<br>
>> *) Curiosity: why ZGC uses TLABs, not PLABs for these relocations to begin with? Shenandoah needs<br>
>> rewinds like that too, for same reason, and we use PLAB(-like) facilities for that, leaving TLABs<br>
>> for "real" mutator allocations.<br>
> <br>
> We haven't really seen the need to introduced the concept of PLABs, neither for mutators nor for GC<br>
> workers. For the most part, an allocation is just an allocation, so we naturally just use a<br>
> mutator's TLAB if there is one. GC workers allocate straight into a CPU-local ZPage (same page a<br>
> mutator on that CPU would allocate a TLAB from). An allocation request in ZGC also comes with a set<br>
> of "allocation flags", which, when needed, can tell us what the allocation is for, etc.<br>
> <br>
> I suspect it might also help reduce fragmentation a bit (i.e. no PLAB waste), but that's secondary<br>
> and might not even matter much in the end.<br>
<br>
Oh yes, fragmentation is something that is helped with separating TLABs and PLABs: young objects are<br>
mostly dead, and relocated objects are usually alive. In Shenandoah, we try to separate the two. It<br>
might not matter for ZGC.<br>
<br>
-Aleksey<br>
<br>
</blockquote></div>