Re: [PATCH] revision: introduce prepare_revision_walk_extended()

From: Jeff King <peff@peff.net>
Date: Dec 19, 2017, 11:49 UTC
Message-ID: <20171219114906.GB24558@sigill.intra.peff.net>
In-Reply-To: <39581cd0-0bfd-c8d1-642b-1245cf425ab4@web.de>

On Mon, Dec 18, 2017 at 08:18:19PM +0100, René Scharfe wrote:

Show 5 quoted lines

> > The root of the matter is that the revision-walking code doesn't clean
> > up after itself. In every case, the caller is just saving these to clean
> > up commit marks, isn't it?
> 
> bundle also checks if the pending objects exists.

Thanks, I missed that one. So just adding a feature to clean up commit marks wouldn't be sufficient to cover that case.

Show 8 quoted lines

> > That sidesteps all of the memory ownership issues by just creating a
> > copy. That's less efficient, but I'd be surprised if it matters in
> > practice (we tend to do one or two revisions per process, there don't
> > tend to be a lot of pending tips, and we're really just talking about
> > copying some pointers here).
> [...]
> I don't know if there can be real-world use cases with millions of
> entries (when it would start to hurt).

I've seen repos which have tens of thousands of tags. Something like "rev-list --all" would have tens of thousands of pending objects. I think in practice it's limited to the number of objects (though in practice more like the number of commits).

I'd note also that for most uses we don't need a full object_array. You really just need a pointer to the "struct object" to wipe its flags.

So there we might waste 8 bytes per object in the worst case. But bear in mind that the process is wasting a lot more than that per "struct commit" that we're holding. And versus the existing scheme, it's only for the moment until prepare_revision_walk() frees the old pending list.

> Why does prepare_revision_walk() clear the list of pending objects at
> all?  Assuming the list is append-only then perhaps remembering the
> last handled index would suffice.

I assume it was mostly to clean up after itself, since there's no explicit "I'm done with the traversal" function. But as I said earlier, I'd be surprised of a revision walk doesn't leave some allocated cruft in rev_info these days (e.g., pathspec cruft). In practice it doesn't matter much because we don't do arbitrary numbers of traversals in single process.

-Peff