Re: [PATCH v2 20/20] pack-revindex.c: avoid direct revindex access in 'offset_to_pack_pos()'

2 messages, 2 authors, 2021-01-14 · open the first message on its own page

Re: [PATCH v2 20/20] pack-revindex.c: avoid direct revindex access in 'offset_to_pack_pos()'

From: Junio C Hamano <hidden>
Date: 2021-01-14 06:43:18

Taylor Blau [off-list ref] writes:
To prepare for on-disk reverse indexes, remove a spot in
'offset_to_pack_pos()' that looks at the 'revindex' array in 'struct
packed_git'.
Hmph, I somehow would have expected that this clean-up would be done
before step [18/20], but that does not matter in the end.  The end
result looks fairly clean.

I wonder if the call overhead to pack_pos_to_offset(), relative to
the direct indexing of an in-core array revindex[] followed by an
access to a member .offset that we used to do, makes a measurable
difference in this tight loop, though.
quoted hunk
diff --git a/pack-revindex.c b/pack-revindex.c
index a508d5f0a4..5e69bc7372 100644
--- a/pack-revindex.c
+++ b/pack-revindex.c
@@ -177,21 +177,21 @@ int load_pack_revindex(struct packed_git *p)
 int offset_to_pack_pos(struct packed_git *p, off_t ofs, uint32_t *pos)
 {
 	unsigned lo, hi;
-	const struct revindex_entry *revindex;
 
 	if (load_pack_revindex(p) < 0)
 		return -1;
 
 	lo = 0;
 	hi = p->num_objects + 1;
-	revindex = p->revindex;
 
 	do {
 		const unsigned mi = lo + (hi - lo) / 2;
-		if (revindex[mi].offset == ofs) {
+		off_t got = pack_pos_to_offset(p, mi);
+
+		if (got == ofs) {
 			*pos = mi;
 			return 0;
-		} else if (ofs < revindex[mi].offset)
+		} else if (ofs < got)
 			hi = mi;
 		else
 			lo = mi + 1;

Re: [PATCH v2 20/20] pack-revindex.c: avoid direct revindex access in 'offset_to_pack_pos()'

From: Taylor Blau <hidden>
Date: 2021-01-14 16:57:10

On Wed, Jan 13, 2021 at 10:42:29PM -0800, Junio C Hamano wrote:
Taylor Blau [off-list ref] writes:
quoted
To prepare for on-disk reverse indexes, remove a spot in
'offset_to_pack_pos()' that looks at the 'revindex' array in 'struct
packed_git'.
Hmph, I somehow would have expected that this clean-up would be done
before step [18/20], but that does not matter in the end.  The end
result looks fairly clean.

I wonder if the call overhead to pack_pos_to_offset(), relative to
the direct indexing of an in-core array revindex[] followed by an
access to a member .offset that we used to do, makes a measurable
difference in this tight loop, though.
I'm skeptical that it does (take that with a grain of salt, since I
haven't done any per-function tests with perf, only "how long does it
take to run 'git cat-file --batch-check=%(objectsize:disk)' and so on").

But even if it were to make a difference, it'll get dwarfed in the next
series by the time that we now _don't_ have to spend building and
sorting the reverse index in memory for each new process.

Thanks,
Taylor
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help