| Message ID | 7d9e67bf-e057-694c-c976-ba19e9521882@web.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | packfile: use oidset for bad objects | expand |
Am 11.09.21 um 18:08 schrieb René Scharfe: > fill_midx_entry() finds the position of an object ID and passes it to > nth_midxed_pack_entry(), which uses the position to look up the object > ID for its own purposes. Inline the latter into the former to avoid > that lookup. > Forgot: Reported-by: Jeff King <peff@peff.net> > Signed-off-by: René Scharfe <l.s.r@web.de> René
On Sat, Sep 11, 2021 at 06:08:42PM +0200, René Scharfe wrote: > fill_midx_entry() finds the position of an object ID and passes it to > nth_midxed_pack_entry(), which uses the position to look up the object > ID for its own purposes. Inline the latter into the former to avoid > that lookup. Ah, I see what you mean now by "inline" in the other part of the thread. Yes, I think this makes sense since there is no other reasonable caller of the nth_midxed_pack_entry() helper (and its one caller is itself trivial). > @@ -304,8 +307,7 @@ static int nth_midxed_pack_entry(struct repository *r, > if (!is_pack_valid(p)) > return 0; > > - nth_midxed_object_oid(&oid, m, pos); > - if (oidset_contains(&p->bad_objects, &oid)) > + if (oidset_contains(&p->bad_objects, oid)) > return 0; So we get to avoid the nth_midxed_object_oid() copy entirely. Very nice. Compared to the code before your series, we still have an extra function call to oidset_contains(), which will (in the common case) notice we have no entries and immediately return. But I think that's getting into pointless micro-optimization. -Peff
Am 11.09.21 um 19:07 schrieb Jeff King: > On Sat, Sep 11, 2021 at 06:08:42PM +0200, René Scharfe wrote: > >> @@ -304,8 +307,7 @@ static int nth_midxed_pack_entry(struct repository *r, >> if (!is_pack_valid(p)) >> return 0; >> >> - nth_midxed_object_oid(&oid, m, pos); >> - if (oidset_contains(&p->bad_objects, &oid)) >> + if (oidset_contains(&p->bad_objects, oid)) >> return 0; > > So we get to avoid the nth_midxed_object_oid() copy entirely. Very nice. > > Compared to the code before your series, we still have an extra function > call to oidset_contains(), which will (in the common case) notice we > have no entries and immediately return. But I think that's getting into > pointless micro-optimization. Right. I measure a 0.5% slowdown for git multi-pack-index verify. An inline oidset_size call avoids it. That's easy enough to add, so let's have it! René
On Sat, Sep 11, 2021 at 10:31:34PM +0200, René Scharfe wrote: > Am 11.09.21 um 19:07 schrieb Jeff King: > > On Sat, Sep 11, 2021 at 06:08:42PM +0200, René Scharfe wrote: > > > >> @@ -304,8 +307,7 @@ static int nth_midxed_pack_entry(struct repository *r, > >> if (!is_pack_valid(p)) > >> return 0; > >> > >> - nth_midxed_object_oid(&oid, m, pos); > >> - if (oidset_contains(&p->bad_objects, &oid)) > >> + if (oidset_contains(&p->bad_objects, oid)) > >> return 0; > > > > So we get to avoid the nth_midxed_object_oid() copy entirely. Very nice. > > > > Compared to the code before your series, we still have an extra function > > call to oidset_contains(), which will (in the common case) notice we > > have no entries and immediately return. But I think that's getting into > > pointless micro-optimization. > > Right. I measure a 0.5% slowdown for git multi-pack-index verify. An > inline oidset_size call avoids it. That's easy enough to add, so let's > have it! I don't mind that, but I wonder if we can have our cake and eat it, too. oidset_contains() is short, too, and could be inlined. Or if we're worried about the size of the embedded kh_get_oid_set() getting inlined, we could do something like: static inline int oidset_contains(const struct oidset *set, const struct object_id *oid) { if (!oidset_size(set)) return 0; return oidset_contains_func(set, oid); } That saves callers from having to deal with it, at the expense of a slightly complicated oidset implementation. I guess it's an extra integer comparison for callers that _do_ expect to have a non-empty set. So maybe it is better left to the caller to decide whether to optimize in this way. (A totally inline oidset_contains() avoids the extra check, but possibly at the cost of larger code size). -Peff
Am 11.09.21 um 23:20 schrieb Jeff King: > On Sat, Sep 11, 2021 at 10:31:34PM +0200, René Scharfe wrote: > >> Am 11.09.21 um 19:07 schrieb Jeff King: >>> On Sat, Sep 11, 2021 at 06:08:42PM +0200, René Scharfe wrote: >>> >>>> @@ -304,8 +307,7 @@ static int nth_midxed_pack_entry(struct repository *r, >>>> if (!is_pack_valid(p)) >>>> return 0; >>>> >>>> - nth_midxed_object_oid(&oid, m, pos); >>>> - if (oidset_contains(&p->bad_objects, &oid)) >>>> + if (oidset_contains(&p->bad_objects, oid)) >>>> return 0; >>> >>> So we get to avoid the nth_midxed_object_oid() copy entirely. Very nice. >>> >>> Compared to the code before your series, we still have an extra function >>> call to oidset_contains(), which will (in the common case) notice we >>> have no entries and immediately return. But I think that's getting into >>> pointless micro-optimization. >> >> Right. I measure a 0.5% slowdown for git multi-pack-index verify. An >> inline oidset_size call avoids it. That's easy enough to add, so let's >> have it! > > I don't mind that, but I wonder if we can have our cake and eat it, too. > > oidset_contains() is short, too, and could be inlined. Or if we're > worried about the size of the embedded kh_get_oid_set() getting inlined, > we could do something like: > > static inline int oidset_contains(const struct oidset *set, const > struct object_id *oid) > { > if (!oidset_size(set)) > return 0; > return oidset_contains_func(set, oid); > } > > That saves callers from having to deal with it, at the expense of a > slightly complicated oidset implementation. > > I guess it's an extra integer comparison for callers that _do_ expect to > have a non-empty set. So maybe it is better left to the caller to > decide whether to optimize in this way. > > (A totally inline oidset_contains() avoids the extra check, but possibly > at the cost of larger code size). I wondered the same. Inlining oidset_contains() would follow the spirit of khash. It adds 16KB to my build (ca. 688 bytes per caller). Hmm. I expected the hybrid approach with an inlined emptiness check and a shared actual contains function to be as fast as the original code, due to caching. I actually saw the 0.1% slowdown of git multi-pack-index verify when I added a fake bad object at the end of prepare_midx_pack() to simulate a non-empty oidset. Hmm! Both are probably defensible, but for this series I took the more targeted approach to limit the impact. René
diff --git a/midx.c b/midx.c index 01623fb339..59517938a8 100644 --- a/midx.c +++ b/midx.c @@ -276,14 +276,17 @@ uint32_t nth_midxed_pack_int_id(struct multi_pack_index *m, uint32_t pos) (off_t)pos * MIDX_CHUNK_OFFSET_WIDTH); } -static int nth_midxed_pack_entry(struct repository *r, - struct multi_pack_index *m, - struct pack_entry *e, - uint32_t pos) +int fill_midx_entry(struct repository * r, + const struct object_id *oid, + struct pack_entry *e, + struct multi_pack_index *m) { + uint32_t pos; uint32_t pack_int_id; struct packed_git *p; - struct object_id oid; + + if (!bsearch_midx(oid, m, &pos)) + return 0; if (pos >= m->num_objects) return 0; @@ -304,8 +307,7 @@ static int nth_midxed_pack_entry(struct repository *r, if (!is_pack_valid(p)) return 0; - nth_midxed_object_oid(&oid, m, pos); - if (oidset_contains(&p->bad_objects, &oid)) + if (oidset_contains(&p->bad_objects, oid)) return 0; e->offset = nth_midxed_offset(m, pos); @@ -314,19 +316,6 @@ static int nth_midxed_pack_entry(struct repository *r, return 1; } -int fill_midx_entry(struct repository * r, - const struct object_id *oid, - struct pack_entry *e, - struct multi_pack_index *m) -{ - uint32_t pos; - - if (!bsearch_midx(oid, m, &pos)) - return 0; - - return nth_midxed_pack_entry(r, m, e, pos); -} - /* Match "foo.idx" against either "foo.pack" _or_ "foo.idx". */ static int cmp_idx_or_pack_name(const char *idx_or_pack_name, const char *idx_name)
fill_midx_entry() finds the position of an object ID and passes it to nth_midxed_pack_entry(), which uses the position to look up the object ID for its own purposes. Inline the latter into the former to avoid that lookup. Signed-off-by: René Scharfe <l.s.r@web.de> --- midx.c | 29 +++++++++-------------------- 1 file changed, 9 insertions(+), 20 deletions(-) -- 2.33.0