<div dir="ltr"><div>Hi Jon, Thomas</div><div><br></div><div>We initially implemented this in consideration of the completeness in execution logic.</div><div>We don't have detailed data to show the effect of lines 144-145 individually.</div><div>We believe that these two lines (if not hit) incurs little overhead, but this logic can help to cache more in some occasions, so we just kept it.<br></div><div>Thanks Thomas, for helping us prove the effectiveness.</div><div><br></div><div class="gmail_extra"><div class="gmail_quote">2016-01-15 20:49 GMT+08:00 Thomas Schatzl <span dir="ltr"><<a href="mailto:thomas.schatzl@oracle.com" target="_blank">thomas.schatzl@oracle.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Thu, 2016-01-14 at 12:09 -0800, Jon Masamitsu wrote:<br>
> <a href="http://cr.openjdk.java.net/~tschatzl/8146987/webrev.1/src/share/vm/gc" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~tschatzl/8146987/webrev.1/src/share/vm/gc</a><br>
</span><div><div class="h5">> /parallel/parMarkBitMap.cpp.frames.html<br>
><br>
>  143   } else if (end_obj < last_obj) {<br>
>  144     if (pointer_delta((HeapWord*)end_obj, (HeapWord*)beg_addr) ><br>
> pointer_delta((HeapWord*)last_obj, (HeapWord*)end_obj)) {<br>
>  145       last_ret = last_ret -<br>
> live_words_in_range_helper((HeapWord*)end_obj, last_obj);<br>
>  146     } else {<br>
>  147       last_ret = live_words_in_range_helper(beg_addr, end_obj);<br>
>  148     }<br>
><br>
> Did you measure the performance improvement afforded by  lines 144 -<br>
> 145?<br>
><br>
> The calculation of the new address is used in two cases.  One is when<br>
> the live object is being moved to its new location.  In that case<br>
> I would expect that the overwhelmingly common case would be<br>
> end_obj > last_obj.  The calculation of the new location for a live<br>
> object<br>
> (where live_words_in_range() is used) proceeds from left to right<br>
> (lower<br>
> to higher addresses) as each region is scanned looking for live<br>
> objects.  I would expect the execution of line 145 to be seldom<br>
> if ever, so that just using 147 would be fine.  The other case is<br>
> less clear.   When an object is being moved, the object references<br>
> within it are updated.   That  access pattern seems like it would be<br>
> more<br>
> random to me (fewer cache hits)  but if you have data that shows<br>
> that line 145 is  beneficial, that would be a good data point.<br>
<br>
</div></div>  I did some measurements on which branches are taken in<br>
live_words_in_range() with SPECjbb2015 with constant IR (why constant<br>
IR? I had that setup and had used that for comparison runs<br>
before/after, see other email) and clamped down adaptive size policy<br>
(basically setting all heap sizes, 10g total heap, ~1g live set).<br>
See the patch here<br>
<a href="https://bugs.openjdk.java.net/secure/attachment/56451/reftypes.diff" rel="noreferrer" target="_blank">https://bugs.openjdk.java.net/secure/attachment/56451/reftypes.diff</a> and<br>
 the results at <a href="https://bugs.openjdk.java.net/secure/attachment/56452/r
ange_helper_types.png" rel="noreferrer" target="_blank">https://bugs.openjdk.java.net/secure/attachment/56452/r<br>
ange_helper_types.png</a><br>
<br>
That graph shows the kind of branching decisions taken during execution<br>
of every full gc. (X-axis is full gc number, y-axis relative branch<br>
 execution frequency). Labels include the index into the array<br>
containing the values.<br>
<br>
Ignore the first and last few full gcs, they are startup and shutdown<br>
related (i.e. system.gc's, whatever).<br>
>From this you can see that actually the branch in lines 144-145 is<br>
rather important (yellow - 4), as it catches a large amount of<br>
references that would otherwise need a full call to<br>
live_words_in_range() (turquoise - 5).<br>
<br>
A quick run with 144-145 removed, showed that this is indeed a problem<br>
and actually around half of the improvements from this patch are<br>
removed if this condition is removed. So I would opt to keep it :)<br>
<br>
With the usual disclaimer of that being only a single data point.<br>
<br>
Thanks,<br>
  Thomas<br>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature">Lei Tianyang<div><br></div></div>
</div></div>