| Message ID | 20200214182218.GE150965@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | combining object filters and bitmaps | expand |
On Fri, Feb 14, 2020 at 01:22:18PM -0500, Jeff King wrote: > There are a few operations in rev-list that are optimized for bitmaps. > Rather than having the code inline in cmd_rev_list(), let's move them > into helpers. This not only makes the flow of the main function simpler, > but it lets us replace the complex "can we do the optimization?" > conditionals with a series of early returns from the functions. That > also makes it easy to add comments explaining those conditions. Makes sense. > Signed-off-by: Jeff King <peff@peff.net> > --- > builtin/rev-list.c | 88 +++++++++++++++++++++++++++++++++++----------- > 1 file changed, 67 insertions(+), 21 deletions(-) > > diff --git a/builtin/rev-list.c b/builtin/rev-list.c > index 4cb5a52dee..38c5ca5603 100644 > --- a/builtin/rev-list.c > +++ b/builtin/rev-list.c > @@ -364,6 +364,69 @@ static inline int parse_missing_action_value(const char *value) > return 0; > } > > +static int try_bitmap_count(struct rev_info *revs) > +{ > + uint32_t commit_count; > + int max_count; > + struct bitmap_index *bitmap_git; > + > + /* This function only handles counting, not general traversal. */ > + if (!revs->count) > + return -1; > + > + /* > + * A bitmap result can't know left/right, etc, because we don't > + * actually traverse. > + */ > + if (revs->left_right || revs->cherry_mark) > + return -1; > + > + /* > + * This must be saved before doing any walking, since the revision > + * machinery will count it down to zero while traversing. > + */ > + max_count = revs->max_count; > + > + bitmap_git = prepare_bitmap_walk(revs); > + if (!bitmap_git) > + return -1; > + > + count_bitmap_commit_list(bitmap_git, &commit_count, NULL, NULL, NULL); > + if (max_count >= 0 && max_count < commit_count) > + commit_count = max_count; > + > + printf("%d\n", commit_count); > + free_bitmap_index(bitmap_git); > + return 0; > +} > + > +static int try_bitmap_traversal(struct rev_info *revs) > +{ > + struct bitmap_index *bitmap_git; > + > + /* > + * We can't use a bitmap result with a traversal limit, since the set > + * of commits we'd get would be essentially random. > + */ > + if (revs->max_count >= 0) > + return -1; > + > + /* > + * Our bitmap result will return all objects, and we're not > + * yet prepared to show only particular types. > + */ > + if (!revs->tag_objects || !revs->tree_objects || !revs->blob_objects) > + return -1; > + > + bitmap_git = prepare_bitmap_walk(revs); > + if (!bitmap_git) > + return -1; > + > + traverse_bitmap_commit_list(bitmap_git, &show_object_fast); > + free_bitmap_index(bitmap_git); > + return 0; > +} > + > int cmd_rev_list(int argc, const char **argv, const char *prefix) > { > struct rev_info revs; > @@ -534,27 +597,10 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix) > progress = start_delayed_progress(show_progress, 0); > > if (use_bitmap_index) { > - if (revs.count && !revs.left_right && !revs.cherry_mark) { > - uint32_t commit_count; > - int max_count = revs.max_count; > - struct bitmap_index *bitmap_git; > - if ((bitmap_git = prepare_bitmap_walk(&revs))) { > - count_bitmap_commit_list(bitmap_git, &commit_count, NULL, NULL, NULL); > - if (max_count >= 0 && max_count < commit_count) > - commit_count = max_count; > - printf("%d\n", commit_count); > - free_bitmap_index(bitmap_git); > - return 0; > - } > - } else if (revs.max_count < 0 && > - revs.tag_objects && revs.tree_objects && revs.blob_objects) { > - struct bitmap_index *bitmap_git; > - if ((bitmap_git = prepare_bitmap_walk(&revs))) { > - traverse_bitmap_commit_list(bitmap_git, &show_object_fast); > - free_bitmap_index(bitmap_git); > - return 0; > - } > - } > + if (!try_bitmap_count(&revs)) > + return 0; > + if (!try_bitmap_traversal(&revs)) > + return 0; > } > > if (prepare_revision_walk(&revs)) > -- > 2.25.0.796.gcc29325708 The refactoring here is straightforward, and looks correct to my reading. Thanks, Taylor
diff --git a/builtin/rev-list.c b/builtin/rev-list.c index 4cb5a52dee..38c5ca5603 100644 --- a/builtin/rev-list.c +++ b/builtin/rev-list.c @@ -364,6 +364,69 @@ static inline int parse_missing_action_value(const char *value) return 0; } +static int try_bitmap_count(struct rev_info *revs) +{ + uint32_t commit_count; + int max_count; + struct bitmap_index *bitmap_git; + + /* This function only handles counting, not general traversal. */ + if (!revs->count) + return -1; + + /* + * A bitmap result can't know left/right, etc, because we don't + * actually traverse. + */ + if (revs->left_right || revs->cherry_mark) + return -1; + + /* + * This must be saved before doing any walking, since the revision + * machinery will count it down to zero while traversing. + */ + max_count = revs->max_count; + + bitmap_git = prepare_bitmap_walk(revs); + if (!bitmap_git) + return -1; + + count_bitmap_commit_list(bitmap_git, &commit_count, NULL, NULL, NULL); + if (max_count >= 0 && max_count < commit_count) + commit_count = max_count; + + printf("%d\n", commit_count); + free_bitmap_index(bitmap_git); + return 0; +} + +static int try_bitmap_traversal(struct rev_info *revs) +{ + struct bitmap_index *bitmap_git; + + /* + * We can't use a bitmap result with a traversal limit, since the set + * of commits we'd get would be essentially random. + */ + if (revs->max_count >= 0) + return -1; + + /* + * Our bitmap result will return all objects, and we're not + * yet prepared to show only particular types. + */ + if (!revs->tag_objects || !revs->tree_objects || !revs->blob_objects) + return -1; + + bitmap_git = prepare_bitmap_walk(revs); + if (!bitmap_git) + return -1; + + traverse_bitmap_commit_list(bitmap_git, &show_object_fast); + free_bitmap_index(bitmap_git); + return 0; +} + int cmd_rev_list(int argc, const char **argv, const char *prefix) { struct rev_info revs; @@ -534,27 +597,10 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix) progress = start_delayed_progress(show_progress, 0); if (use_bitmap_index) { - if (revs.count && !revs.left_right && !revs.cherry_mark) { - uint32_t commit_count; - int max_count = revs.max_count; - struct bitmap_index *bitmap_git; - if ((bitmap_git = prepare_bitmap_walk(&revs))) { - count_bitmap_commit_list(bitmap_git, &commit_count, NULL, NULL, NULL); - if (max_count >= 0 && max_count < commit_count) - commit_count = max_count; - printf("%d\n", commit_count); - free_bitmap_index(bitmap_git); - return 0; - } - } else if (revs.max_count < 0 && - revs.tag_objects && revs.tree_objects && revs.blob_objects) { - struct bitmap_index *bitmap_git; - if ((bitmap_git = prepare_bitmap_walk(&revs))) { - traverse_bitmap_commit_list(bitmap_git, &show_object_fast); - free_bitmap_index(bitmap_git); - return 0; - } - } + if (!try_bitmap_count(&revs)) + return 0; + if (!try_bitmap_traversal(&revs)) + return 0; } if (prepare_revision_walk(&revs))
There are a few operations in rev-list that are optimized for bitmaps. Rather than having the code inline in cmd_rev_list(), let's move them into helpers. This not only makes the flow of the main function simpler, but it lets us replace the complex "can we do the optimization?" conditionals with a series of early returns from the functions. That also makes it easy to add comments explaining those conditions. Signed-off-by: Jeff King <peff@peff.net> --- builtin/rev-list.c | 88 +++++++++++++++++++++++++++++++++++----------- 1 file changed, 67 insertions(+), 21 deletions(-)