Re: Something is broken in repack

On Tue, 11 Dec 2007, Jon Smirl wrote:

Threaded code *always* takes more CPU time. The only thing you can hope 
for is a wall-clock reduction. You're seeing probably a combination of 
 (a) more cache misses
 (b) bigger dataset active at a time
and a probably fairly miniscule
 (c) threading itself tends to have some overheads.

Sure they are shared. They're just not *entirely* shared. But they are shared between each two cores, so each thread essentially has only half the cache they had with the non-threaded version.

Threading is *not* a magic solution to all problems. It gives you potentially twice the CPU power, but there are real downsides that you should keep in mind.

I suspect that it's really simple: you have a few rather big files in the gcc history, with deep delta chains. And what happens when you have four threads running at the same time is that they all need to keep all those objects that they are working on - and their hash state - in memory at the same time!

So if you want to use more threads, that _forces_ you to have a bigger memory footprint, simply because you have more "live" objects that you work on. Normally, that isn't much of a problem, since most source files are small, but if you have a few deep delta chains on big files, both the delta chain itself is going to use memory (you may have limited the size of the cache, but it's still needed for the actual delta generation, so it's not like the memory usage went away).

That said, I suspect there are a few things fighting you:

 - threading is hard. I haven't looked a lot at the changes Nico did to do 
   a threaded object packer, but what I've seen does not convince me it is 
   correct. The "trg_entry" accesses are *mostly* protected with 
   "cache_lock", but nothing else really seems to be, so quite frankly, I 
   wouldn't trust the threaded version very much. It's off by default, and 
   for a good reason, I think.

   For example: the packing code does this:

	if (!src->data) {
		read_lock();
		src->data = read_sha1_file(src_entry->idx.sha1, &type, &sz);
		read_unlock();
		...

   and that's racy. If two threads come in at roughly the same time and 
   see a NULL src->data, theÿ́'ll both get the lock, and they'll both 
   (serially) try to fill it in. It will all *work*, but one of them will 
   have done unnecessary work, and one of them will have their result 
   thrown away and leaked.

   Are you hitting issues like this? I dunno. The object sorting means 
   that different threads normally shouldn't look at the same objects (not 
   even the sources), so probably not, but basically, I wouldn't trust the 
   threading 100%. It needs work, and it needs to stay off by default.

 - you're working on a problem that isn't really even worth optimizing 
   that much. The *normal* case is to re-use old deltas, which makes all 
   of the issues you are fighting basically go away (because you only have 
   a few _incremental_ objects that need deltaing). 

   In other words: the _real_ optimizations have already been done, and 
   are done elsewhere, and are much smarter (the best way to optimize X is 
   not to make X run fast, but to avoid doing X in the first place!). The 
   thing you are trying to work with is the one-time-only case where you 
   explicitly disable that big and important optimization, and then you 
   complain about the end result being slow!

   It's like saying that you're compiling with extreme debugging and no
   optimizations, and then complaining that the end result doesn't run as 
   fast as if you used -O2. Except this is a hundred times worse, because 
   you literally asked git to do the really expensive thing that it really 
   really doesn't want to do ;)

See above: I'd look around at threading-related bugs and check the way we lock (or don't) accesses.

		Linus