mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <>
To: Derrick Stolee via GitGitGadget <>
	Derrick Stolee <>
Subject: Re: [PATCH v2] pack-bitmap: remove trace2 region from hot path
Date: Mon, 26 Sep 2022 17:01:24 +0200	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

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

> From: Derrick Stolee <>
> 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? */
	 				     show_commit, show_object,
	+		/* 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?

  reply	other threads:[~2022-09-26 16:20 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-23 13:00 [PATCH] pack-bitmap: remove trace2 region from hot path Derrick Stolee via GitGitGadget
2022-09-23 17:05 ` Junio C Hamano
2022-09-23 17:25   ` Junio C Hamano
2022-09-23 17:36 ` Junio C Hamano
2022-09-23 18:07   ` Derrick Stolee
2022-09-23 19:17   ` Taylor Blau
2022-09-26 13:17 ` [PATCH v2] " Derrick Stolee via GitGitGadget
2022-09-26 15:01   ` Ævar Arnfjörð Bjarmason [this message]
2022-09-26 17:31     ` Derrick Stolee
2022-09-27 18:37 ` [PATCH] " Jeff Hostetler

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).