Early version of striding Deflater
Hello again, I've finished an early version of the java.util.zip.Deflater implementation that uses striding. Its in an early stage and quite likely will be buggy. It passes FlaterTest and a simple test written by myself, but maybe acts differently in corner-cases. I would be happy to receive some comments as well as criticism ;) lg Clemens PS: Sorry for the traffic lately.
Hi Clemens, Thanks for the code drop! I was not yet able to spend much Quality Time with it. At a glance, it seems like the fix might work. The general ideas seem sound, and I appreciate your effort in addressing it. So please don't take the feedback below as anything but encouragement to help get this bug fixed! That said...it is a low priority bug (for us, anyway; a P4 out of 5) and we have bigger fish to fry. I'll attend to it as time permits... Then too, there are some issues to resolve re the provided code. I know it's an "Early version", but some changes to it would make it easier to examine. For example, Deflater.java was completely reformatted. When diffing the code, every changeof indentation, javadoc removal, spaces to tabs, moving of {, etc. shows up. We don't allow such changes into the JDK sources. Spaces only, and despite whatever awful "standards" (or not!) already in use in the file, please stick to them. Make every change be the best/smallest one which directly addresses the bug. This makes it easier for all concerned to examine the relevant changes. There are similar issues in Deflater.c. The files provided the lack the GNU copyright file headers. My guess is that they originated in the src.zip of a binary distribution. Regardless of their source, could you please instead use files from mercurial repository at hg.openjdk.java.net/jdk7/tl? The above will make it easier to review future changes to the files. Enough of that boring stuff, here's some feedback on "interesting" part of the changes! In Deflater.java, new method rangeCheck() is used in a couple of places, but it is not an adequate replacement for the code previously in Deflater.setDictionary(); it incompatibly changes the error condition checking semantics (we can't omit the check on strm). We strive to not introduce incompatible changes, even small ones like this. Another incompatible change is to the semantics of Deflater.deflate()...I think. In Deflater.c, it seems that deflateBytes will use setParams if necessary and then compress...I reference your email about 6539727 in this regard (You are completely right about that being a non-bug, BTW and I'll update it shortly: thanks!) I think your solution would have been the Right Thing to do way back when, but we don't want to make an incompatible change now. (I haven't reviewed this thoroughly enough; see my notes re formatting & priorities above.) With a change of this sort, we really do need tests along with a fix. Have you started writing any test cases? Finally, it seems that this solution obviates the need for the striding in DeflaterOutputStream...IIRC, that is some of the original motivation for this work. If you have suggested changes to that class as well, please include them. I appreciate the work you've put in, and again, I hope to not dissuade you. But we have certain standards to which we must adhere, and it's a not a very high priority for us now, so we have to minimize the time we spend on it. Thanks, Dave Clemens Eisserer wrote:
Hello again,
I've finished an early version of the java.util.zip.Deflater implementation that uses striding. Its in an early stage and quite likely will be buggy. It passes FlaterTest and a simple test written by myself, but maybe acts differently in corner-cases.
I would be happy to receive some comments as well as criticism ;)
lg Clemens
PS: Sorry for the traffic lately.
Hi Dave, Thanks a lot for your reply. To make it short: Of course I understand that this is low-priority (also for me, its a fun-only fix because someone in forums.java.net mentioned it) so don't hurry. Sorry that I wasted your time with my messy files, they were taken from my "playground" thats why they were in such a bad shape - they were only intended to give an idea which "road" I was taking. I attached the new files taken from the mercurial repositories and only modified at the affected places.
With a change of this sort, we really do need tests along with a fix. Have you started writing any test cases? I completly agree - I have some simple test-cases which test more or less only very basic functionality of Deflater and they work well (also FlatterTest passes). I'll write some more tests which test exotic use-cases like changing compression-level, ... during compression.
I have some open questions: 1.) Is the seperate structure approach to hold the stride-buffers ok? 2.) Any suggestions for the following names: 1. strm-field in class (defAdr), 2. defAdr-parameter,3. defptr - long_to_ptr of defAdr, 4. def_data - name of the structure 3.) I am not really used to program in C. Are the adress-operations ok which I used to get members of the new struct def_data? Thanks for your patience, lg Clemens Some notes, and changes in ramdom order: * Changed deflate-bytes to the old behaviour to return after the call to deflateParams * Verified that its ok to call deflateParams when there's not enough space in the output-buffer to flush all "old" data out (thanks to Mark Adler) * I changed the method-signiture of the native method compared to original, because some variables were read from JNI-code, whereas they could have been passed simply down using method parameters. I think its "cleaner" to pass it. * Allocation of the stride-buffers together with the z_stream structure. z_stream is really large, so the two stride-buffers should not add that much overhead. However this has the advantage of not mallocing/freeing and also beeing able to fill the input-stride-buffer once for several calls of the native method. * Renamed the strm-adress-parameter to defadr, because it no longer really points to a strm. I did not rename the java field "strm" because I did not have an idea for a proper name. * Removed striding from DeflaterOutputStream, (looked how code looked in 1.4.2).
Hi Clemens, Here's some file-by-file feedback, answers to questions, etc. I've attached 2 files: * Deflater.c.reformat is by-and-large the same as the file that you sent, except that it compiles on Solaris without warnings (it wouldn't compile there w/o change; perhaps the linux compiler (I'm guessing) you used is more lenient), and is formatted more in keeping with the rest of the file's style. * Deflater.c has some further changes on my part, described below. I've run all our regression tests on this one and it passes. I haven't run JCK tests, nor our more extensive performance suite. File-by-file commentary: *** Deflate.c Doesn't compile, at least not on Solaris. Several warnings. I fixed them. See attached Deflater.c.reformat. (I have not tried compiling on other platforms.) Some stylistic issues need attention; see e.g. deflateBytes for brace positioning, trailing whitespace, space between keyword and parens. Should document fields in def_data (compare with zip_util.h). Some field IDs no longer necessary, since they're passed in as params. I removed them. There's a slight change to the semantics of "finished". Current code sets Deflater.finished only if setParams is false. Changed code may set it regardlewss of setParams. I suspect this is OK: if client code changed strategy or level and called Deflater.deflate(), it would invoke deflateParams(), and not alter the value of finished. The client's subsequent call to Deflater.deflate() would call deflate() which AFAICT would cause finished to be set. What do you think? Why fall through from Z_OK to Z_BUF_ERROR? In Z_BUF_ERROR and default cases, why continue execution of loop instead of returning 0 as does original code? I changed this; see attached Deflater.c *** Deflater.java: OK *** DeflaterOutputStream.java: OK More inline below. Clemens Eisserer wrote:
Hi Dave,
Thanks a lot for your reply. To make it short: Of course I understand that this is low-priority (also for me, its a fun-only fix because someone in forums.java.net mentioned it) so don't hurry. Sorry that I wasted your time with my messy files, they were taken from my "playground" thats why they were in such a bad shape - they were only intended to give an idea which "road" I was taking. I attached the new files taken from the mercurial repositories and only modified at the affected places.
With a change of this sort, we really do need tests along with a fix. Have you started writing any test cases? I completly agree - I have some simple test-cases which test more or less only very basic functionality of Deflater and they work well (also FlatterTest passes). I'll write some more tests which test exotic use-cases like changing compression-level, ... during compression.
Great, thanks. It would be a Good Idea to have a test that checks my assumption re finished (see above).
I have some open questions: 1.) Is the seperate structure approach to hold the stride-buffers ok?
I think so.
2.) Any suggestions for the following names: 1. strm-field in class (defAdr), 2. defAdr-parameter,3. defptr - long_to_ptr of defAdr, 4. def_data - name of the structure
Those don't quite match what I see in the code; but what's in the code seems OK: def_data for the struct, def_adr as a param to deflateBytes, etc. etc. defptr to reference a def_data in init and deflateBytes
3.) I am not really used to program in C. Are the adress-operations ok which I used to get members of the new struct def_data?
It seems OK.
Thanks for your patience, lg Clemens
Some notes, and changes in ramdom order: * Changed deflate-bytes to the old behaviour to return after the call to deflateParams
Good; AFAICT at maintains the existing semantics.
* Verified that its ok to call deflateParams when there's not enough space in the output-buffer to flush all "old" data out (thanks to Mark Adler) * I changed the method-signiture of the native method compared to original, because some variables were read from JNI-code, whereas they could have been passed simply down using method parameters. I think its "cleaner" to pass it.
The long argument to deflateBytes is a bit cumbersome, but Ken Russell opined that it provides better performance, so it's a Good Thing:
Kenneth Russell wrote: I strongly agree with the contributor's suggestion. Not only is passing the argument from Java less code, it is also faster since field access from Java can be optimized by the HotSpot compiler, where field accesses through the JNI must go through the same set of boilerplate C/C++/assembly every time.
* Allocation of the stride-buffers together with the z_stream structure. z_stream is really large, so the two stride-buffers should not add that much overhead. However this has the advantage of not mallocing/freeing and also beeing able to fill the input-stride-buffer once for several calls of the native method.
Looks good.
* Renamed the strm-adress-parameter to defadr, because it no longer really points to a strm. I did not rename the java field "strm" because I did not have an idea for a proper name.
It should have had a different name from day one. I'm slightly loathe to make a name change, nor do I have (Friday afternoon) a Really Good Idea.
* Removed striding from DeflaterOutputStream, (looked how code looked in 1.4.2).
Looks good. From your other email:
I also thought about implementing striding in the CRC32/Adler32 classes which basically suffer from the same block-the-gc behaviour as Inflater/Deflater did before they were "fixed" ;)
I suspect these are not as much of an issue: presumably CRC32 and adler32 calculations are faster than deflation (caveat: I haven't measured them). Other have pointed out / shamed us because, hey, shouldn't these be in Java anyway?
Furthermore do you have good ideas for regression tests? The usual compression/decompression works fine, can you imagine corner-cases which would be worth special testing? Should the tests written in the jtreg format?
I'd like to see tests where there's a possibility of semantics having changed; as noted above re "finished". I haven't done a performance analysis, but don't expect a regression. If anything, since the striding is kept within native code, there should be fewer Java -> native calls, and better performance (though that is perhaps not measurable). My last comment is about this change in general. It seems like a reasonable fix, though the corresponding bug: http://bugs.sun.com/view_bug.do?bug_id=6399199 is a low priority one for us, and this is code we generally feel is best left alone when possible. We are still learning how best to work with contributions from outside of Sun. I will check with other who've maintained this code in the past, to get their opinion of making this kind of change. While I'm currently responsible for jar/zip code, it's only one of the hats I currently wear ;-) Thanks, Dave
Here are the attached files that I forgot to attach ;-( Dave Dave Bristor wrote:
Hi Clemens,
Here's some file-by-file feedback, answers to questions, etc. I've attached 2 files: * Deflater.c.reformat is by-and-large the same as the file that you sent, except that it compiles on Solaris without warnings (it wouldn't compile there w/o change; perhaps the linux compiler (I'm guessing) you used is more lenient), and is formatted more in keeping with the rest of the file's style.
* Deflater.c has some further changes on my part, described below. I've run all our regression tests on this one and it passes. I haven't run JCK tests, nor our more extensive performance suite.
File-by-file commentary:
*** Deflate.c
Doesn't compile, at least not on Solaris. Several warnings. I fixed them. See attached Deflater.c.reformat. (I have not tried compiling on other platforms.)
Some stylistic issues need attention; see e.g. deflateBytes for brace positioning, trailing whitespace, space between keyword and parens.
Should document fields in def_data (compare with zip_util.h).
Some field IDs no longer necessary, since they're passed in as params. I removed them.
There's a slight change to the semantics of "finished". Current code sets Deflater.finished only if setParams is false. Changed code may set it regardlewss of setParams. I suspect this is OK: if client code changed strategy or level and called Deflater.deflate(), it would invoke deflateParams(), and not alter the value of finished. The client's subsequent call to Deflater.deflate() would call deflate() which AFAICT would cause finished to be set. What do you think?
Why fall through from Z_OK to Z_BUF_ERROR? In Z_BUF_ERROR and default cases, why continue execution of loop instead of returning 0 as does original code? I changed this; see attached Deflater.c
*** Deflater.java: OK
*** DeflaterOutputStream.java: OK
More inline below.
Clemens Eisserer wrote:
Hi Dave,
Thanks a lot for your reply. To make it short: Of course I understand that this is low-priority (also for me, its a fun-only fix because someone in forums.java.net mentioned it) so don't hurry. Sorry that I wasted your time with my messy files, they were taken from my "playground" thats why they were in such a bad shape - they were only intended to give an idea which "road" I was taking. I attached the new files taken from the mercurial repositories and only modified at the affected places.
With a change of this sort, we really do need tests along with a fix. Have you started writing any test cases? I completly agree - I have some simple test-cases which test more or less only very basic functionality of Deflater and they work well (also FlatterTest passes). I'll write some more tests which test exotic use-cases like changing compression-level, ... during compression.
Great, thanks. It would be a Good Idea to have a test that checks my assumption re finished (see above).
I have some open questions: 1.) Is the seperate structure approach to hold the stride-buffers ok?
I think so.
2.) Any suggestions for the following names: 1. strm-field in class (defAdr), 2. defAdr-parameter,3. defptr - long_to_ptr of defAdr, 4. def_data - name of the structure
Those don't quite match what I see in the code; but what's in the code seems OK: def_data for the struct, def_adr as a param to deflateBytes, etc. etc. defptr to reference a def_data in init and deflateBytes
3.) I am not really used to program in C. Are the adress-operations ok which I used to get members of the new struct def_data?
It seems OK.
Thanks for your patience, lg Clemens
Some notes, and changes in ramdom order: * Changed deflate-bytes to the old behaviour to return after the call to deflateParams
Good; AFAICT at maintains the existing semantics.
* Verified that its ok to call deflateParams when there's not enough space in the output-buffer to flush all "old" data out (thanks to Mark Adler) * I changed the method-signiture of the native method compared to original, because some variables were read from JNI-code, whereas they could have been passed simply down using method parameters. I think its "cleaner" to pass it.
The long argument to deflateBytes is a bit cumbersome, but Ken Russell opined that it provides better performance, so it's a Good Thing:
Kenneth Russell wrote: I strongly agree with the contributor's suggestion. Not only is passing the argument from Java less code, it is also faster since field access from Java can be optimized by the HotSpot compiler, where field accesses through the JNI must go through the same set of boilerplate C/C++/assembly every time.
* Allocation of the stride-buffers together with the z_stream structure. z_stream is really large, so the two stride-buffers should not add that much overhead. However this has the advantage of not mallocing/freeing and also beeing able to fill the input-stride-buffer once for several calls of the native method.
Looks good.
* Renamed the strm-adress-parameter to defadr, because it no longer really points to a strm. I did not rename the java field "strm" because I did not have an idea for a proper name.
It should have had a different name from day one. I'm slightly loathe to make a name change, nor do I have (Friday afternoon) a Really Good Idea.
* Removed striding from DeflaterOutputStream, (looked how code looked in 1.4.2).
Looks good.
From your other email:
I also thought about implementing striding in the CRC32/Adler32 classes which basically suffer from the same block-the-gc behaviour as Inflater/Deflater did before they were "fixed" ;)
I suspect these are not as much of an issue: presumably CRC32 and adler32 calculations are faster than deflation (caveat: I haven't measured them). Other have pointed out / shamed us because, hey, shouldn't these be in Java anyway?
Furthermore do you have good ideas for regression tests? The usual compression/decompression works fine, can you imagine corner-cases which would be worth special testing? Should the tests written in the jtreg format?
I'd like to see tests where there's a possibility of semantics having changed; as noted above re "finished".
I haven't done a performance analysis, but don't expect a regression. If anything, since the striding is kept within native code, there should be fewer Java -> native calls, and better performance (though that is perhaps not measurable).
My last comment is about this change in general. It seems like a reasonable fix, though the corresponding bug: http://bugs.sun.com/view_bug.do?bug_id=6399199 is a low priority one for us, and this is code we generally feel is best left alone when possible. We are still learning how best to work with contributions from outside of Sun. I will check with other who've maintained this code in the past, to get their opinion of making this kind of change. While I'm currently responsible for jar/zip code, it's only one of the hats I currently wear ;-)
Thanks, Dave
/* * Copyright 1997-2005 Sun Microsystems, Inc. All Rights Reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it * under the terms of the GNU General Public License version 2 only, as * published by the Free Software Foundation. Sun designates this * particular file as subject to the "Classpath" exception as provided * by Sun in the LICENSE file that accompanied this code. * * This code is distributed in the hope that it will be useful, but WITHOUT * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License * version 2 for more details (a copy is included in the LICENSE file that * accompanied this code). * * You should have received a copy of the GNU General Public License version * 2 along with this work; if not, write to the Free Software Foundation, * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. * * Please contact Sun Microsystems, Inc., 4150 Network Circle, Santa Clara, * CA 95054 USA or visit www.sun.com if you need additional information or * have any questions. */ /* * Native method support for java.util.zip.Deflater */ #include <stdio.h> #include <stdlib.h> #include "jlong.h" #include "jni.h" #include "jni_util.h" #include "zlib.h" #include "java_util_zip_Deflater.h" #define DEF_MEM_LEVEL 8 #define STRIDE_BUF_SIZE 4096 typedef struct { z_stream strm; jbyte in_buf[STRIDE_BUF_SIZE]; jbyte out_buf[STRIDE_BUF_SIZE]; jint stride_read_off; jint stride_read_len; } def_data; static jfieldID strmID; static jfieldID levelID; static jfieldID strategyID; static jfieldID setParamsID; static jfieldID finishID; static jfieldID finishedID; static jfieldID bufID, offID, lenID; JNIEXPORT void JNICALL Java_java_util_zip_Deflater_initIDs(JNIEnv *env, jclass cls) { strmID = (*env)->GetFieldID(env, cls, "strm", "J"); levelID = (*env)->GetFieldID(env, cls, "level", "I"); strategyID = (*env)->GetFieldID(env, cls, "strategy", "I"); setParamsID = (*env)->GetFieldID(env, cls, "setParams", "Z"); finishID = (*env)->GetFieldID(env, cls, "finish", "Z"); finishedID = (*env)->GetFieldID(env, cls, "finished", "Z"); bufID = (*env)->GetFieldID(env, cls, "buf", "[B"); offID = (*env)->GetFieldID(env, cls, "off", "I"); lenID = (*env)->GetFieldID(env, cls, "len", "I"); } JNIEXPORT jlong JNICALL Java_java_util_zip_Deflater_init(JNIEnv *env, jclass cls, jint level, jint strategy, jboolean nowrap) { def_data *defptr = calloc(1, sizeof(def_data)); if (defptr == 0) { JNU_ThrowOutOfMemoryError(env, 0); return jlong_zero; } else { char *msg; switch (deflateInit2(&defptr->strm, level, Z_DEFLATED, nowrap ? -MAX_WBITS : MAX_WBITS, DEF_MEM_LEVEL, strategy)) { case Z_OK: return ptr_to_jlong(defptr); case Z_MEM_ERROR: free(defptr); JNU_ThrowOutOfMemoryError(env, 0); return jlong_zero; case Z_STREAM_ERROR: free(defptr); JNU_ThrowIllegalArgumentException(env, 0); return jlong_zero; default: msg = defptr->strm.msg; free(defptr); JNU_ThrowInternalError(env, msg); return jlong_zero; } } } JNIEXPORT void JNICALL Java_java_util_zip_Deflater_setDictionary(JNIEnv *env, jclass cls, jlong defadr, jarray b, jint off, jint len) { Bytef *buf = (*env)->GetPrimitiveArrayCritical(env, b, 0); int res; if (buf == 0) {/* out of memory */ return; } res = deflateSetDictionary(&(((def_data *)jlong_to_ptr(defadr))->strm), buf + off, len); (*env)->ReleasePrimitiveArrayCritical(env, b, buf, 0); switch (res) { case Z_OK: break; case Z_STREAM_ERROR: JNU_ThrowIllegalArgumentException(env, 0); break; default: JNU_ThrowInternalError(env, (((def_data *)jlong_to_ptr(defadr))->strm.msg)); break; } } JNIEXPORT jint JNICALL Java_java_util_zip_Deflater_deflateBytes(JNIEnv *env, jobject this, jlong def_adr, jbyteArray writebuf, jint writeoff, jint writelen, jbyteArray readbuf, jint readoff, jint readlen, jboolean setParams, jboolean finish) { def_data *defptr = jlong_to_ptr(def_adr); if(defptr == 0) { JNU_ThrowNullPointerException(env, 0); return 0; } else { z_stream *strm = &defptr->strm; const jint writelen_start = writelen; jboolean finished = JNI_FALSE; do { jint availReadBytes; int zres; /*If read-stride is empty and input-bytes are left, fill it up*/ if (defptr->stride_read_len == 0 && readlen > 0) { defptr->stride_read_len = readlen > STRIDE_BUF_SIZE ? STRIDE_BUF_SIZE : readlen; defptr->stride_read_off = 0; (*env)->GetByteArrayRegion(env, readbuf, readoff, defptr->stride_read_len, (jbyte*)&defptr->in_buf); } strm->next_in = (Bytef *)(&defptr->in_buf + defptr->stride_read_off); strm->next_out = (Bytef *)&defptr->out_buf; strm->avail_in = defptr->stride_read_len; availReadBytes = writelen > STRIDE_BUF_SIZE ? STRIDE_BUF_SIZE : writelen; strm->avail_out = availReadBytes; if (setParams) { int level = (*env)->GetIntField(env, this, levelID); int strategy = (*env)->GetIntField(env, this, strategyID); zres = deflateParams(strm, level, strategy); } else { /* Only finish if all input data has been sent to zlib * instead of finishing per stride, which would unnecessarily * lower the compression ratio. */ zres = deflate(strm, (readlen == 0 && finish) ? Z_FINISH : Z_NO_FLUSH); } switch (zres) { case Z_STREAM_END: finished = JNI_TRUE; /* fall through */ case Z_OK: case Z_BUF_ERROR: { jint processed_out = availReadBytes - strm->avail_out; jint processed_in = defptr->stride_read_len - strm->avail_in; if (processed_out > 0) { (*env)->SetByteArrayRegion(env, writebuf, writeoff, processed_out, (jbyte *)&defptr->out_buf); writeoff += processed_out; writelen -= processed_out; } readlen -= processed_in; readoff += processed_in; defptr->stride_read_off += processed_in; defptr->stride_read_len -= processed_in; break; } default: JNU_ThrowInternalError(env, strm->msg); } } while (writelen > 0 && readlen > 0 && !finished && !setParams); if (finished) { (*env)->SetBooleanField(env, this, finishedID, JNI_TRUE); } if (setParams) { (*env)->SetBooleanField(env, this, setParamsID, JNI_FALSE); } (*env)->SetIntField(env, this, offID, readoff); (*env)->SetIntField(env, this, lenID, readlen); return writelen_start - writelen; } } JNIEXPORT jint JNICALL Java_java_util_zip_Deflater_getAdler(JNIEnv *env, jclass cls, jlong defadr) { return ((def_data *)jlong_to_ptr(defadr))->strm.adler; } JNIEXPORT jlong JNICALL Java_java_util_zip_Deflater_getBytesRead(JNIEnv *env, jclass cls, jlong defadr) { return ((def_data *)jlong_to_ptr(defadr))->strm.total_in; } JNIEXPORT jlong JNICALL Java_java_util_zip_Deflater_getBytesWritten(JNIEnv *env, jclass cls, jlong defadr) { return ((def_data *)jlong_to_ptr(defadr))->strm.total_out; } JNIEXPORT void JNICALL Java_java_util_zip_Deflater_reset(JNIEnv *env, jclass cls, jlong defadr) { if (deflateReset(&((def_data *)jlong_to_ptr(defadr))->strm) != Z_OK) { JNU_ThrowInternalError(env, 0); } } JNIEXPORT void JNICALL Java_java_util_zip_Deflater_end(JNIEnv *env, jclass cls, jlong defadr) { def_data *defptr = (def_data *)jlong_to_ptr(defadr); if (deflateEnd(&defptr->strm) == Z_STREAM_ERROR) { JNU_ThrowInternalError(env, 0); } else { free(defptr); } } /* * Copyright 1997-2005 Sun Microsystems, Inc. All Rights Reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it * under the terms of the GNU General Public License version 2 only, as * published by the Free Software Foundation. Sun designates this * particular file as subject to the "Classpath" exception as provided * by Sun in the LICENSE file that accompanied this code. * * This code is distributed in the hope that it will be useful, but WITHOUT * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License * version 2 for more details (a copy is included in the LICENSE file that * accompanied this code). * * You should have received a copy of the GNU General Public License version * 2 along with this work; if not, write to the Free Software Foundation, * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. * * Please contact Sun Microsystems, Inc., 4150 Network Circle, Santa Clara, * CA 95054 USA or visit www.sun.com if you need additional information or * have any questions. */ /* * Native method support for java.util.zip.Deflater */ #include <stdio.h> #include <stdlib.h> #include "jlong.h" #include "jni.h" #include "jni_util.h" #include "zlib.h" #include "java_util_zip_Deflater.h" #define DEF_MEM_LEVEL 8 #define STRIDE_BUF_SIZE 4096 typedef struct { z_stream strm; jbyte in_buf[STRIDE_BUF_SIZE]; jbyte out_buf[STRIDE_BUF_SIZE]; jint stride_read_off; jint stride_read_len; } def_data; static jfieldID levelID; static jfieldID strategyID; static jfieldID setParamsID; static jfieldID finishedID; static jfieldID offID, lenID; JNIEXPORT void JNICALL Java_java_util_zip_Deflater_initIDs(JNIEnv *env, jclass cls) { levelID = (*env)->GetFieldID(env, cls, "level", "I"); strategyID = (*env)->GetFieldID(env, cls, "strategy", "I"); setParamsID = (*env)->GetFieldID(env, cls, "setParams", "Z"); finishedID = (*env)->GetFieldID(env, cls, "finished", "Z"); offID = (*env)->GetFieldID(env, cls, "off", "I"); lenID = (*env)->GetFieldID(env, cls, "len", "I"); } JNIEXPORT jlong JNICALL Java_java_util_zip_Deflater_init(JNIEnv *env, jclass cls, jint level, jint strategy, jboolean nowrap) { def_data *defptr = calloc(1, sizeof(def_data)); if (defptr == 0) { JNU_ThrowOutOfMemoryError(env, 0); return jlong_zero; } else { char *msg; switch (deflateInit2(&defptr->strm, level, Z_DEFLATED, nowrap ? -MAX_WBITS : MAX_WBITS, DEF_MEM_LEVEL, strategy)) { case Z_OK: return ptr_to_jlong(defptr); case Z_MEM_ERROR: free(defptr); JNU_ThrowOutOfMemoryError(env, 0); return jlong_zero; case Z_STREAM_ERROR: free(defptr); JNU_ThrowIllegalArgumentException(env, 0); return jlong_zero; default: msg = defptr->strm.msg; free(defptr); JNU_ThrowInternalError(env, msg); return jlong_zero; } } } JNIEXPORT void JNICALL Java_java_util_zip_Deflater_setDictionary(JNIEnv *env, jclass cls, jlong defadr, jarray b, jint off, jint len) { Bytef *buf = (*env)->GetPrimitiveArrayCritical(env, b, 0); int res; if (buf == 0) {/* out of memory */ return; } res = deflateSetDictionary(&(((def_data *)jlong_to_ptr(defadr))->strm), buf + off, len); (*env)->ReleasePrimitiveArrayCritical(env, b, buf, 0); switch (res) { case Z_OK: break; case Z_STREAM_ERROR: JNU_ThrowIllegalArgumentException(env, 0); break; default: JNU_ThrowInternalError(env, (((def_data *)jlong_to_ptr(defadr))->strm.msg)); break; } } JNIEXPORT jint JNICALL Java_java_util_zip_Deflater_deflateBytes(JNIEnv *env, jobject this, jlong def_adr, jbyteArray writebuf, jint writeoff, jint writelen, jbyteArray readbuf, jint readoff, jint readlen, jboolean setParams, jboolean finish) { def_data *defptr = jlong_to_ptr(def_adr); if(defptr == 0) { JNU_ThrowNullPointerException(env, 0); return 0; } else { z_stream *strm = &defptr->strm; const jint writelen_start = writelen; jboolean finished = JNI_FALSE; do { jint availReadBytes; int zres; /*If read-stride is empty and input-bytes are left, fill it up*/ if (defptr->stride_read_len == 0 && readlen > 0) { defptr->stride_read_len = readlen > STRIDE_BUF_SIZE ? STRIDE_BUF_SIZE : readlen; defptr->stride_read_off = 0; (*env)->GetByteArrayRegion(env, readbuf, readoff, defptr->stride_read_len, (jbyte*)&defptr->in_buf); } strm->next_in = (Bytef *)(&defptr->in_buf + defptr->stride_read_off); strm->next_out = (Bytef *)&defptr->out_buf; strm->avail_in = defptr->stride_read_len; availReadBytes = writelen > STRIDE_BUF_SIZE ? STRIDE_BUF_SIZE : writelen; strm->avail_out = availReadBytes; if (setParams) { int level = (*env)->GetIntField(env, this, levelID); int strategy = (*env)->GetIntField(env, this, strategyID); zres = deflateParams(strm, level, strategy); (*env)->SetBooleanField(env, this, setParamsID, JNI_FALSE); } else { /* Only finish if all input data has been sent to zlib * instead of finishing per stride, which would unnecessarily * lower the compression ratio. */ zres = deflate(strm, (readlen == 0 && finish) ? Z_FINISH : Z_NO_FLUSH); } switch (zres) { case Z_STREAM_END: finished = JNI_TRUE; (*env)->SetBooleanField(env, this, finishedID, JNI_TRUE); /* fall through */ case Z_OK: { jint processed_out = availReadBytes - strm->avail_out; jint processed_in = defptr->stride_read_len - strm->avail_in; if (processed_out > 0) { (*env)->SetByteArrayRegion(env, writebuf, writeoff, processed_out, (jbyte *)&defptr->out_buf); writeoff += processed_out; writelen -= processed_out; } readlen -= processed_in; readoff += processed_in; defptr->stride_read_off += processed_in; defptr->stride_read_len -= processed_in; break; } case Z_BUF_ERROR: return 0; default: JNU_ThrowInternalError(env, strm->msg); return 0; } } while (writelen > 0 && readlen > 0 && !finished && !setParams); (*env)->SetIntField(env, this, offID, readoff); (*env)->SetIntField(env, this, lenID, readlen); return writelen_start - writelen; } } JNIEXPORT jint JNICALL Java_java_util_zip_Deflater_getAdler(JNIEnv *env, jclass cls, jlong defadr) { return ((def_data *)jlong_to_ptr(defadr))->strm.adler; } JNIEXPORT jlong JNICALL Java_java_util_zip_Deflater_getBytesRead(JNIEnv *env, jclass cls, jlong defadr) { return ((def_data *)jlong_to_ptr(defadr))->strm.total_in; } JNIEXPORT jlong JNICALL Java_java_util_zip_Deflater_getBytesWritten(JNIEnv *env, jclass cls, jlong defadr) { return ((def_data *)jlong_to_ptr(defadr))->strm.total_out; } JNIEXPORT void JNICALL Java_java_util_zip_Deflater_reset(JNIEnv *env, jclass cls, jlong defadr) { if (deflateReset(&((def_data *)jlong_to_ptr(defadr))->strm) != Z_OK) { JNU_ThrowInternalError(env, 0); } } JNIEXPORT void JNICALL Java_java_util_zip_Deflater_end(JNIEnv *env, jclass cls, jlong defadr) { def_data *defptr = (def_data *)jlong_to_ptr(defadr); if (deflateEnd(&defptr->strm) == Z_STREAM_ERROR) { JNU_ThrowInternalError(env, 0); } else { free(defptr); } }
Hi Dave, Thanks a lot for your detailed review, and for fixing the warnings and problems on solaris. Unfourtunatly I am quite busy for now (Math exam is coming and I am really bad in maths) - but I'll continue working on it as soon as times permits. Thanks a lot, lg Clemens
There's a slight change to the semantics of "finished". Current code sets Deflater.finished only if setParams is false. Changed code may set it regardlewss of setParams. I suspect this is OK: if client code changed strategy or level and called Deflater.deflate(), it would invoke deflateParams(), and not alter the value of finished. The client's subsequent call to Deflater.deflate() would call deflate() which AFAICT would cause finished to be set. What do you think?
Why fall through from Z_OK to Z_BUF_ERROR? In Z_BUF_ERROR and default cases, why continue execution of loop instead of returning 0 as does original code? I changed this; see attached Deflater.c
*** Deflater.java: OK
*** DeflaterOutputStream.java: OK
More inline below.
Clemens Eisserer wrote:
Hi Dave,
Thanks a lot for your reply. To make it short: Of course I understand that this is low-priority (also for me, its a fun-only fix because someone in forums.java.net mentioned it) so don't hurry. Sorry that I wasted your time with my messy files, they were taken from my "playground" thats why they were in such a bad shape - they were only intended to give an idea which "road" I was taking. I attached the new files taken from the mercurial repositories and only modified at the affected places.
With a change of this sort, we really do need tests along with a fix. Have you started writing any test cases? I completly agree - I have some simple test-cases which test more or less only very basic functionality of Deflater and they work well (also FlatterTest passes). I'll write some more tests which test exotic use-cases like changing compression-level, ... during compression.
Great, thanks. It would be a Good Idea to have a test that checks my assumption re finished (see above).
I have some open questions: 1.) Is the seperate structure approach to hold the stride-buffers ok?
I think so.
2.) Any suggestions for the following names: 1. strm-field in class (defAdr), 2. defAdr-parameter,3. defptr - long_to_ptr of defAdr, 4. def_data - name of the structure
Those don't quite match what I see in the code; but what's in the code seems OK: def_data for the struct, def_adr as a param to deflateBytes, etc. etc. defptr to reference a def_data in init and deflateBytes
3.) I am not really used to program in C. Are the adress-operations ok which I used to get members of the new struct def_data?
It seems OK.
Thanks for your patience, lg Clemens
Some notes, and changes in ramdom order: * Changed deflate-bytes to the old behaviour to return after the call to deflateParams
Good; AFAICT at maintains the existing semantics.
* Verified that its ok to call deflateParams when there's not enough space in the output-buffer to flush all "old" data out (thanks to Mark Adler) * I changed the method-signiture of the native method compared to original, because some variables were read from JNI-code, whereas they could have been passed simply down using method parameters. I think its "cleaner" to pass it.
The long argument to deflateBytes is a bit cumbersome, but Ken Russell opined that it provides better performance, so it's a Good Thing:
Kenneth Russell wrote: I strongly agree with the contributor's suggestion. Not only is passing the argument from Java less code, it is also faster since field access from Java can be optimized by the HotSpot compiler, where field accesses through the JNI must go through the same set of boilerplate C/C++/assembly every time.
* Allocation of the stride-buffers together with the z_stream structure. z_stream is really large, so the two stride-buffers should not add that much overhead. However this has the advantage of not mallocing/freeing and also beeing able to fill the input-stride-buffer once for several calls of the native method.
Looks good.
* Renamed the strm-adress-parameter to defadr, because it no longer really points to a strm. I did not rename the java field "strm" because I did not have an idea for a proper name.
It should have had a different name from day one. I'm slightly loathe to make a name change, nor do I have (Friday afternoon) a Really Good Idea.
* Removed striding from DeflaterOutputStream, (looked how code looked in 1.4.2).
Looks good.
From your other email:
I also thought about implementing striding in the CRC32/Adler32 classes which basically suffer from the same block-the-gc behaviour as Inflater/Deflater did before they were "fixed" ;)
I suspect these are not as much of an issue: presumably CRC32 and adler32 calculations are faster than deflation (caveat: I haven't measured them). Other have pointed out / shamed us because, hey, shouldn't these be in Java anyway?
Furthermore do you have good ideas for regression tests? The usual compression/decompression works fine, can you imagine corner-cases which would be worth special testing? Should the tests written in the jtreg format?
I'd like to see tests where there's a possibility of semantics having changed; as noted above re "finished".
I haven't done a performance analysis, but don't expect a regression. If anything, since the striding is kept within native code, there should be fewer Java -> native calls, and better performance (though that is perhaps not measurable).
My last comment is about this change in general. It seems like a reasonable fix, though the corresponding bug: http://bugs.sun.com/view_bug.do?bug_id=6399199 is a low priority one for us, and this is code we generally feel is best left alone when possible. We are still learning how best to work with contributions from outside of Sun. I will check with other who've maintained this code in the past, to get their opinion of making this kind of change. While I'm currently responsible for jar/zip code, it's only one of the hats I currently wear ;-)
Thanks, Dave
participants (2)
-
Clemens Eisserer
-
Dave Bristor