You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@harmony.apache.org by Regis <xu...@gmail.com> on 2009/07/06 11:27:25 UTC
call for review: (HARMONY-6257) [classlib][luni] - Optimize OSMemory.get/setByteArray
Regis Xu (JIRA) wrote:
> [ https://issues.apache.org/jira/browse/HARMONY-6257?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
>
> Regis Xu updated HARMONY-6257:
> ------------------------------
>
> Attachment: HARMONY-6257.diff
>
>> [classlib][luni] - Optimize OSMemory.get/setByteArray
>> -----------------------------------------------------
>>
>> Key: HARMONY-6257
>> URL: https://issues.apache.org/jira/browse/HARMONY-6257
>> Project: Harmony
>> Issue Type: Improvement
>> Components: Classlib
>> Affects Versions: 5.0M10
>> Reporter: Regis Xu
>> Attachments: HARMONY-6257.diff
>>
>>
>> in getByteArray, use SetByteArrayRegion to avoid memory copy from java to native
>> in setByteArray, Get/ReleasePrimitiveArrayCritical instead of GetByteArrayElements/ReleaseByteArrayElements to avoid memory copy.
>
I just found in OSMemory.c, get/setByteArray do some unnecessary memory copies
between Java and native.
In getByteArray, GetByteArrayElements copy data from java to native, and then
ReleaseByteArrayElements do the reverse, I think using SetByteArrayRegion is enough.
In setByteArray, I found JNI calls
GetPrimitiveArrayCritical/ReleasePrimitiveArrayCritical could get a pointer to
the primitive array without any copy, but seems they has some side effects (on
GC?), I don't have much confidence that they can apply here. I hope some one can
give some advices about this patch. Thanks.
--
Best Regards,
Regis.
Re: call for review: (HARMONY-6257) [classlib][luni] - Optimize
OSMemory.get/setByteArray
Posted by Regis <xu...@gmail.com>.
Pavel Pervov wrote:
> Hi, Regis.
>
> In Harmony VM Get/ReleasePrimitiveArrayCritical are implemented by
> pinning/unpinning array object (see
> working_vm/vm/vmcore/src/jni/jni.cpp, lines 1377 and 1415). The
> complexity of the operation depends on GC implementation. You can
> estimate Harmony VM times by looking at the implementation of
> gc_pin_object/gc_unpin_object under working_vm/vm/gc_gen/.
>
> WBR,
> Pavel.
>
I searched "gc_pin_object" in "working_vm/vm", seems there is only one
implementation in "working_vm/vm/gc_gen/src/common/gc_for_vm.cpp":
Boolean gc_is_object_pinned (Managed_Object_Handle obj)
{ return 0; }
void gc_pin_object (Managed_Object_Handle* p_object)
{ return; }
void gc_unpin_object (Managed_Object_Handle* p_object)
{ return; }
seems drlvm is not support pin object currently, so there is no difference
between Get/ReleasePrimitiveArrayCritical and
Get/Release<PrimitiveType>ArrayElements.
> On Mon, Aug 3, 2009 at 10:29 AM, Regis<xu...@gmail.com> wrote:
>> Regis wrote:
>>> Jimmy,Jing Lv wrote:
>>>> Hi,
>>>>
>>>> 2009/7/9 Regis <xu...@gmail.com>
>>>>
>>>>> Regis wrote:
>>>>>
>>>>>> Regis Xu (JIRA) wrote:
>>>>>>
>>>>>>> [
>>>>>>>
>>>>>>> https://issues.apache.org/jira/browse/HARMONY-6257?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel]
>>>>>>>
>>>>>>> Regis Xu updated HARMONY-6257:
>>>>>>> ------------------------------
>>>>>>>
>>>>>>> Attachment: HARMONY-6257.diff
>>>>>>>
>>>>>>> [classlib][luni] - Optimize OSMemory.get/setByteArray
>>>>>>>> -----------------------------------------------------
>>>>>>>>
>>>>>>>> Key: HARMONY-6257
>>>>>>>> URL: https://issues.apache.org/jira/browse/HARMONY-6257
>>>>>>>> Project: Harmony
>>>>>>>> Issue Type: Improvement
>>>>>>>> Components: Classlib
>>>>>>>> Affects Versions: 5.0M10
>>>>>>>> Reporter: Regis Xu
>>>>>>>> Attachments: HARMONY-6257.diff
>>>>>>>>
>>>>>>>>
>>>>>>>> in getByteArray, use SetByteArrayRegion to avoid memory copy from
>>>>>>>> java
>>>>>>>> to native
>>>>>>>> in setByteArray, Get/ReleasePrimitiveArrayCritical instead of
>>>>>>>> GetByteArrayElements/ReleaseByteArrayElements to avoid memory copy.
>>>>>>>>
>>>>>> I just found in OSMemory.c, get/setByteArray do some unnecessary memory
>>>>>> copies between Java and native.
>>>>>>
>>>>>> In getByteArray, GetByteArrayElements copy data from java to native,
>>>>>> and
>>>>>> then ReleaseByteArrayElements do the reverse, I think using
>>>>>> SetByteArrayRegion is enough.
>>>>>>
>>>> +1 for this fix
>>>>
>>>>
>>>>>> In setByteArray, I found JNI calls
>>>>>> GetPrimitiveArrayCritical/ReleasePrimitiveArrayCritical could get a
>>>>>> pointer
>>>>>> to the primitive array without any copy, but seems they has some side
>>>>>> effects (on GC?), I don't have much confidence that they can apply
>>>>>> here. I
>>>>>> hope some one can give some advices about this patch. Thanks.
>>>>>>
>>>>>>
>>> According to HARMONY-1634, ReleasePrimitiveArrayCritical should be called
>>> every time, whether "isCopy" is true or not. I'll update patch soon.
>>>
>>>> As I know, use this GetPrimitiveArrayCritical is something like entering
>>>> the
>>>> "critical region", the spec reads:
>>>>
>>>> After calling GetPrimitiveArrayCritical, the native code should not run
>>>> for
>>>> an extended period of time before it calls ReleasePrimitiveArrayCritical.
>>>> We
>>>> must treat the code inside this pair of functions as running in a
>>>> "critical
>>>> region." Inside a critical region, native code must not call other JNI
>>>> functions, or any system call that may cause the current thread to block
>>>> and
>>>> wait for another Java thread. (For example, the current thread must not
>>>> call
>>>> read on a stream being written by another Java thread.)
>>>>
>>>> thus it may potentially stall GC, as well as other jni critical section
>>>> is
>>> Maybe only critical section on the same primitive array would be blocked?
>>>
>>>> delayed, so this may cause another performance degradation.
>>> In this case, the critical section is very small, it should not be a
>>> problem, I think.
>>>
>>>> So the question is, how key is this improvement, and what benchmark show
>>>> its improvement? I am not sure, it may be in some environment/situations,
>>>> e.g. in some small heap environment, this modification may be a little
>>>> slower? We'd better do some more benchmarks?
>>> Yes, I have tested on a internal benchmark, I can't say much details, but
>>> it based on real complicated applications and heavily used
>>> DirectByteBuffer.get/put(). And after this patch, it boost 2-3%.
>>>
>>> It's also possible to write a micro benchmark, but as you said it's hard
>>> to simulate all the environment/situations.
>>>
>>>>
>>>>> The patch passed all luni and nio tests, if no one objected, I'd like to
>>>>> commit it.
>>>>>
>>>>> --
>>>>> Best Regards,
>>>>> Regis.
>>>>>
>>>>
>>>>
>>>
>> Can any vm gurus help to explain how vm implement
>> Get/ReleasePrimitiveArrayCritical and what does "critical region" here
>> exactly mean? So it would be clear whether Get/ReleasePrimitiveArrayCritical
>> is expensive here.
>> Thanks.
>>
>> --
>> Best Regards,
>> Regis.
>>
>
--
Best Regards,
Regis.
Re: call for review: (HARMONY-6257) [classlib][luni] - Optimize
OSMemory.get/setByteArray
Posted by Pavel Pervov <pm...@gmail.com>.
Hi, Regis.
In Harmony VM Get/ReleasePrimitiveArrayCritical are implemented by
pinning/unpinning array object (see
working_vm/vm/vmcore/src/jni/jni.cpp, lines 1377 and 1415). The
complexity of the operation depends on GC implementation. You can
estimate Harmony VM times by looking at the implementation of
gc_pin_object/gc_unpin_object under working_vm/vm/gc_gen/.
WBR,
Pavel.
On Mon, Aug 3, 2009 at 10:29 AM, Regis<xu...@gmail.com> wrote:
> Regis wrote:
>>
>> Jimmy,Jing Lv wrote:
>>>
>>> Hi,
>>>
>>> 2009/7/9 Regis <xu...@gmail.com>
>>>
>>>> Regis wrote:
>>>>
>>>>> Regis Xu (JIRA) wrote:
>>>>>
>>>>>> [
>>>>>>
>>>>>> https://issues.apache.org/jira/browse/HARMONY-6257?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel]
>>>>>>
>>>>>> Regis Xu updated HARMONY-6257:
>>>>>> ------------------------------
>>>>>>
>>>>>> Attachment: HARMONY-6257.diff
>>>>>>
>>>>>> [classlib][luni] - Optimize OSMemory.get/setByteArray
>>>>>>>
>>>>>>> -----------------------------------------------------
>>>>>>>
>>>>>>> Key: HARMONY-6257
>>>>>>> URL: https://issues.apache.org/jira/browse/HARMONY-6257
>>>>>>> Project: Harmony
>>>>>>> Issue Type: Improvement
>>>>>>> Components: Classlib
>>>>>>> Affects Versions: 5.0M10
>>>>>>> Reporter: Regis Xu
>>>>>>> Attachments: HARMONY-6257.diff
>>>>>>>
>>>>>>>
>>>>>>> in getByteArray, use SetByteArrayRegion to avoid memory copy from
>>>>>>> java
>>>>>>> to native
>>>>>>> in setByteArray, Get/ReleasePrimitiveArrayCritical instead of
>>>>>>> GetByteArrayElements/ReleaseByteArrayElements to avoid memory copy.
>>>>>>>
>>>>>>
>>>>> I just found in OSMemory.c, get/setByteArray do some unnecessary memory
>>>>> copies between Java and native.
>>>>>
>>>>> In getByteArray, GetByteArrayElements copy data from java to native,
>>>>> and
>>>>> then ReleaseByteArrayElements do the reverse, I think using
>>>>> SetByteArrayRegion is enough.
>>>>>
>>> +1 for this fix
>>>
>>>
>>>>> In setByteArray, I found JNI calls
>>>>> GetPrimitiveArrayCritical/ReleasePrimitiveArrayCritical could get a
>>>>> pointer
>>>>> to the primitive array without any copy, but seems they has some side
>>>>> effects (on GC?), I don't have much confidence that they can apply
>>>>> here. I
>>>>> hope some one can give some advices about this patch. Thanks.
>>>>>
>>>>>
>>
>> According to HARMONY-1634, ReleasePrimitiveArrayCritical should be called
>> every time, whether "isCopy" is true or not. I'll update patch soon.
>>
>>> As I know, use this GetPrimitiveArrayCritical is something like entering
>>> the
>>> "critical region", the spec reads:
>>>
>>> After calling GetPrimitiveArrayCritical, the native code should not run
>>> for
>>> an extended period of time before it calls ReleasePrimitiveArrayCritical.
>>> We
>>> must treat the code inside this pair of functions as running in a
>>> "critical
>>> region." Inside a critical region, native code must not call other JNI
>>> functions, or any system call that may cause the current thread to block
>>> and
>>> wait for another Java thread. (For example, the current thread must not
>>> call
>>> read on a stream being written by another Java thread.)
>>>
>>> thus it may potentially stall GC, as well as other jni critical section
>>> is
>>
>> Maybe only critical section on the same primitive array would be blocked?
>>
>>> delayed, so this may cause another performance degradation.
>>
>> In this case, the critical section is very small, it should not be a
>> problem, I think.
>>
>>> So the question is, how key is this improvement, and what benchmark show
>>> its improvement? I am not sure, it may be in some environment/situations,
>>> e.g. in some small heap environment, this modification may be a little
>>> slower? We'd better do some more benchmarks?
>>
>> Yes, I have tested on a internal benchmark, I can't say much details, but
>> it based on real complicated applications and heavily used
>> DirectByteBuffer.get/put(). And after this patch, it boost 2-3%.
>>
>> It's also possible to write a micro benchmark, but as you said it's hard
>> to simulate all the environment/situations.
>>
>>>
>>>
>>>> The patch passed all luni and nio tests, if no one objected, I'd like to
>>>> commit it.
>>>>
>>>> --
>>>> Best Regards,
>>>> Regis.
>>>>
>>>
>>>
>>>
>>
>>
>
> Can any vm gurus help to explain how vm implement
> Get/ReleasePrimitiveArrayCritical and what does "critical region" here
> exactly mean? So it would be clear whether Get/ReleasePrimitiveArrayCritical
> is expensive here.
> Thanks.
>
> --
> Best Regards,
> Regis.
>
Re: call for review: (HARMONY-6257) [classlib][luni] - Optimize
OSMemory.get/setByteArray
Posted by Regis <xu...@gmail.com>.
Ian Rogers wrote:
> 2009/8/2 Regis <xu...@gmail.com>:
>> Regis wrote:
>>> Jimmy,Jing Lv wrote:
>>>> Hi,
>>>>
>>>> 2009/7/9 Regis <xu...@gmail.com>
>>>>
>>>>> Regis wrote:
>>>>>
>>>>>> Regis Xu (JIRA) wrote:
>>>>>>
>>>>>>> [
>>>>>>>
>>>>>>> https://issues.apache.org/jira/browse/HARMONY-6257?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel]
>>>>>>>
>>>>>>> Regis Xu updated HARMONY-6257:
>>>>>>> ------------------------------
>>>>>>>
>>>>>>> Attachment: HARMONY-6257.diff
>>>>>>>
>>>>>>> [classlib][luni] - Optimize OSMemory.get/setByteArray
>>>>>>>> -----------------------------------------------------
>>>>>>>>
>>>>>>>> Key: HARMONY-6257
>>>>>>>> URL: https://issues.apache.org/jira/browse/HARMONY-6257
>>>>>>>> Project: Harmony
>>>>>>>> Issue Type: Improvement
>>>>>>>> Components: Classlib
>>>>>>>> Affects Versions: 5.0M10
>>>>>>>> Reporter: Regis Xu
>>>>>>>> Attachments: HARMONY-6257.diff
>>>>>>>>
>>>>>>>>
>>>>>>>> in getByteArray, use SetByteArrayRegion to avoid memory copy from
>>>>>>>> java
>>>>>>>> to native
>>>>>>>> in setByteArray, Get/ReleasePrimitiveArrayCritical instead of
>>>>>>>> GetByteArrayElements/ReleaseByteArrayElements to avoid memory copy.
>>>>>>>>
>>>>>> I just found in OSMemory.c, get/setByteArray do some unnecessary memory
>>>>>> copies between Java and native.
>>>>>>
>>>>>> In getByteArray, GetByteArrayElements copy data from java to native,
>>>>>> and
>>>>>> then ReleaseByteArrayElements do the reverse, I think using
>>>>>> SetByteArrayRegion is enough.
>>>>>>
>>>> +1 for this fix
>>>>
>>>>
>>>>>> In setByteArray, I found JNI calls
>>>>>> GetPrimitiveArrayCritical/ReleasePrimitiveArrayCritical could get a
>>>>>> pointer
>>>>>> to the primitive array without any copy, but seems they has some side
>>>>>> effects (on GC?), I don't have much confidence that they can apply
>>>>>> here. I
>>>>>> hope some one can give some advices about this patch. Thanks.
>>>>>>
>>>>>>
>>> According to HARMONY-1634, ReleasePrimitiveArrayCritical should be called
>>> every time, whether "isCopy" is true or not. I'll update patch soon.
>>>
>>>> As I know, use this GetPrimitiveArrayCritical is something like entering
>>>> the
>>>> "critical region", the spec reads:
>>>>
>>>> After calling GetPrimitiveArrayCritical, the native code should not run
>>>> for
>>>> an extended period of time before it calls ReleasePrimitiveArrayCritical.
>>>> We
>>>> must treat the code inside this pair of functions as running in a
>>>> "critical
>>>> region." Inside a critical region, native code must not call other JNI
>>>> functions, or any system call that may cause the current thread to block
>>>> and
>>>> wait for another Java thread. (For example, the current thread must not
>>>> call
>>>> read on a stream being written by another Java thread.)
>>>>
>>>> thus it may potentially stall GC, as well as other jni critical section
>>>> is
>>> Maybe only critical section on the same primitive array would be blocked?
>>>
>>>> delayed, so this may cause another performance degradation.
>>> In this case, the critical section is very small, it should not be a
>>> problem, I think.
>>>
>>>> So the question is, how key is this improvement, and what benchmark show
>>>> its improvement? I am not sure, it may be in some environment/situations,
>>>> e.g. in some small heap environment, this modification may be a little
>>>> slower? We'd better do some more benchmarks?
>>> Yes, I have tested on a internal benchmark, I can't say much details, but
>>> it based on real complicated applications and heavily used
>>> DirectByteBuffer.get/put(). And after this patch, it boost 2-3%.
>>>
>>> It's also possible to write a micro benchmark, but as you said it's hard
>>> to simulate all the environment/situations.
>>>
>>>>
>>>>> The patch passed all luni and nio tests, if no one objected, I'd like to
>>>>> commit it.
>>>>>
>>>>> --
>>>>> Best Regards,
>>>>> Regis.
>>>>>
>>>>
>>>>
>>>
>> Can any vm gurus help to explain how vm implement
>> Get/ReleasePrimitiveArrayCritical and what does "critical region" here
>> exactly mean? So it would be clear whether Get/ReleasePrimitiveArrayCritical
>> is expensive here.
>> Thanks.
>>
>> --
>> Best Regards,
>> Regis.
>>
>
> Hi Regis,
>
> Get/ReleasePrimitiveArrayCritical is a way of getting a direct pointer
> to an array. As the pointer can be used to update the array, until the
> release is performed, the VM may not move and do other garbage
> collection games with the array. The VM may choose to simply disable
> garbage collection to enable this operation to be performed. The
> corresponding JNI code in MRP is here [1].
>
> Regards,
> Ian
>
> [1] http://git.codehaus.org/gitweb.cgi?p=mrp.git;a=blob;f=rvm/src/org/jikesrvm/jni/JNIFunctions.java;h=17ed784ed22fc267775de5a6f9dcdac672a8608d;hb=HEAD#l4483
> --
> Metacircular Harmony with MRP - http://mrp.codehaus.org/
>
Thanks Ian, so "critical region" means region where GC will not touch the array
point, while different VMs may have differnet ways to handle this, is it right?
In this case, the region is just one memcpy call, disable GC in such short time
seems acceptable to me. And maybe some VMs could do more sophisticated things
(GC still work, but ignore this array point. Just guess, seems feasible), the
extra cost should be lower.
--
Best Regards,
Regis.
Re: call for review: (HARMONY-6257) [classlib][luni] - Optimize
OSMemory.get/setByteArray
Posted by Ian Rogers <ia...@manchester.ac.uk>.
2009/8/2 Regis <xu...@gmail.com>:
> Regis wrote:
>>
>> Jimmy,Jing Lv wrote:
>>>
>>> Hi,
>>>
>>> 2009/7/9 Regis <xu...@gmail.com>
>>>
>>>> Regis wrote:
>>>>
>>>>> Regis Xu (JIRA) wrote:
>>>>>
>>>>>> [
>>>>>>
>>>>>> https://issues.apache.org/jira/browse/HARMONY-6257?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel]
>>>>>>
>>>>>> Regis Xu updated HARMONY-6257:
>>>>>> ------------------------------
>>>>>>
>>>>>> Attachment: HARMONY-6257.diff
>>>>>>
>>>>>> [classlib][luni] - Optimize OSMemory.get/setByteArray
>>>>>>>
>>>>>>> -----------------------------------------------------
>>>>>>>
>>>>>>> Key: HARMONY-6257
>>>>>>> URL: https://issues.apache.org/jira/browse/HARMONY-6257
>>>>>>> Project: Harmony
>>>>>>> Issue Type: Improvement
>>>>>>> Components: Classlib
>>>>>>> Affects Versions: 5.0M10
>>>>>>> Reporter: Regis Xu
>>>>>>> Attachments: HARMONY-6257.diff
>>>>>>>
>>>>>>>
>>>>>>> in getByteArray, use SetByteArrayRegion to avoid memory copy from
>>>>>>> java
>>>>>>> to native
>>>>>>> in setByteArray, Get/ReleasePrimitiveArrayCritical instead of
>>>>>>> GetByteArrayElements/ReleaseByteArrayElements to avoid memory copy.
>>>>>>>
>>>>>>
>>>>> I just found in OSMemory.c, get/setByteArray do some unnecessary memory
>>>>> copies between Java and native.
>>>>>
>>>>> In getByteArray, GetByteArrayElements copy data from java to native,
>>>>> and
>>>>> then ReleaseByteArrayElements do the reverse, I think using
>>>>> SetByteArrayRegion is enough.
>>>>>
>>> +1 for this fix
>>>
>>>
>>>>> In setByteArray, I found JNI calls
>>>>> GetPrimitiveArrayCritical/ReleasePrimitiveArrayCritical could get a
>>>>> pointer
>>>>> to the primitive array without any copy, but seems they has some side
>>>>> effects (on GC?), I don't have much confidence that they can apply
>>>>> here. I
>>>>> hope some one can give some advices about this patch. Thanks.
>>>>>
>>>>>
>>
>> According to HARMONY-1634, ReleasePrimitiveArrayCritical should be called
>> every time, whether "isCopy" is true or not. I'll update patch soon.
>>
>>> As I know, use this GetPrimitiveArrayCritical is something like entering
>>> the
>>> "critical region", the spec reads:
>>>
>>> After calling GetPrimitiveArrayCritical, the native code should not run
>>> for
>>> an extended period of time before it calls ReleasePrimitiveArrayCritical.
>>> We
>>> must treat the code inside this pair of functions as running in a
>>> "critical
>>> region." Inside a critical region, native code must not call other JNI
>>> functions, or any system call that may cause the current thread to block
>>> and
>>> wait for another Java thread. (For example, the current thread must not
>>> call
>>> read on a stream being written by another Java thread.)
>>>
>>> thus it may potentially stall GC, as well as other jni critical section
>>> is
>>
>> Maybe only critical section on the same primitive array would be blocked?
>>
>>> delayed, so this may cause another performance degradation.
>>
>> In this case, the critical section is very small, it should not be a
>> problem, I think.
>>
>>> So the question is, how key is this improvement, and what benchmark show
>>> its improvement? I am not sure, it may be in some environment/situations,
>>> e.g. in some small heap environment, this modification may be a little
>>> slower? We'd better do some more benchmarks?
>>
>> Yes, I have tested on a internal benchmark, I can't say much details, but
>> it based on real complicated applications and heavily used
>> DirectByteBuffer.get/put(). And after this patch, it boost 2-3%.
>>
>> It's also possible to write a micro benchmark, but as you said it's hard
>> to simulate all the environment/situations.
>>
>>>
>>>
>>>> The patch passed all luni and nio tests, if no one objected, I'd like to
>>>> commit it.
>>>>
>>>> --
>>>> Best Regards,
>>>> Regis.
>>>>
>>>
>>>
>>>
>>
>>
>
> Can any vm gurus help to explain how vm implement
> Get/ReleasePrimitiveArrayCritical and what does "critical region" here
> exactly mean? So it would be clear whether Get/ReleasePrimitiveArrayCritical
> is expensive here.
> Thanks.
>
> --
> Best Regards,
> Regis.
>
Hi Regis,
Get/ReleasePrimitiveArrayCritical is a way of getting a direct pointer
to an array. As the pointer can be used to update the array, until the
release is performed, the VM may not move and do other garbage
collection games with the array. The VM may choose to simply disable
garbage collection to enable this operation to be performed. The
corresponding JNI code in MRP is here [1].
Regards,
Ian
[1] http://git.codehaus.org/gitweb.cgi?p=mrp.git;a=blob;f=rvm/src/org/jikesrvm/jni/JNIFunctions.java;h=17ed784ed22fc267775de5a6f9dcdac672a8608d;hb=HEAD#l4483
--
Metacircular Harmony with MRP - http://mrp.codehaus.org/
Re: call for review: (HARMONY-6257) [classlib][luni] - Optimize
OSMemory.get/setByteArray
Posted by Regis <xu...@gmail.com>.
Regis wrote:
> Jimmy,Jing Lv wrote:
>> Hi,
>>
>> 2009/7/9 Regis <xu...@gmail.com>
>>
>>> Regis wrote:
>>>
>>>> Regis Xu (JIRA) wrote:
>>>>
>>>>> [
>>>>> https://issues.apache.org/jira/browse/HARMONY-6257?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel]
>>>>>
>>>>>
>>>>> Regis Xu updated HARMONY-6257:
>>>>> ------------------------------
>>>>>
>>>>> Attachment: HARMONY-6257.diff
>>>>>
>>>>> [classlib][luni] - Optimize OSMemory.get/setByteArray
>>>>>> -----------------------------------------------------
>>>>>>
>>>>>> Key: HARMONY-6257
>>>>>> URL:
>>>>>> https://issues.apache.org/jira/browse/HARMONY-6257
>>>>>> Project: Harmony
>>>>>> Issue Type: Improvement
>>>>>> Components: Classlib
>>>>>> Affects Versions: 5.0M10
>>>>>> Reporter: Regis Xu
>>>>>> Attachments: HARMONY-6257.diff
>>>>>>
>>>>>>
>>>>>> in getByteArray, use SetByteArrayRegion to avoid memory copy from
>>>>>> java
>>>>>> to native
>>>>>> in setByteArray, Get/ReleasePrimitiveArrayCritical instead of
>>>>>> GetByteArrayElements/ReleaseByteArrayElements to avoid memory copy.
>>>>>>
>>>>>
>>>> I just found in OSMemory.c, get/setByteArray do some unnecessary memory
>>>> copies between Java and native.
>>>>
>>>> In getByteArray, GetByteArrayElements copy data from java to native,
>>>> and
>>>> then ReleaseByteArrayElements do the reverse, I think using
>>>> SetByteArrayRegion is enough.
>>>>
>> +1 for this fix
>>
>>
>>>> In setByteArray, I found JNI calls
>>>> GetPrimitiveArrayCritical/ReleasePrimitiveArrayCritical could get a
>>>> pointer
>>>> to the primitive array without any copy, but seems they has some side
>>>> effects (on GC?), I don't have much confidence that they can apply
>>>> here. I
>>>> hope some one can give some advices about this patch. Thanks.
>>>>
>>>>
>
> According to HARMONY-1634, ReleasePrimitiveArrayCritical should be
> called every time, whether "isCopy" is true or not. I'll update patch soon.
>
>> As I know, use this GetPrimitiveArrayCritical is something like
>> entering the
>> "critical region", the spec reads:
>>
>> After calling GetPrimitiveArrayCritical, the native code should not
>> run for
>> an extended period of time before it calls
>> ReleasePrimitiveArrayCritical. We
>> must treat the code inside this pair of functions as running in a
>> "critical
>> region." Inside a critical region, native code must not call other JNI
>> functions, or any system call that may cause the current thread to
>> block and
>> wait for another Java thread. (For example, the current thread must
>> not call
>> read on a stream being written by another Java thread.)
>>
>> thus it may potentially stall GC, as well as other jni critical
>> section is
>
> Maybe only critical section on the same primitive array would be blocked?
>
>> delayed, so this may cause another performance degradation.
>
> In this case, the critical section is very small, it should not be a
> problem, I think.
>
>> So the question is, how key is this improvement, and what benchmark show
>> its improvement? I am not sure, it may be in some environment/situations,
>> e.g. in some small heap environment, this modification may be a little
>> slower? We'd better do some more benchmarks?
>
> Yes, I have tested on a internal benchmark, I can't say much details,
> but it based on real complicated applications and heavily used
> DirectByteBuffer.get/put(). And after this patch, it boost 2-3%.
>
> It's also possible to write a micro benchmark, but as you said it's hard
> to simulate all the environment/situations.
>
>>
>>
>>> The patch passed all luni and nio tests, if no one objected, I'd like to
>>> commit it.
>>>
>>> --
>>> Best Regards,
>>> Regis.
>>>
>>
>>
>>
>
>
Can any vm gurus help to explain how vm implement
Get/ReleasePrimitiveArrayCritical and what does "critical region" here exactly
mean? So it would be clear whether Get/ReleasePrimitiveArrayCritical is
expensive here.
Thanks.
--
Best Regards,
Regis.
Re: call for review: (HARMONY-6257) [classlib][luni] - Optimize
OSMemory.get/setByteArray
Posted by Regis <xu...@gmail.com>.
Jimmy,Jing Lv wrote:
> Hi,
>
> 2009/7/9 Regis <xu...@gmail.com>
>
>> Regis wrote:
>>
>>> Regis Xu (JIRA) wrote:
>>>
>>>> [
>>>> https://issues.apache.org/jira/browse/HARMONY-6257?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel]
>>>>
>>>> Regis Xu updated HARMONY-6257:
>>>> ------------------------------
>>>>
>>>> Attachment: HARMONY-6257.diff
>>>>
>>>> [classlib][luni] - Optimize OSMemory.get/setByteArray
>>>>> -----------------------------------------------------
>>>>>
>>>>> Key: HARMONY-6257
>>>>> URL: https://issues.apache.org/jira/browse/HARMONY-6257
>>>>> Project: Harmony
>>>>> Issue Type: Improvement
>>>>> Components: Classlib
>>>>> Affects Versions: 5.0M10
>>>>> Reporter: Regis Xu
>>>>> Attachments: HARMONY-6257.diff
>>>>>
>>>>>
>>>>> in getByteArray, use SetByteArrayRegion to avoid memory copy from java
>>>>> to native
>>>>> in setByteArray, Get/ReleasePrimitiveArrayCritical instead of
>>>>> GetByteArrayElements/ReleaseByteArrayElements to avoid memory copy.
>>>>>
>>>>
>>> I just found in OSMemory.c, get/setByteArray do some unnecessary memory
>>> copies between Java and native.
>>>
>>> In getByteArray, GetByteArrayElements copy data from java to native, and
>>> then ReleaseByteArrayElements do the reverse, I think using
>>> SetByteArrayRegion is enough.
>>>
> +1 for this fix
>
>
>>> In setByteArray, I found JNI calls
>>> GetPrimitiveArrayCritical/ReleasePrimitiveArrayCritical could get a pointer
>>> to the primitive array without any copy, but seems they has some side
>>> effects (on GC?), I don't have much confidence that they can apply here. I
>>> hope some one can give some advices about this patch. Thanks.
>>>
>>>
According to HARMONY-1634, ReleasePrimitiveArrayCritical should be called every
time, whether "isCopy" is true or not. I'll update patch soon.
> As I know, use this GetPrimitiveArrayCritical is something like entering the
> "critical region", the spec reads:
>
> After calling GetPrimitiveArrayCritical, the native code should not run for
> an extended period of time before it calls ReleasePrimitiveArrayCritical. We
> must treat the code inside this pair of functions as running in a "critical
> region." Inside a critical region, native code must not call other JNI
> functions, or any system call that may cause the current thread to block and
> wait for another Java thread. (For example, the current thread must not call
> read on a stream being written by another Java thread.)
>
> thus it may potentially stall GC, as well as other jni critical section is
Maybe only critical section on the same primitive array would be blocked?
> delayed, so this may cause another performance degradation.
In this case, the critical section is very small, it should not be a problem, I
think.
> So the question is, how key is this improvement, and what benchmark show
> its improvement? I am not sure, it may be in some environment/situations,
> e.g. in some small heap environment, this modification may be a little
> slower? We'd better do some more benchmarks?
Yes, I have tested on a internal benchmark, I can't say much details, but it
based on real complicated applications and heavily used
DirectByteBuffer.get/put(). And after this patch, it boost 2-3%.
It's also possible to write a micro benchmark, but as you said it's hard to
simulate all the environment/situations.
>
>
>> The patch passed all luni and nio tests, if no one objected, I'd like to
>> commit it.
>>
>> --
>> Best Regards,
>> Regis.
>>
>
>
>
--
Best Regards,
Regis.
Re: call for review: (HARMONY-6257) [classlib][luni] - Optimize
OSMemory.get/setByteArray
Posted by "Jimmy,Jing Lv" <fi...@gmail.com>.
Hi,
2009/7/9 Regis <xu...@gmail.com>
> Regis wrote:
>
>> Regis Xu (JIRA) wrote:
>>
>>> [
>>> https://issues.apache.org/jira/browse/HARMONY-6257?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel]
>>>
>>> Regis Xu updated HARMONY-6257:
>>> ------------------------------
>>>
>>> Attachment: HARMONY-6257.diff
>>>
>>> [classlib][luni] - Optimize OSMemory.get/setByteArray
>>>> -----------------------------------------------------
>>>>
>>>> Key: HARMONY-6257
>>>> URL: https://issues.apache.org/jira/browse/HARMONY-6257
>>>> Project: Harmony
>>>> Issue Type: Improvement
>>>> Components: Classlib
>>>> Affects Versions: 5.0M10
>>>> Reporter: Regis Xu
>>>> Attachments: HARMONY-6257.diff
>>>>
>>>>
>>>> in getByteArray, use SetByteArrayRegion to avoid memory copy from java
>>>> to native
>>>> in setByteArray, Get/ReleasePrimitiveArrayCritical instead of
>>>> GetByteArrayElements/ReleaseByteArrayElements to avoid memory copy.
>>>>
>>>
>>>
>> I just found in OSMemory.c, get/setByteArray do some unnecessary memory
>> copies between Java and native.
>>
>> In getByteArray, GetByteArrayElements copy data from java to native, and
>> then ReleaseByteArrayElements do the reverse, I think using
>> SetByteArrayRegion is enough.
>>
>
+1 for this fix
>
>> In setByteArray, I found JNI calls
>> GetPrimitiveArrayCritical/ReleasePrimitiveArrayCritical could get a pointer
>> to the primitive array without any copy, but seems they has some side
>> effects (on GC?), I don't have much confidence that they can apply here. I
>> hope some one can give some advices about this patch. Thanks.
>>
>>
As I know, use this GetPrimitiveArrayCritical is something like entering the
"critical region", the spec reads:
After calling GetPrimitiveArrayCritical, the native code should not run for
an extended period of time before it calls ReleasePrimitiveArrayCritical. We
must treat the code inside this pair of functions as running in a "critical
region." Inside a critical region, native code must not call other JNI
functions, or any system call that may cause the current thread to block and
wait for another Java thread. (For example, the current thread must not call
read on a stream being written by another Java thread.)
thus it may potentially stall GC, as well as other jni critical section is
delayed, so this may cause another performance degradation.
So the question is, how key is this improvement, and what benchmark show
its improvement? I am not sure, it may be in some environment/situations,
e.g. in some small heap environment, this modification may be a little
slower? We'd better do some more benchmarks?
>
> The patch passed all luni and nio tests, if no one objected, I'd like to
> commit it.
>
> --
> Best Regards,
> Regis.
>
--
Best Regards!
Jimmy, Jing Lv
China Software Development Lab, IBM
Re: call for review: (HARMONY-6257) [classlib][luni] - Optimize OSMemory.get/setByteArray
Posted by Regis <xu...@gmail.com>.
Regis wrote:
> Regis Xu (JIRA) wrote:
>> [
>> https://issues.apache.org/jira/browse/HARMONY-6257?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
>> ]
>>
>> Regis Xu updated HARMONY-6257:
>> ------------------------------
>>
>> Attachment: HARMONY-6257.diff
>>
>>> [classlib][luni] - Optimize OSMemory.get/setByteArray
>>> -----------------------------------------------------
>>>
>>> Key: HARMONY-6257
>>> URL: https://issues.apache.org/jira/browse/HARMONY-6257
>>> Project: Harmony
>>> Issue Type: Improvement
>>> Components: Classlib
>>> Affects Versions: 5.0M10
>>> Reporter: Regis Xu
>>> Attachments: HARMONY-6257.diff
>>>
>>>
>>> in getByteArray, use SetByteArrayRegion to avoid memory copy from
>>> java to native
>>> in setByteArray, Get/ReleasePrimitiveArrayCritical instead of
>>> GetByteArrayElements/ReleaseByteArrayElements to avoid memory copy.
>>
>
> I just found in OSMemory.c, get/setByteArray do some unnecessary memory
> copies between Java and native.
>
> In getByteArray, GetByteArrayElements copy data from java to native, and
> then ReleaseByteArrayElements do the reverse, I think using
> SetByteArrayRegion is enough.
>
> In setByteArray, I found JNI calls
> GetPrimitiveArrayCritical/ReleasePrimitiveArrayCritical could get a
> pointer to the primitive array without any copy, but seems they has some
> side effects (on GC?), I don't have much confidence that they can apply
> here. I hope some one can give some advices about this patch. Thanks.
>
The patch passed all luni and nio tests, if no one objected, I'd like to commit it.
--
Best Regards,
Regis.