Mailing-List: contact dev-help@harmony.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@harmony.apache.org
Received-SPF: pass (nike.apache.org: domain of xu.regis@gmail.com designates
 209.85.211.202 as permitted sender)
DomainKey-Signature: a=rsa-sha1; c=nofws;
        d=gmail.com; s=gamma;
        h=message-id:date:from:user-agent:mime-version:to:subject:references
         :in-reply-to:content-type:content-transfer-encoding;
        b=XWkv+Wq8Ua4GB+BHJ1MUKWbFxq+/W5u0+9NYlEcK8Npe3psBsnrbAn5qDOjpOshcv7
         g6u2kPe3tW1PmYUYSuB8jxUT6JwoOX9epe7qUCMKoJtxnpkkSdMLrLJbznGPGBM9CtxJ
         6piTexBXkjPkYmhyk/b7E1Kn2/l734FtTVRgc=
Message-ID: <4AAF5592.30402@gmail.com>
Date: Tue, 15 Sep 2009 16:51:30 +0800
From: Regis <xu.regis@gmail.com>
User-Agent: Thunderbird 2.0.0.23 (Windows/20090812)
MIME-Version: 1.0
To: dev@harmony.apache.org
Subject: Re: [jira] Created: (HARMONY-6328) [classlib][nio] optimize
 SocketChannel.write(ByteBuffer[],
 int, int) by writev
References: <290644005.1251857072710.JavaMail.jira@brutus>
 <4A9F8871.4020605@gmail.com> <4A9F9976.7010003@gmail.com>
 <4A9FA881.5020402@gmail.com> <4A9FB1A9.3060803@gmail.com>
 <4AA4C556.3080808@gmail.com> <4AAE6786.4020809@gmail.com>
 <4AAF0963.7030109@gmail.com> <4AAF4B66.5070608@gmail.com>
In-Reply-To: <4AAF4B66.5070608@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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.