From: Stefan Sperling <stsp@stsp.name>
Subject: Re: deltify addblk() fixes
To: Christian Weisgerber <naddy@mips.inka.de>
Cc: gameoftrees@openbsd.org
Date: Fri, 11 Jun 2021 19:22:06 +0200

On Fri, Jun 11, 2021 at 12:11:47AM +0200, Christian Weisgerber wrote:
> Stefan Sperling:
> 
> > Thank you! Here is a new diff with these problems fixed.
> > 
> > Your fixes don't make a huge difference but they do cause 'malloc'
> > and 'fseek' calls reported by gprof to drop down by about 2% on a
> > trivial test repository.
> 
> ok naddy@

Thanks!

> I have a further suggestion, though:
> 
>         while (dt->blocks[i].len != 0) {
>                 if (r == 0) {
> 			... read block into buf ...
>                 }
>                 if (len == dt->blocks[i].len && h == dt->blocks[i].hash) {
> 			... read other block into buf2 and compare ...
>                 }
>                 i = (i + 1) % dt->nalloc;
>         }
> 
> There's no point in reading a block until we know that we need to
> compare it.  The "if (r == 0)" part should move under the following
> if.
> 
>                 if (len == dt->blocks[i].len && h == dt->blocks[i].hash) {
>                         if (r == 0) {
>                                 ... read block into buf ...
>                         }
>                         ... read other block into buf2 and compare ...
>                 }

Yes, that makes sense. This patch implemenents your suggestion.


diff 68bdcdc2f5d3c37d918f85368c2537a8aa7d90eb /home/stsp/src/got
blob - 45add0094489e3d46e675165b1d5379f56a01a4a
file + lib/deltify.c
--- lib/deltify.c
+++ lib/deltify.c
@@ -100,7 +100,6 @@ addblk(struct got_delta_table *dt, FILE *f, off_t len,
 {
 	const struct got_error *err = NULL;
 	int i;
-	uint8_t buf[GOT_DELTIFY_MAXCHUNK];
 	size_t r = 0;
 
 	if (len == 0)
@@ -108,23 +107,24 @@ addblk(struct got_delta_table *dt, FILE *f, off_t len,
 
 	i = h % dt->nalloc;
 	while (dt->blocks[i].len != 0) {
-		if (r == 0) {
-			if (fseeko(f, offset, SEEK_SET) == -1)
-				return got_error_from_errno("fseeko");
-			r = fread(buf, 1, len, f);
-			if (r != len) {
-				if (ferror(f))
-					return got_ferror(f, GOT_ERR_IO);
-				return NULL;
-			}
-		}
 		/*
 		 * Avoid adding duplicate blocks.
 		 * NB: A matching hash is insufficient for detecting equality.
 		 * The hash can only detect inequality.
 		 */
 		if (len == dt->blocks[i].len && h == dt->blocks[i].hash) {
+			uint8_t buf[GOT_DELTIFY_MAXCHUNK];
 			uint8_t buf2[GOT_DELTIFY_MAXCHUNK];
+			if (r == 0) {
+				if (fseeko(f, offset, SEEK_SET) == -1)
+					return got_error_from_errno("fseeko");
+				r = fread(buf, 1, len, f);
+				if (r != len) {
+					if (!ferror(f))
+						return NULL;
+					return got_ferror(f, GOT_ERR_IO);
+				}
+			}
 			if (fseeko(f, dt->blocks[i].offset, SEEK_SET) == -1)
 				return got_error_from_errno("fseeko");
 			if (fread(buf2, 1, len, f) != len)