Re: [PATCH v2] pack-bitmap: remove trace2 region from hot path

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 26, 2022, 15:01 UTC
Message-ID: <220926.868rm618ds.gmgdl@evledraar.gmail.com>
In-Reply-To: <pull.1365.v2.git.1664198277250.gitgitgadget@gmail.com>

On Mon, Sep 26 2022, Derrick Stolee via GitGitGadget wrote:

Show 20 quoted lines

> From: Derrick Stolee <derrickstolee@github.com>
>
> 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, &not_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?