git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 1/3] revision: unify {tree,blob}_objects in rev_info

From
Jeff King <peff@peff.net>
Date
Feb 28, 2017, 22:06 UTC
Message-ID
<20170228220626.at4cihedmvkqiq5c@sigill.intra.peff.net>
In-Reply-To
<06a84f8c77924b275606384ead8bb2fd7d75f7b6.1487984670.git.jonathantanmy@google.com>
On Fri, Feb 24, 2017 at 05:18:36PM -0800, Jonathan Tan wrote:
Show 12 quoted lines
> Whenever tree_objects is set to 1 in revision.h's struct rev_info,
> blob_objects is likewise set, and vice versa. Combine those two fields
> into one.
> 
> Some of the existing code does not handle tree_objects being different
> from blob_objects properly. For example, "handle_commit" in revision.c
> recurses from an UNINTERESTING tree into its subtree if tree_objects ==
> 1, completely ignoring blob_objects; it probably should still recurse if
> tree_objects == 0 and blob_objects == 1 (to mark the blobs), and should
> behave differently depending on blob_objects (controlling the
> instantiation and marking of blob objects). This commit resolves the
> issue by forbidding tree_objects from being different to blob_objects.

Yeah, I agree that is awkward. I'm OK with the rule "if blob_objects is set, then tree_objects must also be set". It's the other way around I care more about.

Show 7 quoted lines
> It could be argued that in the future, Git might need to distinguish
> tree_objects from blob_objects - in particular, a user might want
> rev-list to print the trees but not the blobs. However, this results in
> a minor performance savings at best in that objects no longer need to be
> instantiated (causing memory allocations and hashtable insertions) - no
> disk reads are being done for objects whether blob_objects is set or
> not.

In a full object-graph traversal, we actually spend a big chunk of our time in hash lookups. My measurements (admittedly from 2013, which I haven't repeated lately) show something like a 20-25% speedup for this case.

My only use for it (and the source of those timings) was to compute archive reachability, which nobody seems to care too much about. But I suspect we could speed up your case, too, when we are just computing the reachability of a non-blob. I.e., you should be able to turn on the smallest subset of "commits only", "commits and trees", and "commits, trees, and blobs", based on what the other side has asked for.

-Peff
Previous: Junio C HamanoNext: Jonathan Tan
Message 16 of 17 in “Test fetch-pack's ability to fetch arbitrary blobs”
  1. 0/3 Test fetch-pack's ability to fetch arbitrary blobsJonathan Tan, Feb 25, 2017
  2. 2/3 revision: exclude trees/blobs given commitJonathan Tan, Feb 25, 2017
  3. Junio C HamanoFeb 28, 2017
  4. Jeff KingFeb 28, 2017
  5. t/perf: export variable used in other blocksJonathan Tan, Mar 2, 2017
  6. Jeff KingMar 3, 2017
  7. t/perf: use $MODERN_GIT for all repo-copying stepsJeff King, Mar 3, 2017
  8. t/perf: add fallback for pre-bin-wrappers versions of gitJeff King, Mar 3, 2017
  9. Junio C HamanoMar 3, 2017
  10. Jeff KingMar 3, 2017
  11. Junio C HamanoFeb 28, 2017
  12. 1/3 revision: unify {tree,blob}_objects in rev_infoJonathan Tan, Feb 25, 2017
  13. Junio C HamanoFeb 28, 2017
  14. Jeff KingFeb 28, 2017
  15. Junio C HamanoMar 2, 2017
  16. Jeff KingFeb 28, 2017
  17. 3/3 upload-pack: compute blob reachability correctlyJonathan Tan, Feb 25, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.