From 202e7f1e161b5bce6587d1a696843ead10a8b477 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 10 Aug 2018 19:09:06 -0400 Subject: for_each_*_object: store flag definitions in a single location These flags were split between cache.h and packfile.h, because some of the flags apply only to packs. However, they share a single numeric namespace, since both are respected for the packed variant. Let's make sure they're defined together so that nobody accidentally adds a new flag in one location that duplicates the other. While we're here, let's also put them in an enum (which helps debugger visibility) and use "(1< Signed-off-by: Junio C Hamano --- packfile.h | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) (limited to 'packfile.h') diff --git a/packfile.h b/packfile.h index cc7eaffe1b..6ddc6a2e91 100644 --- a/packfile.h +++ b/packfile.h @@ -148,15 +148,11 @@ extern int has_object_pack(const struct object_id *oid); extern int has_pack_index(const unsigned char *sha1); -/* - * Only iterate over packs obtained from the promisor remote. - */ -#define FOR_EACH_OBJECT_PROMISOR_ONLY 2 - /* * Iterate over packed objects in both the local * repository and any alternates repositories (unless the - * FOR_EACH_OBJECT_LOCAL_ONLY flag, defined in cache.h, is set). + * FOR_EACH_OBJECT_LOCAL_ONLY flag is set). See cache.h for the complete list + * of flags. */ typedef int each_packed_object_fn(const struct object_id *oid, struct packed_git *pack, -- cgit v1.2.1 From a7ff6f5a0f310406aa4973e8d7ec25815554bcb5 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 10 Aug 2018 19:09:44 -0400 Subject: for_each_*_object: take flag arguments as enum It's not wrong to pass our flags in an "unsigned", as we know it will be at least as large as the enum. However, using the enum in the declaration makes it more obvious where to find the list of flags. While we're here, let's also drop the "extern" noise-words from the declarations, per our modern coding style. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- packfile.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'packfile.h') diff --git a/packfile.h b/packfile.h index 6ddc6a2e91..9861728514 100644 --- a/packfile.h +++ b/packfile.h @@ -158,8 +158,9 @@ typedef int each_packed_object_fn(const struct object_id *oid, struct packed_git *pack, uint32_t pos, void *data); -extern int for_each_object_in_pack(struct packed_git *p, each_packed_object_fn, void *data); -extern int for_each_packed_object(each_packed_object_fn, void *, unsigned flags); +int for_each_object_in_pack(struct packed_git *p, each_packed_object_fn, void *data); +int for_each_packed_object(each_packed_object_fn, void *, + enum for_each_object_flags flags); /* * Return 1 if an object in a promisor packfile is or refers to the given -- cgit v1.2.1 From 8b361551900be9bedd946386362f2d0e2a506845 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 10 Aug 2018 19:11:14 -0400 Subject: for_each_*_object: give more comprehensive docstrings We already mention the local/alternate behavior of these functions, but we can help clarify a few other behaviors: - there's no need to mention LOCAL_ONLY specifically, since we already reference the flags by type (and as we add more flags, we don't want to have to mention each) - clarify that reachability doesn't matter here; this is all accessible objects - what ordering/uniqueness guarantees we give - how pack-specific flags are handled for the loose case Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- packfile.h | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) (limited to 'packfile.h') diff --git a/packfile.h b/packfile.h index 9861728514..c86a8c2716 100644 --- a/packfile.h +++ b/packfile.h @@ -149,10 +149,14 @@ extern int has_object_pack(const struct object_id *oid); extern int has_pack_index(const unsigned char *sha1); /* - * Iterate over packed objects in both the local - * repository and any alternates repositories (unless the - * FOR_EACH_OBJECT_LOCAL_ONLY flag is set). See cache.h for the complete list - * of flags. + * Iterate over all accessible packed objects without respect to reachability. + * By default, this includes both local and alternate packs. + * + * Note that some objects may appear twice if they are found in multiple packs. + * Each pack is visited in an unspecified order. Objects within a pack are + * visited in pack-idx order (i.e., sorted by oid). + * + * The list of flags can be found in cache.h. */ typedef int each_packed_object_fn(const struct object_id *oid, struct packed_git *pack, -- cgit v1.2.1 From 736eb88fdc8a2dea4302114d2f74b580d0f83cfe Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 10 Aug 2018 19:15:49 -0400 Subject: for_each_packed_object: support iterating in pack-order We currently iterate over objects within a pack in .idx order, which uses the object hashes. That means that it is effectively random with respect to the location of the object within the pack. If you're going to access the actual object data, there are two reasons to move linearly through the pack itself: 1. It improves the locality of access in the packfile. In the cold-cache case, this may mean fewer disk seeks, or better usage of disk cache. 2. We store related deltas together in the packfile. Which means that the delta base cache can operate much more efficiently if we visit all of those related deltas in sequence, as the earlier items are likely to still be in the cache. Whereas if we visit the objects in random order, our cache entries are much more likely to have been evicted by unrelated deltas in the meantime. So in general, if you're going to access the object contents pack order is generally going to end up more efficient. But if you're simply generating a list of object names, or if you're going to end up sorting the result anyway, you're better off just using the .idx order, as finding the pack order means generating the in-memory pack-revindex. According to the numbers in 8b8dfd5132 (pack-revindex: radix-sort the revindex, 2013-07-11), that takes about 200ms for linux.git, and 20ms for git.git (those numbers are a few years old but are still a good ballpark). That makes it a good optimization for some cases (we can save tens of seconds in git.git by having good locality of delta access, for a 20ms cost), but a bad one for others (e.g., right now "cat-file --batch-all-objects --batch-check="%(objectname)" is 170ms in git.git, so adding 20ms to that is noticeable). Hence this patch makes it an optional flag. You can't actually do any interesting timings yet, as it's not plumbed through to any user-facing tools like cat-file. That will come in a later patch. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- packfile.h | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) (limited to 'packfile.h') diff --git a/packfile.h b/packfile.h index c86a8c2716..99411bdd85 100644 --- a/packfile.h +++ b/packfile.h @@ -153,8 +153,8 @@ extern int has_pack_index(const unsigned char *sha1); * By default, this includes both local and alternate packs. * * Note that some objects may appear twice if they are found in multiple packs. - * Each pack is visited in an unspecified order. Objects within a pack are - * visited in pack-idx order (i.e., sorted by oid). + * Each pack is visited in an unspecified order. By default, objects within a + * pack are visited in pack-idx order (i.e., sorted by oid). * * The list of flags can be found in cache.h. */ @@ -162,7 +162,9 @@ typedef int each_packed_object_fn(const struct object_id *oid, struct packed_git *pack, uint32_t pos, void *data); -int for_each_object_in_pack(struct packed_git *p, each_packed_object_fn, void *data); +int for_each_object_in_pack(struct packed_git *p, + each_packed_object_fn, void *data, + enum for_each_object_flags flags); int for_each_packed_object(each_packed_object_fn, void *, enum for_each_object_flags flags); -- cgit v1.2.1 From 0889aae1cd18c1804ba01c1a4229e516dfb9fe9b Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 14 Aug 2018 14:21:18 -0400 Subject: for_each_*_object: move declarations to object-store.h The for_each_loose_object() and for_each_packed_object() functions are meant to be part of a unified interface: they use the same set of for_each_object_flags, and it's not inconceivable that we might one day add a single for_each_object() wrapper around them. Let's put them together in a single file, so we can avoid awkwardness like saying "the flags for this function are over in cache.h". Moving the loose functions to packfile.h is silly. Moving the packed functions to cache.h works, but makes the "cache.h is a kitchen sink" problem worse. The best place is the recently-created object-store.h, since these are quite obviously related to object storage. The for_each_*_in_objdir() functions do not use the same flags, but they are logically part of the same interface as for_each_loose_object(), and share callback signatures. So we'll move those, as well, as they also make sense in object-store.h. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- packfile.h | 20 -------------------- 1 file changed, 20 deletions(-) (limited to 'packfile.h') diff --git a/packfile.h b/packfile.h index 99411bdd85..d91e43fe73 100644 --- a/packfile.h +++ b/packfile.h @@ -148,26 +148,6 @@ extern int has_object_pack(const struct object_id *oid); extern int has_pack_index(const unsigned char *sha1); -/* - * Iterate over all accessible packed objects without respect to reachability. - * By default, this includes both local and alternate packs. - * - * Note that some objects may appear twice if they are found in multiple packs. - * Each pack is visited in an unspecified order. By default, objects within a - * pack are visited in pack-idx order (i.e., sorted by oid). - * - * The list of flags can be found in cache.h. - */ -typedef int each_packed_object_fn(const struct object_id *oid, - struct packed_git *pack, - uint32_t pos, - void *data); -int for_each_object_in_pack(struct packed_git *p, - each_packed_object_fn, void *data, - enum for_each_object_flags flags); -int for_each_packed_object(each_packed_object_fn, void *, - enum for_each_object_flags flags); - /* * Return 1 if an object in a promisor packfile is or refers to the given * object, 0 otherwise. -- cgit v1.2.1