harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Regis <xu.re...@gmail.com>
Subject Re: call for review: (HARMONY-6257) [classlib][luni] - Optimize OSMemory.get/setByteArray
Date Fri, 10 Jul 2009 05:38:02 GMT
Jimmy,Jing Lv wrote:
> Hi,
> 
> 2009/7/9 Regis <xu.regis@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.

Mime
View raw message