Re: call for review: (HARMONY-6257) [classlib][luni] - Optimize OSMemory.get/setByteArray

[Regis]

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.re...@gmail.com> wrote:
Regis wrote:
Jimmy,Jing Lv wrote:
Hi,

2009/7/9 Regis <xu.re...@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.