ByteBuffer views, alignment, word tearing
I saw a few ACCEPTABLE_SPEC results for ByteBuffer views in jcstress. This results from word tearing. I was rather surprised, because I'd worked on the HeapByteBuffer implementation, and I knew that on we would't expect to see any word tearing for HeapByteBuffers or DirectByteBuffers, at least for aligned word fetches and stores. Unfortunately, we're still doing this in e.g. ByteBufferAsIntBufferL: public int get(int i) { return Bits.getIntL(bb, ix(checkIndex(i))); } where Bits.getIntL is defined as: static int getIntL(ByteBuffer bb, int bi) { return makeInt(bb._get(bi + 3), bb._get(bi + 2), bb._get(bi + 1), bb._get(bi )); } instead of what I'd thought we were doing, which was: public int get(int i) { return bb.getInt(ix(i)); } ... so we get slow byte-at-a-time implementations of all the getX and putX methods -- with no HotSpot intrinsic optimizations. And we get word tearing even though the data in a ByteBuffer view is always aligned. It never occurred to me that the ByteBuffer views didn't defer to the ByteBuffer.{get,put} methods. Either that or I forgot, but I should have fixed ByteBuffer views at the same time as HeapByteBuffer. It's not a huge job to fix this, but rather late for JDK 9. Andrew. [P.S. It's a little bit more complicated than this because a ByteBuffer view has the endianness that its parent had when the view was created. But that's not hard to do.]
Hi Andrew, See also: https://bugs.openjdk.java.net/browse/JDK-8151163 <https://bugs.openjdk.java.net/browse/JDK-8151163> http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8151163-buffer-unsafe-unaligned... <http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8151163-buffer-unsafe-unaligned-access/> http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8151163-buffer-unsafe-unaligned... <http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8151163-buffer-unsafe-unaligned-access/webrev/> http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8151163-buffer-unsafe-unaligned... <http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8151163-buffer-unsafe-unaligned-access-hotspot/webrev/> I opted to directly use Unsafe rather than defer. I am not sure it’s always possible to defer to the underlying buffer since its positional and order state is mutable. I got bogged down investigating some regressions on SPARC for long values that are misaligned such that 8 bytes are read. But i planned to proceed with this and see if we can clean up SPARC later on, but then i forgot until you reminded me :-) Paul.
On 11 Jul 2016, at 17:08, Andrew Haley <aph@redhat.com> wrote:
I saw a few ACCEPTABLE_SPEC results for ByteBuffer views in jcstress. This results from word tearing. I was rather surprised, because I'd worked on the HeapByteBuffer implementation, and I knew that on we would't expect to see any word tearing for HeapByteBuffers or DirectByteBuffers, at least for aligned word fetches and stores.
Unfortunately, we're still doing this in e.g. ByteBufferAsIntBufferL:
public int get(int i) { return Bits.getIntL(bb, ix(checkIndex(i))); }
where Bits.getIntL is defined as:
static int getIntL(ByteBuffer bb, int bi) { return makeInt(bb._get(bi + 3), bb._get(bi + 2), bb._get(bi + 1), bb._get(bi )); }
instead of what I'd thought we were doing, which was:
public int get(int i) { return bb.getInt(ix(i)); }
... so we get slow byte-at-a-time implementations of all the getX and putX methods -- with no HotSpot intrinsic optimizations. And we get word tearing even though the data in a ByteBuffer view is always aligned.
It never occurred to me that the ByteBuffer views didn't defer to the ByteBuffer.{get,put} methods. Either that or I forgot, but I should have fixed ByteBuffer views at the same time as HeapByteBuffer.
It's not a huge job to fix this, but rather late for JDK 9.
Andrew.
[P.S. It's a little bit more complicated than this because a ByteBuffer view has the endianness that its parent had when the view was created. But that's not hard to do.]
On 11/07/16 17:04, Paul Sandoz wrote:
I opted to directly use Unsafe rather than defer. I am not sure it’s always possible to defer to the underlying buffer since its positional and order state is mutable.
Eww. The view has its own position, and the byte ordering is fixed when the view is created. The wrapped byte buffer is in a protected field, and the view is a package-private class so it can't be subclassed by the user. So I would have thought that could be made to work, but but never mind, I can see you've gone a long way down this road already. The SPARC regression is very odd: I would have thought that if it's broken here it would also be broken in HeapByteBuffer.getLong(). Andrew.
Hi Andrew, Would you mind formally reviewing the patches? Including hotspot-dev as i have updated a hotspot test so should probably push to hs. I rebased to hs and fixed a silly bug in generated heap view buffers that were incorrectly calculating the unsafe byte offset: http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8151163-buffer-unsafe-unaligned... <http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8151163-buffer-unsafe-unaligned-access/webrev/> http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8151163-buffer-unsafe-unaligned... <http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8151163-buffer-unsafe-unaligned-access-hotspot/webrev/> JPRT runs for core and hotspot pass.
On 11 Jul 2016, at 18:34, Andrew Haley <aph@redhat.com> wrote:
On 11/07/16 17:04, Paul Sandoz wrote:
I opted to directly use Unsafe rather than defer. I am not sure it’s always possible to defer to the underlying buffer since its positional and order state is mutable.
Eww. The view has its own position, and the byte ordering is fixed when the view is created. The wrapped byte buffer is in a protected field, and the view is a package-private class so it can't be subclassed by the user. So I would have thought that could be made to work, but but never mind, I can see you've gone a long way down this road already.
Yes, and i laid some ground work in a previous fix to enable support for Unsafe double addressing mode on heap and direct buffers, with a grander plan to try and unify some of the implementations (careful performance analysis is required for direct buffers that access the base field whose value is null).
The SPARC regression is very odd: I would have thought that if it's broken here it would also be broken in HeapByteBuffer.getLong().
I did not get a chance to look at the generated code, so there could be a number of factors involved. My feeling is the three alignment checks are causing more issues on SPARC (perhaps combined less efficient addressing calculations [*], with loop unrolling), which led to the suggestion of a modifying the unaligned Unsafe methods to take an extra arg that is the constant offset to check alignment, thus could be hoisted out of a loop making constant strides. Paul. [*] some work on x86 was recently done by Roland to improve this area
participants (2)
-
Andrew Haley
-
Paul Sandoz