Fix alignment issue on old ART runtime.

Fix alignment issue on old ART runtime.

Fixes #147

BUG=https://github.com/domokit/mojo/issues/147
R=jamesr@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/9c93cfa7fc480104701a53ab4514cf725ef6c5d4

[jamesr, 2015-05-12 17:26:15]

https://codereview.chromium.org/1127313006/diff/40001/mojo/android/system/src...
File mojo/android/system/src/org/chromium/mojo/system/impl/CoreImpl.java
(right):

https://codereview.chromium.org/1127313006/diff/40001/mojo/android/system/src...
mojo/android/system/src/org/chromium/mojo/system/impl/CoreImpl.java:77:
mByteBufferOffset = nativeGetNativeBufferOffset(ByteBuffer.allocateDirect(8),
8);
if I understand correctly this does one 8-byte allocation at startup and checks
if it is aligned, correct? Is the alignment of this initial allocation always a
good predictor of alignment of future allocations?

[qsr, 2015-05-13 07:51:11]

On 2015/05/12 17:26:15, jamesr wrote:
>
https://codereview.chromium.org/1127313006/diff/40001/mojo/android/system/src...
> File mojo/android/system/src/org/chromium/mojo/system/impl/CoreImpl.java
> (right):
> 
>
https://codereview.chromium.org/1127313006/diff/40001/mojo/android/system/src...
> mojo/android/system/src/org/chromium/mojo/system/impl/CoreImpl.java:77:
> mByteBufferOffset = nativeGetNativeBufferOffset(ByteBuffer.allocateDirect(8),
> 8);
> if I understand correctly this does one 8-byte allocation at startup and
checks
> if it is aligned, correct? Is the alignment of this initial allocation always
a
> good predictor of alignment of future allocations?

 Not exactly. It retrieves how much not aligned it is, and use that for all
subsequent allocation. So this will be fine if it is aligned, and will also be
fined if it is always unaligned the same way. The last part depends on the
behavior of the native allocator for ART, and it is not clear what it is, so
this is mostly sending this in the wild and see if it helps... Also if this
doesn't work, then I don't see any way to get the information reliably anyway.

[jamesr, 2015-05-13 19:52:59]

In general I think this is a pretty poor strategy but looking at that bugfix I
think this might work for that particular bug.  In general if an allocator does
not have an alignment contract it's probably not a good idea to assume all
allocations have the same alignment.

Can you add some version guards around this so we don't pay this cost on
non-buggy VMs?  Since this is specific to early ARTs we should be able to avoid
running this on dalvik (in practice that probably means all K) or on newer ARTs
(i.e. anything +L).  Can you get the ART version in any reasonable way?

[qsr, 2015-05-18 07:32:24]

On 2015/05/13 19:52:59, jamesr wrote:
> In general I think this is a pretty poor strategy but looking at that bugfix I
> think this might work for that particular bug.  In general if an allocator
does
> not have an alignment contract it's probably not a good idea to assume all
> allocations have the same alignment.

 I agree with you in general. But there is no good perfect solution here, and we
are speaking about a workaround anyway.

> Can you add some version guards around this so we don't pay this cost on
> non-buggy VMs?  Since this is specific to early ARTs we should be able to
avoid
> running this on dalvik (in practice that probably means all K) or on newer
ARTs
> (i.e. anything +L).  Can you get the ART version in any reasonable way?

 Depends what you call version guards. In java everything will be dynamic. Find
out the version will mean getting a system property and doing string parsing,
which will be more costly that the single jni call that this code is doing. So I
don't think this is worth it. What I can do though, is checking before doing the
slice() operation so that I do not allocate a new object if this is not
necessary.

[jamesr, 2015-05-21 21:43:13]

lgtm

https://codereview.chromium.org/1127313006/diff/60001/mojo/android/system/src...
File mojo/android/system/src/org/chromium/mojo/system/impl/CoreImpl.java
(right):

https://codereview.chromium.org/1127313006/diff/60001/mojo/android/system/src...
mojo/android/system/src/org/chromium/mojo/system/impl/CoreImpl.java:77:
mByteBufferOffset = nativeGetNativeBufferOffset(ByteBuffer.allocateDirect(8),
8);
please document what's going on here (link to the issue, explanation of why this
is assuming alignment will be consistent)

[qsr, 2015-05-22 12:45:06]

Committed patchset #5 (id:80001) manually as
9c93cfa7fc480104701a53ab4514cf725ef6c5d4 (presubmit successful).

[qsr, 2015-05-22 12:53:31]

https://codereview.chromium.org/1127313006/diff/60001/mojo/android/system/src...
File mojo/android/system/src/org/chromium/mojo/system/impl/CoreImpl.java
(right):

https://codereview.chromium.org/1127313006/diff/60001/mojo/android/system/src...
mojo/android/system/src/org/chromium/mojo/system/impl/CoreImpl.java:77:
mByteBufferOffset = nativeGetNativeBufferOffset(ByteBuffer.allocateDirect(8),
8);
On 2015/05/21 21:43:13, jamesr wrote:
> please document what's going on here (link to the issue, explanation of why
this
> is assuming alignment will be consistent)

Done.

https://codereview.chromium.org/1127313006/diff/60001/mojo/android/system/src...
mojo/android/system/src/org/chromium/mojo/system/impl/CoreImpl.java:77:
mByteBufferOffset = nativeGetNativeBufferOffset(ByteBuffer.allocateDirect(8),
8);
On 2015/05/21 21:43:13, jamesr wrote:
> please document what's going on here (link to the issue, explanation of why
this
> is assuming alignment will be consistent)

Done.