Tim Ellison wrote:
On 15/Sep/2009 04:26, Regis wrote:
Thanks for helping to review the patch.
Tim Ellison wrote:
On 07/Sep/2009 09:33, Regis wrote:
I managed to implement in this way and submit a patch to JIRA.
I tried to pass a object array, if it's direct buffer, fill the array
element with buffer directly (heap byte buffer without an array will be
copied to direct buffer first), otherwise, pass byte array. Because we
still need to know the offset of arrays, so I have to pass it to native
code.
Thanks Regis.
Looking at the patch "HARMONY-6328.v3.diff",
(1) This test is checking it is an instance of java.nio.ByteBuffer, but
it should be checking for instances of java.nio.DirectByteBuffer, right?
isDirectBuffer = (*env)->IsInstanceOf(env, buffer, byteBufferClass);
It's a little bit trick here. java.nio.DirectByteBuffer is not a
standard API, so I hesitated to use it here. And the elements of passed
in "buffers" must be direct buffer or a byte array, so it's safe to
check it is an instance of java.nio.ByteBuffer.
I see, but it would be clearer if it checked for DirectByteBuffer don't
you think? I know that is not a public type, but the natives are
certainly not limited to dealing with public elements (e.g. the call you
make earlier to getJavaIoFileDescriptorContentsAsAPointer()).
Agreed, I will change it.
(2) Not sure if any JNI implementations will care, but AIUI you only
should call ReleaseByteArrayElements if you got a copy of the array
(i.e. as indicated by the GetByteArrayElements).
I had the puzzle too, and did some searches, I found HARMONY-1634, not
sure it's just suitable for drlvm, but "The Get.. Release pair can be
used to prevent GC during the operation" seems reasonable, so I followed
it.
My point was that, AIUI, the Release should only be called if the Get
returned a copy, and you are not checking whether the Get returned a
copy or not. In practice, it probably doesn't matter anyway.
AIUI, If Get pinned java heap, the pinned array will be locked to prevent GC
collect or move it, Release will unlock it. So I think Release should be called
anyway, like we did in HARMONY-1634.
Regards,
Tim
I didn't study the SocketChannelImpl too closely, but it looks better
now :-) If you agree with (at leat) (1) above then we should apply the
patch and do some comparisons!
Regards,
Tim
--
Best Regards,
Regis.
