Re: [PATCH v4 00/32] Lockfile correctness and refactoring

[Jeff King <peff@peff.net>]

On Fri, Sep 12, 2014 at 04:21:09PM +0200, Michael Haggerty wrote:

> > But I still wonder how hard it would be to just remove lock_file structs
> > from the global list when they are committed or rolled back.
> [...]
>
> To make that change, we would have to remove entries from the list of
> lock_file objects in a way that the code can be interrupted at any time
> by a signal while leaving it in a state that is traversable by the
> signal handler.
> 
> I think that can be done pretty easily with a singly-linked list. But
> with a singly-linked list, we would have to iterate through the list to
> find the node that needs to be removed. This could get expensive if
> there are a lot of nodes in the list (see below).

Yes, I considered that, but noticed that if we actually cleaned up
closed files, the list would not grow to more than a handful of entries.
But...

> The ref-transaction code is, I think, moving in the direction of
> updating all references in a single transaction. This means that we
> would need to hold locks for all of the references at once anyway. So it
> might be all for naught.

That nullifies the whole discussion. Besides the list-traversal thing
above, it would mean that we literally _do_ have all of the lockfiles
open at once. So cleaning up after ourselves would be nice, but it would
not impact the peak memory usage, which would necessarily have one
allocated struct per ref.

The use of a strbuf is probably a big enough change to save us there.
This case was pathological for a few reasons:

  1. A ridiculous number of refs in the repository.

  2. Touching a large number of them in sequence (via pack-refs).

  3. Allocating a 4K buffer per object.

For (3), if the average allocation is dropped even to 400 bytes (which
would accommodate quite a long pathname), that would reduce the memory
usage to ~700MB. Not amazing, but enough not to tip over most modern
machines.

-Peff