Review Request for 6751338: ZIP inflater/deflater performance

[Xueming Shen]

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<xueming.shen at oracle.com>  wrote:
>
>> From: Xueming Shen<xueming.shen at oracle.com>
>> Subject: Review Request for 6751338: ZIP inflater/deflater performance
>> To: "Dave Bristor"<bristor at yahoo.com>, "BATEMAN,ALAN"<alan.bateman at oracle.com>, "core-libs-dev"<core-libs-dev at openjdk.java.net>
>> 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
>>
>>