@matheusbsbs / matheus-bagliano / commits / c525ce95b4

Fork 0

commit-graph: check all leading directories in changed path Bloom filters

The file 'dir/subdir/file' can only be modified if its leading directories 'dir' and 'dir/subdir' are modified as well. So when checking modified path Bloom filters looking for commits modifying a path with multiple path components, then check not only the full path in the Bloom filters, but all its leading directories as well. Take care to check these paths in "deepest first" order, because it's the full path that is least likely to be modified, and the Bloom filter queries can short circuit sooner. This can significantly reduce the average false positive rate, by about an order of magnitude or three(!), and can further speed up pathspec-limited revision walks. The table below compares the average false positive rate and runtime of git rev-list HEAD -- "$path" before and after this change for 5000+ randomly* selected paths from each repository: Average false Average Average positive rate runtime runtime before after before after difference ------------------------------------------------------------------ git 3.220% 0.7853% 0.0558s 0.0387s -30.6% linux 2.453% 0.0296% 0.1046s 0.0766s -26.8% tensorflow 2.536% 0.6977% 0.0594s 0.0420s -29.2% *Path selection was done with the following pipeline: git ls-tree -r --name-only HEAD | sort -R | head -n 5000 The improvements in runtime are much smaller than the improvements in average false positive rate, as we are clearly reaching diminishing returns here. However, all these timings depend on that accessing tree objects is reasonably fast (warm caches). If we had a partial clone and the tree objects had to be fetched from a promisor remote, e.g.: $ git clone --filter=tree:0 --bare file://.../webkit.git webkit.notrees.git $ git -C webkit.git -c core.modifiedPathBloomFilters=1 \ commit-graph write --reachable $ cp webkit.git/objects/info/commit-graph webkit.notrees.git/objects/info/ $ git -C webkit.notrees.git -c core.modifiedPathBloomFilters=1 \ rev-list HEAD -- "$path" then checking all leading path component can reduce the runtime from over an hour to a few seconds (and this is with the clone and the promisor on the same machine). This adjusts the tracing values in t4216-log-bloom.sh, which provides a concrete way to notice the improvement. Helped-by: Taylor Blau <me@ttaylorr.com> Helped-by: René Scharfe <l.s.r@web.de> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

SZEDER Gábor committed Jul 1, 2020 at 13:27 UTC c525ce95b46b34f344c360dbef036cec3ea08e53

3 files changed +42 -12

revision.c

+37 -9

@@ -668,9 +668,10 @@ static void prepare_to_use_bloom_filter(struct rev_info *revs)

668 {

669 struct pathspec_item *pi;

670 char *path_alloc = NULL;

671 - const char *path;

671 + const char *path, *p;

672 int last_index;

673 - int len;

673 + size_t len;

674 + int path_component_nr = 1;

675

676 if (!revs->commits)

677 return;

@@ -707,8 +708,33 @@ static void prepare_to_use_bloom_filter(struct rev_info *revs)

708 return;

709 }

710

710 - revs->bloom_key = xmalloc(sizeof(struct bloom_key));

711 - fill_bloom_key(path, len, revs->bloom_key, revs->bloom_filter_settings);

711 + p = path;

712 + while (*p) {

713 + /*

714 + * At this point, the path is normalized to use Unix-style

715 + * path separators. This is required due to how the

716 + * changed-path Bloom filters store the paths.

717 + */

718 + if (*p == '/')

719 + path_component_nr++;

720 + p++;

721 + }

722 +

723 + revs->bloom_keys_nr = path_component_nr;

724 + ALLOC_ARRAY(revs->bloom_keys, revs->bloom_keys_nr);

725 +

726 + fill_bloom_key(path, len, &revs->bloom_keys[0],

727 + revs->bloom_filter_settings);

728 + path_component_nr = 1;

729 +

730 + p = path + len - 1;

731 + while (p > path) {

732 + if (*p == '/')

733 + fill_bloom_key(path, p - path,

734 + &revs->bloom_keys[path_component_nr++],

735 + revs->bloom_filter_settings);

736 + p--;

737 + }

738

739 if (trace2_is_enabled() && !bloom_filter_atexit_registered) {

740 atexit(trace2_bloom_filter_statistics_atexit);

@@ -722,7 +748,7 @@ static int check_maybe_different_in_bloom_filter(struct rev_info *revs,

748 struct commit *commit)

749 {

750 struct bloom_filter *filter;

725 - int result;

751 + int result = 1, j;

752

753 if (!revs->repo->objects->commit_graph)

754 return -1;

@@ -737,9 +763,11 @@ static int check_maybe_different_in_bloom_filter(struct rev_info *revs,

763 return -1;

764 }

765

740 - result = bloom_filter_contains(filter,

741 - revs->bloom_key,

742 - revs->bloom_filter_settings);

766 + for (j = 0; result && j < revs->bloom_keys_nr; j++) {

767 + result = bloom_filter_contains(filter,

768 + &revs->bloom_keys[j],

769 + revs->bloom_filter_settings);

770 + }

771

772 if (result)

773 count_bloom_filter_maybe++;

@@ -779,7 +807,7 @@ static int rev_compare_tree(struct rev_info *revs,

807 return REV_TREE_SAME;

808 }

809

782 - if (revs->bloom_key && !nth_parent) {

810 + if (revs->bloom_keys_nr && !nth_parent) {

811 bloom_ret = check_maybe_different_in_bloom_filter(revs, commit);

812

813 if (bloom_ret == 0)

revision.h

+4 -2

@@ -295,8 +295,10 @@ struct rev_info {

295 struct topo_walk_info *topo_walk_info;

296

297 /* Commit graph bloom filter fields */

298 - /* The bloom filter key for the pathspec */

299 - struct bloom_key *bloom_key;

298 + /* The bloom filter key(s) for the pathspec */

299 + struct bloom_key *bloom_keys;

300 + int bloom_keys_nr;

301 +

302 /*

303 * The bloom filter settings used to generate the key.

304 * This is loaded from the commit-graph being used.

t/t4216-log-bloom.sh

+1 -1

@@ -146,7 +146,7 @@ test_expect_success 'setup - add commit-graph to the chain with Bloom filters' '

146

147 test_bloom_filters_used_when_some_filters_are_missing () {

148 log_args=$1

149 - bloom_trace_prefix="statistics:{\"filter_not_present\":3,\"maybe\":8,\"definitely_not\":6"

149 + bloom_trace_prefix="statistics:{\"filter_not_present\":3,\"maybe\":6,\"definitely_not\":8"

150 setup "$log_args" &&

151 grep -q "$bloom_trace_prefix" "$TRASH_DIRECTORY/trace.perf" &&

152 test_cmp log_wo_bloom log_w_bloom