On 04-02-2011 9:01 AM, Dave Bristor wrote:
The webrev looks fine. I have only this minor comment:
* Inflater.c:
A minor suggestion: In inflateBytes, cases Z_OK and Z_NEED_DICT, I suggest
replacing:
this_off += this_len - strm->avail_in;
(*env)->SetIntField(env, this, offID, this_off);
with:
(*env)->SetIntField(env, this, offID, this_len - strm->avail_in);
as this_off is not further used in the function.
It would be
(*env)->SetIntField(env, this, offID, this_off + this_len - strm->avail_in);
Personally I feel to do it in two steps makes it more clear (we are
updating this_off...) and it
is what it was before the change. So if you don't insist, I prefer to
just keep it asis.
Thanks,
-Sherman
Thanks!
Dave
--- On Fri, 4/1/11, Xueming Shen<[email protected]> wrote:
From: Xueming Shen<[email protected]>
Subject: Review Request for 6751338: ZIP inflater/deflater performance
To: "Dave Bristor"<[email protected]>, "BATEMAN,ALAN"<[email protected]>,
"core-libs-dev"<[email protected]>
Date: Friday, April 1, 2011, 4:04 PM
Dave, Alan,
Here is the final webrev based on Dave's patch and the
jdk1.5 code that does not
have the change for 6206933. JPRT job result suggests no
new testing failure and
my "non-scientific" benchmark test (to use
GZIPOu/InputStream to compress/
decompress the rt.jar) does show relative performance gain.
Will try to run more
tests the weekend, but here is the webrev.
http://cr.openjdk.java.net/~sherman/6751338/webrev/
Background Info:
This fix is basically to back out the fix for #6206933 we
made back to jdk5, which
is to use malloc+GetByteArrayuRegion to replace the
original GetPrimitiveArrayCritical/
ReleasePrimitiveArrayCritical() pair when access the java
byte array at native code
Inflater/Deflater.c, to mainly workaround the
GC/Critical... issue discussed in
#6186200.
The change for #6206933 itself has triggered lots of
performance issues
since its integration, some fixed, some still outstanding.
The GC rfe#6186200 has
been fixed long time ago, after couple weeks of
discussion/debating, we all agreed
that it's the time to back out#6206933.
-Sherman
