Thread (3 messages) flat view 3 messages, 3 authors, 2022-09-23

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

From: Junio C Hamano <hidden>
Date: 2022-09-23 17:38:14

"Derrick Stolee via GitGitGadget" [off-list ref] writes:
    I noticed this while trying to backport the Abhradeep's lookup table
    work into GitHub's fork. Something went wrong in that process, causing
    this region to significantly slow down. It turns out that slow down does
    not reproduce on current 'master', which is good. I must have missed
    something while I was backporting.
    
    Regardless, the use of trace2_region_enter() here should be removed or
    replaced. For the sake of 2.38.0, this simple removal is safe enough.
Well, not doing this removal is even safer, if we know that it is
not hurting in the -rc code.  It would be better than breaking our
tests without knowing ;-)  I am not upset that the patch broke the
test, by the way.  Compared to things silently breaking without
surfacing, a loud breakage in tests is a much easier thing to
handle.

My test run with the attached just finished, so that's what I am
going to queue tentatively on top.

Thanks.

 pack-bitmap.c           | 1 +
 t/t5310-pack-bitmaps.sh | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)
diff --git c/pack-bitmap.c w/pack-bitmap.c
index 13457dd77e..a2bbbbd811 100644
--- c/pack-bitmap.c
+++ w/pack-bitmap.c
@@ -830,6 +830,7 @@ struct ewah_bitmap *bitmap_for_commit(struct bitmap_index *bitmap_git,
 		if (!bitmap_git->table_lookup)
 			return NULL;
 
+		/* this is a fairly hot codepath - no trace2_region please */
 		/* NEEDSWORK: cache misses aren't recorded */
 		bitmap = lazy_bitmap_for_commit(bitmap_git, commit);
 		if (!bitmap)
diff --git c/t/t5310-pack-bitmaps.sh w/t/t5310-pack-bitmaps.sh
index 7e50f8e765..2b1efc923b 100755
--- c/t/t5310-pack-bitmaps.sh
+++ w/t/t5310-pack-bitmaps.sh
@@ -455,7 +455,7 @@ test_expect_success 'verify writing bitmap lookup table when enabled' '
 	grep "\"label\":\"writing_lookup_table\"" trace2
 '
 
-test_expect_success 'lookup table is actually used to traverse objects' '
+: test_expect_success 'lookup table is actually used to traverse objects' '
 	git repack -adb &&
 	GIT_TRACE2_EVENT="$(pwd)/trace3" \
 		git rev-list --use-bitmap-index --count --all &&
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help