Re: [PATCH v4 08/11] pack-objects: shrink z_delta_size field in struct object_entry

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

We only cache deltas when it's smaller than a certain limit. This limit
defaults to 1000 but save its compressed length in a 64-bit field.
Shrink that field down to 16 bits, so you can only cache 65kb deltas.
Larger deltas must be recomputed at when the pack is written down.

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---

 		if (entry->delta_data && !pack_to_stdout) {
-			entry->z_delta_size = do_compress(&entry->delta_data,
-							  entry->delta_size);
-			cache_lock();
-			delta_cache_size -= entry->delta_size;
-			delta_cache_size += entry->z_delta_size;
-			cache_unlock();
+			unsigned long size;
+
+			size = do_compress(&entry->delta_data, entry->delta_size);
+			entry->z_delta_size = size;
+			if (entry->z_delta_size == size) {

It is confusing to readers to write

	A = B;
	if (A == B) {
		/* OK, A was big enough */
	} else {
		/* No, B is too big to fit on A */
	}

I actually was about to complain that you attempted an unrelated
micro-optimization to skip cache_lock/unlock when delta_size and
z_delta_size are the same, and made a typo.  Something like:

	size = do_compress(...);
	if (size < (1 << OE_Z_DELTA_BITS)) {
		entry->z_delta_size = size;
		cache_lock();
		...
                cache_unlock();
	} else {
		FREE_AND_NULL(entry->delta_data);
		entry->z_delta_size = 0;
	}

would have saved me a few dozens of seconds of head-scratching.

+				cache_lock();
+				delta_cache_size -= entry->delta_size;
+				delta_cache_size += entry->z_delta_size;
+				cache_unlock();
+			} else {
+				FREE_AND_NULL(entry->delta_data);
+				entry->z_delta_size = 0;
+			}
 		}