On Mon, Sep 26 2022, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <derrickstolee@xxxxxxxxxx>
>
> The trace2 region around the call to lazy_bitmap_for_commit() in
> bitmap_for_commit() was added in 28cd730680d (pack-bitmap: prepare to
> read lookup table extension, 2022-08-14). While adding trace2 regions is
> typically helpful for tracking performance, this method is called
> possibly thousands of times as a commit walk explores commit history
> looking for a matching bitmap. When trace2 output is enabled, this
> region is emitted many times and performance is throttled by that
> output.
>
> For now, remove these regions entirely.
>
> This is a critical path, and it would be valuable to measure that the
> time spent in bitmap_for_commit() does not increase when using the
> commit lookup table. The best way to do that would be to use a mechanism
> that sums the time spent in a region and reports a single value at the
> end of the process. This technique was introduced but not merged by [1]
> so maybe this example presents some justification to revisit that
> approach.
Just getting rid of this seems like a good thing for now.
But aside: Yes, one way to mitigate this rather than removing the
tracing would be to make it really fast.
But just skimming pack-bitmap.c do we really need trace2 at the
granularity of a single commit? Looking at who calls bitmap_for_commit()
wouldn't something like this sketch-out be much more useful?:
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 9d5205055a5..439aec220c7 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -830,10 +830,8 @@ struct ewah_bitmap *bitmap_for_commit(struct bitmap_index *bitmap_git,
if (!bitmap_git->table_lookup)
return NULL;
- trace2_region_enter("pack-bitmap", "reading_lookup_table", the_repository);
/* NEEDSWORK: cache misses aren't recorded */
bitmap = lazy_bitmap_for_commit(bitmap_git, commit);
- trace2_region_leave("pack-bitmap", "reading_lookup_table", the_repository);
if (!bitmap)
return NULL;
return lookup_stored_bitmap(bitmap);
@@ -1042,6 +1040,7 @@ static struct bitmap *find_objects(struct bitmap_index *bitmap_git,
* The ones without bitmaps in the index will be stored in the
* `not_mapped_list` for further processing.
*/
+ /* begin trace2 find roots? */
while (roots) {
struct object *object = roots->item;
roots = roots->next;
@@ -1055,6 +1054,7 @@ static struct bitmap *find_objects(struct bitmap_index *bitmap_git,
object_list_insert(object, ¬_mapped);
}
+ /* end trace2 find roots? */
/*
* Best case scenario: We found bitmaps for all the roots,
* so the resulting `or` bitmap has the full reachability analysis
@@ -1100,7 +1100,7 @@ static struct bitmap *find_objects(struct bitmap_index *bitmap_git,
incdata.base = base;
incdata.seen = seen;
- revs->include_check = should_include;
+ revs->include_check = should_include; // Will call bitmap_for_commit()
revs->include_check_obj = should_include_obj;
revs->include_check_data = &incdata;
@@ -1110,9 +1110,11 @@ static struct bitmap *find_objects(struct bitmap_index *bitmap_git,
show_data.bitmap_git = bitmap_git;
show_data.base = base;
+ /* begin trace2 rev list? */
traverse_commit_list(revs,
show_commit, show_object,
&show_data);
+ /* end trace2 rev list? */
revs->include_check = NULL;
revs->include_check_obj = NULL;
This is *not* the same as stricking the tracing into
bitmap_for_commit(), but do we really need the tracing that far inside
our loop?
