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);
}
}
