[Zlib-devel] Mark Adler, where are you?

[Daniel Richard G.]

On Fri, 20 Jul 2012, Jan Seiffert wrote:

> But you could test if it regresses on other architectures (maybe 
> important to you) then s/390 ;)

Not much I can do there besides running "make check"...

>> Absent his request, however, unsolicited opinions from people he 
>> doesn't know and trust are going to be of little value to him, and not 
>> a good use of time for us.
>
> Hmmm, i wouldn't call tests "opinions"

If someone finds a bug, that's a fact. If someone says everything looks 
good, that's an opinion.

> All i know is Mark is always very silent.
>
> He's probably busy with daily business (shooting stuff into space or 
> whatever his job is nowadays).

Admittedly more fun than hacking on a wildly popular data-compression 
library. So Mark doesn't have any trusted lieutenants who can help him 
with the day-to-day work of maintainership?

> Some reflections on the patches:

> Longest match - Longest match is a tough nut to optimize. As a SIMD 
[...]

> and use that. But i see that the glibc <stdint.h> defines uint_fast32_t to
> unsigned long on 64Bit x86, so it would be no win. Maybe
>
> #ifdef HAVE_STDINT_H
> # include <stdint.h>
> # ifdef __x86_64__
> typedef uint32_t uFastInt;
> # else
> typedef uint_fast32_t uFastInt;
> # endif
> #else
> typedef unigned int uFastInt;
> #endif

That's a lot of trouble to go to to make uFastInt a 32-bit integer on 
x86-64. Why not just use uint32_t unconditionally? Is there any platform 
that will run faster with uFastInt being a 64-bit type?

> Your valgrind cleanup - Here the "problem" is you are doing an (often) 128kb
> memset (if memory serves me right). And that probably for nothing.

That memset() follows a malloc(). If inner-loop performance is a concern, 
then the latter is a *much* bigger issue than the former.

And it's certainly not for nothing---not for all the times that I've been 
Valgrinding a large application, and the end result is no memory leaked, 
and several thousand uninitialized-bytes errors in one context deep within 
zlib. There's no good reason to keep things like that.

> If you look at deflate.c at 191 (around #define CLEAR_HASH)
> it says that prev[] will be initialised on the fly.
> This could be wrong, but i doubt it. Valgrind can sometimes
> be very tricky. Or it may be an idiom Valgrind can't understand
> like "Yes, the code does read uninitialised data, but we do
> nothing with it". (cmp. deflate.c at 1358, btw, did you compile
> with FASTEST or not?)

Nope, didn't use FASTEST (described as "deflate algorithm with only one, 
lowest compression level" in zlib.h).

There is indeed a way that Valgrind can be told "yes, we're reading 
uninitialized data here, but that's okay." And I'm sure that there is a 
way to tell Purify the same thing. It's even possible that there exist 
configuration options to do this for most of the entries in

     http://en.wikipedia.org/wiki/Memory_debugger#List_of_memory_debugging_tools

Calling memset() is easier :-)

> It would have been good to include the error output from valgrind.

You would really have found that helpful?


--Daniel


-- 
Daniel Richard G. || danielg at teragram.com || Software Developer
Teragram Linguistic Technologies (a division of SAS)
http://www.teragram.com/