Re: Something is broken in repack

From: Nicolas Pitre <nico@cam.org>
Date: Dec 11, 2007, 17:21 UTC
Message-ID: <alpine.LFD.0.99999.0712111202470.555@xanadu.home>
In-Reply-To: <alpine.LFD.0.9999.0712110806540.25032@woody.linux-foundation.org>

On Tue, 11 Dec 2007, Linus Torvalds wrote:

Show 8 quoted lines

> 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.

I beg to differ (of course, since I always know precisely what I do, and like you, my code never has bugs).

Seriously though, the trg_entry has not to be protected at all. Why? Simply because each thread has its own exclusive set of objects which no other threads ever mess with. They never overlap.

Show 13 quoted lines

>    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, theÿ́'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.

No. Once again, it is impossible for two threads to ever see the same src->data at all. The lock is there simply because read_sha1_file() is not reentrant.

>    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.

For now it is, but I wouldn't say it really needs significant work at this point. The latest thread patches were more about tuning than correctness.

What the threading could be doing, though, is uncovering some other bugs, like in the pack mmap windowing code for example. Although that code is serialized by the read lock above, the fact that multiple threads are hammering on it in turns means that the mmap window is possibly seeking back and forth much more often than otherwise, possibly leaking something in the process.

Show 17 quoted lines

>  - 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 ;)

Linus, please pay attention to the _actual_ important issue here.

Sure I've been tuning the threading code in parallel to the attempt to debug this memory usage issue.

BUT. The point is that repacking the gcc repo using "git repack -a -f --window=250" has a radically different memory usage profile whether you do the repack on the earlier 2.1GB pack or the later 300MB pack. _That_ is the issue. Ironically, it is the 300MB pack that causes the repack to blow memory usage out of proportion.

And in both cases, the threading code has to do the same work whether or not the original pack was densely packed or not since -f throws away every existing deltas anyway.

So something is fishy elsewhere than in the packing code.

Nicolas