Re: [PATCH 3/5] make object_directory.loose_objects_subdir_seen a bitmap

From: Eric Wong <e@80x24.org>
Date: Jun 28, 2021, 23:09 UTC
Message-ID: <20210628230953.GA9830@dcvr>
In-Reply-To: <496545dc-e372-401c-13f4-daa7ee765d39@web.de>

René Scharfe <l.s.r@web.de> wrote:

Show 6 quoted lines

> Am 27.06.21 um 04:47 schrieb Eric Wong:
> Anyway, it would look something like this:
> 
> 	size_t word_bits = bitsizeof(odb->loose_objects_subdir_seen[0]);
> 	size_t word_index = subdir_nr / word_bits;
> 	size_t mask = 1 << (subdir_nr % word_bits);

<snip> yeah, I missed bitsizeof :x

Show 11 quoted lines

> > --- a/object-store.h
> > +++ b/object-store.h
> > @@ -22,7 +22,7 @@ struct object_directory {
> >  	 *
> >  	 * Be sure to call odb_load_loose_cache() before using.
> >  	 */
> > -	char loose_objects_subdir_seen[256];
> > +	uint32_t loose_objects_subdir_seen[8]; /* 256 bits */
> 
> Perhaps	DIV_ROUND_UP(256, bitsizeof(uint32_t))?  The comment explains
> it nicely already, though.

I think I'll keep my original there... IMHO the macros obfuscate the meaning for those less familiar with our codebase (myself included :x)

Show 7 quoted lines

> >  	struct oid_array loose_objects_cache[256];
> >
> >  	/*
> >
> 
> Summary: Good idea, the implementation looks correct, I stumbled
> over some of the names, bitsizeof() could be used.

Thanks for the review.

I'll squash up the following for v2 while awaiting feedback for the rest of the series:

diff --git a/object-file.c b/object-file.c
index d33b84c4a4..6c397fb4f1 100644
--- a/object-file.c
+++ b/object-file.c
@@ -2463,16 +2463,17 @@ struct oidtree *odb_loose_cache(struct object_directory *odb,
 {
 	int subdir_nr = oid->hash[0];
 	struct strbuf buf = STRBUF_INIT;
-	size_t BM_SIZE = sizeof(odb->loose_objects_subdir_seen[0]) * CHAR_BIT;
+	size_t word_bits = bitsizeof(odb->loose_objects_subdir_seen[0]);
+	size_t word_index = subdir_nr / word_bits;
+	size_t mask = 1 << (subdir_nr % word_bits);
 	uint32_t *bitmap;
-	uint32_t bit = 1 << (subdir_nr % BM_SIZE);
 
 	if (subdir_nr < 0 ||
-	    subdir_nr >= ARRAY_SIZE(odb->loose_objects_subdir_seen) * BM_SIZE)
+	    subdir_nr >= bitsizeof(odb->loose_objects_subdir_seen))
 		BUG("subdir_nr out of range");
 
-	bitmap = &odb->loose_objects_subdir_seen[subdir_nr / BM_SIZE];
-	if (*bitmap & bit)
+	bitmap = &odb->loose_objects_subdir_seen[word_index];
+	if (*bitmap & mask)
 		return &odb->loose_objects_cache;
 
 	strbuf_addstr(&buf, odb->path);
@@ -2480,7 +2481,7 @@ struct oidtree *odb_loose_cache(struct object_directory *odb,
 				    append_loose_object,
 				    NULL, NULL,
 				    &odb->loose_objects_cache);
-	*bitmap |= bit;
+	*bitmap |= mask;
 	strbuf_release(&buf);
 	return &odb->loose_objects_cache;
 }