RFR (XS): 8218192: Remove copy constructor for MemRegion
manc at google.com
Wed Feb 6 11:13:49 UTC 2019
Thank you both for the reviews and suggestions.
I will add such comment in this patch.
It is really helpful to know these little details.
For those other places that pass MemRegions by const-ref,
do you think we should change them to pass-by-value if we
validate that the compiler generates the same code as pass-by-ref?
I still think it might be better to just pass by const-ref for most
places, as it will prevent this issue from happening even if the
compiler failed to apply the optimization for pass-by-value.
On Tue, Feb 5, 2019 at 10:42 AM Kim Barrett <kim.barrett at oracle.com> wrote:
> > On Feb 5, 2019, at 11:25 AM, Man Cao <manc at google.com> wrote:
> > Hi all,
> > Could I have reviews for this small patch?
> > Webrev: https://cr.openjdk.java.net/~manc/8218192/webrev.00/
> > RFE: https://bugs.openjdk.java.net/browse/JDK-8218192
> > 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.
> > Thanks,
> > Man
> Looks good.
> The only thing I might suggest is adding a comment that the destructor
> and copy constructor must be trivial, to support optimization of pass
> by value. I don't need a new webrev if you add such a comment.
> The explicit copy constructor is clearly interfering with the expected
> optimization on Linux-x64, and likely other platforms where I didn't
> examine the generated assembly. The SysV ABI describes passing such
> an object in a pair of registers for the members, but only when the
> destructor and copy constructor are trivial. (An older version of the
> ABI required the class to be POD, which is unnecessarily restrictive;
> gcc through 4.9 (I think) used that.)
> There are a handful of places that are passing MemRegions by const-ref
> rather than by value that probably ought to be looked at, but that can
> be done separately.
> I did some performance testing of this change as well. Seemed pretty
> much in the noise.
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the hotspot-gc-dev