Early version of striding Deflater

[Dave Bristor]

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