Re: Be more careful about zlib return values

From: Junio C Hamano <junkio@cox.net>
Date: Mar 21, 2007, 08:11 UTC
Message-ID: <7vodmn2eya.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <Pine.LNX.4.64.0703201124260.6730@woody.linux-foundation.org>

Linus Torvalds <torvalds@linux-foundation.org> writes:

Show 11 quoted lines

> When creating a new object, we use "deflate(stream, Z_FINISH)" in a loop 
> until it no longer returns Z_OK, and then we do "deflateEnd()" to finish 
> up business.
>
> That should all work, but the fact is, it's not how you're _supposed_ to 
> use the zlib return values properly:
> ...
> Somebody who has worked more with zlib should probably double-check me, 
> but this is what <zlib.h> claims is the right thing to do.
>
> 		Linus

I am observing a very curious performance regression with this patch. I do not see anything obviously wrong correctness-wise nor performance-wise with it.

This is driving me crazy, as the benchmark is in that repository with a 1.7GB pack:

	$ git-rev-list HEAD -- org.eclipse.debug.ui/ >/dev/null

and the patch is ONLY ABOUT write_sha1_file() codepath. The function should not even be called!

Without the patch (3 typical runs):

16.16user 0.16system 0:16.32elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k 0inputs+0outputs (0major+79008minor)pagefaults 0swaps 15.84user 0.14system 0:16.02elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k 0inputs+0outputs (0major+79009minor)pagefaults 0swaps 16.04user 0.12system 0:16.17elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k 0inputs+0outputs (0major+79008minor)pagefaults 0swaps

With the patch:

17.62user 0.20system 0:17.94elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k 0inputs+0outputs (0major+79009minor)pagefaults 0swaps 17.79user 0.19system 0:18.03elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k 0inputs+0outputs (0major+79011minor)pagefaults 0swaps 17.40user 0.14system 0:17.63elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k 0inputs+0outputs (0major+79009minor)pagefaults 0swaps

Another funny thing is that if I replace the body of write_sha1_file() with just:

int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned char *returnsha1)
{
	setup_object_header(NULL, NULL, 0);
	die("foo");
}

I get somewhat faster number.

I guess I probably should write it off as some curiosity coming from insn cacheline bouncing between hot functions, that is caused by the size of this unused function changing.