Re: [PATCH 09/12] receive-pack: use oidset to...

Re: [PATCH 09/12] receive-pack: use oidset to de-duplicate .have lines

From: Junio C Hamano <hidden>
Date: 2017-01-25 19:32:13

Jeff King [off-list ref] writes:

If you have an alternate object store with a very large
number of refs, the peak memory usage of the sha1_array can
grow high, even if most of them are duplicates that end up
not being printed at all.
...
Signed-off-by: Jeff King <redacted>
---
 builtin/receive-pack.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

Nice.  

Incidentally, this also shows that the refnames in alternate ref
namespace will not matter.  We are to only show just one of many
anyway (and that name is masked and replaced with ".have").  Perhaps
we want to do 04/12 without refname?

quoted hunk

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index b9f2c0cc5..27bb52988 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c

@@ -21,6 +21,7 @@
 #include "sigchain.h"
 #include "fsck.h"
 #include "tmp-objdir.h"
+#include "oidset.h"
 
 static const char * const receive_pack_usage[] = {
 	N_("git receive-pack <git-dir>"),

@@ -271,27 +272,24 @@ static int show_ref_cb(const char *path_full, const struct object_id *oid,
 	return 0;
 }
 
-static int show_one_alternate_sha1(const unsigned char sha1[20], void *unused)
+static void show_one_alternate_ref(const char *refname,
+				   const struct object_id *oid,
+				   void *data)
 {
-	show_ref(".have", sha1);
-	return 0;
-}
+	struct oidset *seen = data;
 
-static void collect_one_alternate_ref(const char *refname,
-				      const struct object_id *oid,
-				      void *data)
-{
-	struct sha1_array *sa = data;
-	sha1_array_append(sa, oid->hash);
+	if (oidset_insert(seen, oid))
+		return;
+
+	show_ref(".have", oid->hash);
 }
 
 static void write_head_info(void)
 {
-	struct sha1_array sa = SHA1_ARRAY_INIT;
+	static struct oidset seen = OIDSET_INIT;
 
-	for_each_alternate_ref(collect_one_alternate_ref, &sa);
-	sha1_array_for_each_unique(&sa, show_one_alternate_sha1, NULL);
-	sha1_array_clear(&sa);
+	for_each_alternate_ref(show_one_alternate_ref, &seen);
+	oidset_clear(&seen);
 	for_each_ref(show_ref_cb, NULL);
 	if (!sent_capabilities)
 		show_ref("capabilities^{}", null_sha1);

Re: [PATCH 09/12] receive-pack: use oidset to de-duplicate .have lines

From: Jeff King <hidden>
Date: 2017-01-25 19:54:31

On Wed, Jan 25, 2017 at 11:32:05AM -0800, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

If you have an alternate object store with a very large
number of refs, the peak memory usage of the sha1_array can
grow high, even if most of them are duplicates that end up
not being printed at all.
...
Signed-off-by: Jeff King <redacted>
---
 builtin/receive-pack.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

Nice.  

Incidentally, this also shows that the refnames in alternate ref
namespace will not matter.  We are to only show just one of many
anyway (and that name is masked and replaced with ".have").  Perhaps
we want to do 04/12 without refname?

I kept the refnames there as a "plausible" thing that a callback might
want. But I have ulterior motives. :)

I have another series which uses alternate refs as part of
check_connected(). Since that function calls rev-list, it needs to add
something like "rev-list --alternate-refs". Even check_connected() does
not care about the refnames, it seems like that rev-list option should
have some useful name for each ref. It can make things like "--source"
more sensible, instead of just reporting everything as a blank ".have".

I could be persuaded otherwise, though.  I don't think we pay a huge for
the refnames here. The generating for-each-ref has them either way, and
the receiving side only keeps one in memory at a time. So we are really
only paying to send them across the pipe and parse them, which doesn't
seem extravagant. I didn't actually measure it, though.

-Peff

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help