RFR JDK-8141491: Unaligned memory access in Bits.c

[David Holmes]

Hi Mikael,

Can't really comment on the bit-twiddling details.

A couple of minor style nits:

- don't put "return" on a line by itself, include the first part of the 
return expression
- spaces after commas in template definitions/instantiation

The JVM_ENTRY_FROM_LEAF etc was a little mind twisting but seems okay.

Otherwise hotspot and JDK code appear okay.

Thanks,
David

On 3/02/2016 5:25 AM, Mikael Vidstedt wrote:
>
> Please review this change which introduces a Copy::conjoint_swap and an
> Unsafe.copySwapMemory method to call it from Java, along with the
> necessary changes to have java.nio.Bits call it instead of the Bits.c code.
>
> http://cr.openjdk.java.net/~mikael/webrevs/8141491/webrev.03/hotspot/webrev/
>
> http://cr.openjdk.java.net/~mikael/webrevs/8141491/webrev.03/jdk/webrev/
>
> On the jdk/ side I don't think there should be a lot of surprises.
> Bits.c is gone and that required a mapfile-vers to be changed
> accordingly. I also added a relatively extensive
> jdk/internal/misc/Unsafe/CopySwap.java test which exercises all the
> various copySwap configurations and verifies that the resulting data is
> correct. There are also a handful of negative tests in there.
>
> On the hotspot/ side:
>
> * the copy logic in copy.cpp is leveraging templates to help the C++
> compiler produce tight copy loops for the various configurations
> {element type, copy direction, src aligned, dst aligned}.
> * Unsafe_CopySwapMemory is a leaf to not stall safe points more than
> necessary. Only if needed (THROW, copy involves heap objects) will it
> enter VM using a new JVM_ENTRY_FROM_LEAF macro.
> * JVM_ENTRY_FROM_LEAF calls a new VM_ENTRY_BASE_FROM_LEAF helper macro,
> which mimics what VM_ENTRY_BASE does, but also does a
> debug_only(ResetNoHandleMark __rnhm;) - this is because
> JVM_LEAF/VM_LEAF_BASE does debug_only(NoHandleMark __hm;).
>
> I'm in the process of getting the last performance numbers, but from
> what I've seen so far this will outperform the earlier implementation.
>
> Cheers,
> Mikeal
>
> On 2016-01-27 17:13, Mikael Vidstedt wrote:
>>
>> Just an FYI:
>>
>> I'm working on moving all of this to the Hotspot Copy class and
>> bridging to it via jdk.internal.misc.Unsafe, removing Bits.c
>> altogether. The implementation is working, and the preliminary
>> performance numbers beat the pants off of any of the suggested Bits.c
>> implementations (yay!).
>>
>> I'm currently in the progress of getting some unit tests in place for
>> it all to make sure it covers all the corner cases and then I'll run
>> some real benchmarks to see if it actually lives up to the expectations.
>>
>> Cheers,
>> Mikael
>>
>> On 2016-01-26 11:13, John Rose wrote:
>>> On Jan 26, 2016, at 11:08 AM, Andrew Haley <aph at redhat.com> wrote:
>>>> On 01/26/2016 07:04 PM, John Rose wrote:
>>>>> Unsafe.copyMemory bottoms out to Copy::conjoint_memory_atomic.
>>>>> IMO that's a better starting point than memcpy.  Perhaps it can be
>>>>> given an additional parameter (or overloading) to specify a swap size.
>>>> OK, but conjoint_memory_atomic doesn't guarantee that destination
>>>> words won't be torn if their source is misaligned: in fact it
>>>> guarantees that they will will be.
>>> That's a good point, and argues for a new function with the
>>> stronger guarantee.  Actually, it would be perfectly reasonable
>>> to strengthen the guarantee on the existing function.  I don't
>>> think anyone will care about the slight performance change,
>>> especially since it is probably favorable.  Since it's Unsafe,
>>> they are not supposed to care, either.
>>>
>>> — John
>>
>