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()). >> (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. 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 >> > >
