Skip to content

Commit 89a1ab8

Browse files
derrickstoleegitster
authored andcommitted
pack-bitmap: remove trace2 region from hot path
The trace2 region around the call to lazy_bitmap_for_commit() in bitmap_for_commit() was added in 28cd730 (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. [1] https://lore.kernel.org/git/[email protected]/ To help with the 'git blame' output in this region, add a comment that warns against adding a trace2 region. Delete a test from t5310 that used that trace output to check that this lookup optimization was activated. To create this kind of test again in the future, the stopwatch traces mentioned earlier could be used as a signal that we activated this code path. Helpedy-by: Junio C Hamano <[email protected]> Signed-off-by: Derrick Stolee <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
1 parent 969a564 commit 89a1ab8

File tree

2 files changed

+1
-9
lines changed

2 files changed

+1
-9
lines changed

pack-bitmap.c

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -830,10 +830,9 @@ struct ewah_bitmap *bitmap_for_commit(struct bitmap_index *bitmap_git,
830830
if (!bitmap_git->table_lookup)
831831
return NULL;
832832

833-
trace2_region_enter("pack-bitmap", "reading_lookup_table", the_repository);
833+
/* this is a fairly hot codepath - no trace2_region please */
834834
/* NEEDSWORK: cache misses aren't recorded */
835835
bitmap = lazy_bitmap_for_commit(bitmap_git, commit);
836-
trace2_region_leave("pack-bitmap", "reading_lookup_table", the_repository);
837836
if (!bitmap)
838837
return NULL;
839838
return lookup_stored_bitmap(bitmap);

t/t5310-pack-bitmaps.sh

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -455,13 +455,6 @@ test_expect_success 'verify writing bitmap lookup table when enabled' '
455455
grep "\"label\":\"writing_lookup_table\"" trace2
456456
'
457457

458-
test_expect_success 'lookup table is actually used to traverse objects' '
459-
git repack -adb &&
460-
GIT_TRACE2_EVENT="$(pwd)/trace3" \
461-
git rev-list --use-bitmap-index --count --all &&
462-
grep "\"label\":\"reading_lookup_table\"" trace3
463-
'
464-
465458
test_expect_success 'truncated bitmap fails gracefully (lookup table)' '
466459
test_config pack.writebitmaphashcache false &&
467460
git repack -adb &&

0 commit comments

Comments
 (0)