<div dir="ltr"><div dir="ltr">Thank you both for the reviews and suggestions.</div><div>I will add such comment in this patch.</div><div dir="ltr">It is really helpful to know these little details.</div><div dir="ltr"><br></div><div>For those other places that pass MemRegions by const-ref,</div><div>do you think we should change them to pass-by-value if we</div><div>validate that the compiler generates the same code as pass-by-ref?</div><div><br></div><div>I still think it might be better to just pass by const-ref for most</div><div>places, as it will prevent this issue from happening even if the</div><div>compiler failed to apply the optimization for pass-by-value.</div><div dir="ltr"><br clear="all"><div><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr">-Man</div></div></div><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Feb 5, 2019 at 10:42 AM Kim Barrett <<a href="mailto:kim.barrett@oracle.com">kim.barrett@oracle.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">> On Feb 5, 2019, at 11:25 AM, Man Cao <<a href="mailto:manc@google.com" target="_blank">manc@google.com</a>> wrote:<br>
> <br>
> Hi all,<br>
> <br>
> Could I have reviews for this small patch?<br>
> Webrev: <a href="https://cr.openjdk.java.net/~manc/8218192/webrev.00/" rel="noreferrer" target="_blank">https://cr.openjdk.java.net/~manc/8218192/webrev.00/</a><br>
> RFE: <a href="https://bugs.openjdk.java.net/browse/JDK-8218192" rel="noreferrer" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8218192</a><br>
> <br>
> This will improve G1 write barrier performance for jython in DaCapo, when building HotSpot with LLVM. More Background and performance numbers are included in the JBS.<br>
> <br>
> Thanks,<br>
> Man<br>
<br>
Looks good.<br>
<br>
The only thing I might suggest is adding a comment that the destructor<br>
and copy constructor must be trivial, to support optimization of pass<br>
by value.  I don't need a new webrev if you add such a comment.<br>
<br>
The explicit copy constructor is clearly interfering with the expected<br>
optimization on Linux-x64, and likely other platforms where I didn't<br>
examine the generated assembly.  The SysV ABI describes passing such<br>
an object in a pair of registers for the members, but only when the<br>
destructor and copy constructor are trivial.  (An older version of the<br>
ABI required the class to be POD, which is unnecessarily restrictive;<br>
gcc through 4.9 (I think) used that.)<br>
<br>
There are a handful of places that are passing MemRegions by const-ref<br>
rather than by value that probably ought to be looked at, but that can<br>
be done separately.<br>
<br>
I did some performance testing of this change as well.  Seemed pretty<br>
much in the noise.<br>
<br>
</blockquote></div></div>