From bec2a69fe4c2dbb377d2742a4def7e3569b4c1d4 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 27 Feb 2006 21:37:56 -0800 Subject: [PATCH 1/2] Revert "Revert "diff-delta: produce optimal pack data"" --- diff-delta.c | 77 ++++++++++++++++++++-------------------------------- 1 file changed, 30 insertions(+), 47 deletions(-) diff --git a/diff-delta.c b/diff-delta.c index 2ed5984b1c..27f83a0858 100644 --- a/diff-delta.c +++ b/diff-delta.c @@ -20,21 +20,11 @@ #include #include -#include #include "delta.h" -/* block size: min = 16, max = 64k, power of 2 */ -#define BLK_SIZE 16 - -#define MIN(a, b) ((a) < (b) ? (a) : (b)) - -#define GR_PRIME 0x9e370001 -#define HASH(v, shift) (((unsigned int)(v) * GR_PRIME) >> (shift)) - struct index { const unsigned char *ptr; - unsigned int val; struct index *next; }; @@ -42,21 +32,21 @@ static struct index ** delta_index(const unsigned char *buf, unsigned long bufsize, unsigned int *hash_shift) { - unsigned int hsize, hshift, entries, blksize, i; + unsigned long hsize; + unsigned int hshift, i; const unsigned char *data; struct index *entry, **hash; void *mem; /* determine index hash size */ - entries = (bufsize + BLK_SIZE - 1) / BLK_SIZE; - hsize = entries / 4; - for (i = 4; (1 << i) < hsize && i < 16; i++); + hsize = bufsize / 4; + for (i = 8; (1 << i) < hsize && i < 16; i++); hsize = 1 << i; - hshift = 32 - i; + hshift = i - 8; *hash_shift = hshift; /* allocate lookup index */ - mem = malloc(hsize * sizeof(*hash) + entries * sizeof(*entry)); + mem = malloc(hsize * sizeof(*hash) + bufsize * sizeof(*entry)); if (!mem) return NULL; hash = mem; @@ -64,17 +54,12 @@ static struct index ** delta_index(const unsigned char *buf, memset(hash, 0, hsize * sizeof(*hash)); /* then populate it */ - data = buf + entries * BLK_SIZE - BLK_SIZE; - blksize = bufsize - (data - buf); - while (data >= buf) { - unsigned int val = adler32(0, data, blksize); - i = HASH(val, hshift); - entry->ptr = data; - entry->val = val; + data = buf + bufsize - 2; + while (data > buf) { + entry->ptr = --data; + i = data[0] ^ data[1] ^ (data[2] << hshift); entry->next = hash[i]; hash[i] = entry++; - blksize = BLK_SIZE; - data -= BLK_SIZE; } return hash; @@ -141,29 +126,27 @@ void *diff_delta(void *from_buf, unsigned long from_size, while (data < top) { unsigned int moff = 0, msize = 0; - unsigned int blksize = MIN(top - data, BLK_SIZE); - unsigned int val = adler32(0, data, blksize); - i = HASH(val, hash_shift); - for (entry = hash[i]; entry; entry = entry->next) { - const unsigned char *ref = entry->ptr; - const unsigned char *src = data; - unsigned int ref_size = ref_top - ref; - if (entry->val != val) - continue; - if (ref_size > top - src) - ref_size = top - src; - while (ref_size && *src++ == *ref) { - ref++; - ref_size--; - } - ref_size = ref - entry->ptr; - if (ref_size > msize) { - /* this is our best match so far */ - moff = entry->ptr - ref_data; - msize = ref_size; - if (msize >= 0x10000) { - msize = 0x10000; + if (data + 2 < top) { + i = data[0] ^ data[1] ^ (data[2] << hash_shift); + for (entry = hash[i]; entry; entry = entry->next) { + const unsigned char *ref = entry->ptr; + const unsigned char *src = data; + unsigned int ref_size = ref_top - ref; + if (ref_size > top - src) + ref_size = top - src; + if (ref_size > 0x10000) + ref_size = 0x10000; + if (ref_size <= msize) break; + while (ref_size && *src++ == *ref) { + ref++; + ref_size--; + } + ref_size = ref - entry->ptr; + if (msize < ref - entry->ptr) { + /* this is our best match so far */ + msize = ref - entry->ptr; + moff = entry->ptr - ref_data; } } } From 2b8d9347aa1a11f1ac13591f89ca9f984d467c77 Mon Sep 17 00:00:00 2001 From: Nicolas Pitre Date: Mon, 27 Feb 2006 23:09:55 -0500 Subject: [PATCH 2/2] diff-delta: bound hash list length to avoid O(m*n) behavior The diff-delta code can exhibit O(m*n) behavior with some patological data set where most hash entries end up in the same hash bucket. The latest code rework reduced the block size making it particularly vulnerable to this issue, but the issue was always there and can be triggered regardless of the block size. This patch does two things: 1) the hashing has been reworked to offer a better distribution to atenuate the problem a bit, and 2) a limit is imposed to the number of entries that can exist in the same hash bucket. Because of the above the code is a bit more expensive on average, but the problematic samples used to diagnoze the issue are now orders of magnitude less expensive to process with only a slight loss in compression. Signed-off-by: Nicolas Pitre Signed-off-by: Junio C Hamano --- diff-delta.c | 69 ++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 56 insertions(+), 13 deletions(-) diff --git a/diff-delta.c b/diff-delta.c index 27f83a0858..0730b24df8 100644 --- a/diff-delta.c +++ b/diff-delta.c @@ -30,19 +30,20 @@ struct index { static struct index ** delta_index(const unsigned char *buf, unsigned long bufsize, + unsigned long trg_bufsize, unsigned int *hash_shift) { unsigned long hsize; - unsigned int hshift, i; + unsigned int i, hshift, hlimit, *hash_count; const unsigned char *data; struct index *entry, **hash; void *mem; /* determine index hash size */ hsize = bufsize / 4; - for (i = 8; (1 << i) < hsize && i < 16; i++); + for (i = 8; (1 << i) < hsize && i < 24; i += 2); hsize = 1 << i; - hshift = i - 8; + hshift = (i - 8) / 2; *hash_shift = hshift; /* allocate lookup index */ @@ -53,15 +54,59 @@ static struct index ** delta_index(const unsigned char *buf, entry = mem + hsize * sizeof(*hash); memset(hash, 0, hsize * sizeof(*hash)); - /* then populate it */ + /* allocate an array to count hash entries */ + hash_count = calloc(hsize, sizeof(*hash_count)); + if (!hash_count) { + free(hash); + return NULL; + } + + /* then populate the index */ data = buf + bufsize - 2; while (data > buf) { entry->ptr = --data; - i = data[0] ^ data[1] ^ (data[2] << hshift); + i = data[0] ^ ((data[1] ^ (data[2] << hshift)) << hshift); entry->next = hash[i]; hash[i] = entry++; + hash_count[i]++; } + /* + * Determine a limit on the number of entries in the same hash + * bucket. This guard us against patological data sets causing + * really bad hash distribution with most entries in the same hash + * bucket that would bring us to O(m*n) computing costs (m and n + * corresponding to reference and target buffer sizes). + * + * The more the target buffer is large, the more it is important to + * have small entry lists for each hash buckets. With such a limit + * the cost is bounded to something more like O(m+n). + */ + hlimit = (1 << 26) / trg_bufsize; + if (hlimit < 16) + hlimit = 16; + + /* + * Now make sure none of the hash buckets has more entries than + * we're willing to test. Otherwise we short-circuit the entry + * list uniformly to still preserve a good repartition across + * the reference buffer. + */ + for (i = 0; i < hsize; i++) { + if (hash_count[i] < hlimit) + continue; + entry = hash[i]; + do { + struct index *keep = entry; + int skip = hash_count[i] / hlimit / 2; + do { + entry = entry->next; + } while(--skip && entry); + keep->next = entry; + } while(entry); + } + free(hash_count); + return hash; } @@ -85,7 +130,7 @@ void *diff_delta(void *from_buf, unsigned long from_size, if (!from_size || !to_size) return NULL; - hash = delta_index(from_buf, from_size, &hash_shift); + hash = delta_index(from_buf, from_size, to_size, &hash_shift); if (!hash) return NULL; @@ -126,8 +171,8 @@ void *diff_delta(void *from_buf, unsigned long from_size, while (data < top) { unsigned int moff = 0, msize = 0; - if (data + 2 < top) { - i = data[0] ^ data[1] ^ (data[2] << hash_shift); + if (data + 3 <= top) { + i = data[0] ^ ((data[1] ^ (data[2] << hash_shift)) << hash_shift); for (entry = hash[i]; entry; entry = entry->next) { const unsigned char *ref = entry->ptr; const unsigned char *src = data; @@ -138,11 +183,9 @@ void *diff_delta(void *from_buf, unsigned long from_size, ref_size = 0x10000; if (ref_size <= msize) break; - while (ref_size && *src++ == *ref) { - ref++; - ref_size--; - } - ref_size = ref - entry->ptr; + if (*ref != *src) + continue; + while (ref_size-- && *++src == *++ref); if (msize < ref - entry->ptr) { /* this is our best match so far */ msize = ref - entry->ptr;