git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 09/12] bloom/diff: properly short-circuit on max_changes

From
Taylor Blau <me@ttaylorr.com>
Date
Sep 9, 2020, 15:23 UTC
Message-ID
<e5bdae44567e72dfc0d6cc9010cadf5d20903642.1599664389.git.me@ttaylorr.com>
In-Reply-To
<cover.1599664389.git.me@ttaylorr.com>
From: Derrick Stolee <dstolee@microsoft.com>

Commit e3696980 (diff: halt tree-diff early after max_changes, 2020-03-30) intended to create a mechanism to short-circuit a diff calculation after a certain number of paths were modified. By incrementing a "num_changes" counter throughout the recursive ll_diff_tree_paths(), this was supposed to match the number of changes that would be written into the changed-path Bloom filters. Unfortunately, this was not implemented correctly and instead misses simple cases like file modifications. This then does not stop very large changed-path filters from being written (unless they add or remove many files).

To start, change the implementation in ll_diff_tree_paths() to instead use the global diff_queue_diff struct's 'nr' member as the count. This is a way to simplify the logic instead of making more mistakes in the complicated diff code.

This has a drawback: the diff_queue_diff struct only lists the paths corresponding to blob changes, not their leading directories. Thus, get_or_compute_bloom_filter() needs an additional check to see if the hashmap with the leading directories becomes too large.

One reason why this was not caught by test cases was that the test in t4216-log-bloom.sh that was supposed to check this "too many changes" condition only checked this on the initial commit of a repository. The old logic counted these values correctly. Update this test in a few ways:

1. Use GIT_TEST_BLOOM_SETTINGS_MAX_CHANGED_PATHS to reduce the limit,
   allowing smaller commits to engage with this logic.
2. Create several interesting cases of edits, adds, removes, and mode
   changes (in the second commit). By testing both sides of the
   inequality with the *_MAX_CHANGED_PATHS variable, we can see that
   the count is exactly correct, so none of these changes are missed
   or over-counted.
3. Use the trace2 data value filter_found_large to verify that these
   commits are on the correct side of the limit.

Another way to verify the behavior is correct is through performance tests. By testing on my local copies of the Git repository and the Linux kernel repository, I could measure the effect of these short-circuits when computing a fresh commit-graph file with changed-path Bloom filters using the command

  GIT_TEST_BLOOM_SETTINGS_MAX_CHANGED_PATHS=N time \
    git commit-graph write --reachable --changed-paths
and reporting the wall time and resulting commit-graph size.
For Git, the results are
|        |      N=1       |       N=10     |      N=512     |
|--------|----------------|----------------|----------------|
| HEAD~1 | 10.90s  9.18MB | 11.11s  9.34MB | 11.31s  9.35MB |
| HEAD   |  9.21s  8.62MB | 11.11s  9.29MB | 11.29s  9.34MB |
For Linux, the results are
|        |       N=1      |     N=20      |     N=512     |
|--------|----------------|---------------|---------------|
| HEAD~1 | 61.28s  64.3MB | 76.9s  72.6MB | 77.6s  72.6MB |
| HEAD   | 49.44s  56.3MB | 68.7s  65.9MB | 69.2s  65.9MB |

Naturally, the improvement becomes much less as the limit grows, as fewer commits satisfy the short-circuit.

Reported-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 bloom.c              |  9 ++++-
 diff.h               |  2 -
 t/t4216-log-bloom.sh | 88 +++++++++++++++++++++++++++++++++++++++-----
 tree-diff.c          |  5 +--
 4 files changed, 88 insertions(+), 16 deletions(-)
diff --git a/bloom.c b/bloom.c
index 2d6aef9098..db9fb82437 100644
--- a/bloom.c
+++ b/bloom.c
@@ -222,7 +222,7 @@ struct bloom_filter *get_or_compute_bloom_filter(struct repository *r,
 		diff_tree_oid(NULL, &c->object.oid, "", &diffopt);
 	diffcore_std(&diffopt);
 
-	if (diffopt.num_changes <= settings->max_changed_paths) {
+	if (diff_queued_diff.nr <= settings->max_changed_paths) {
 		struct hashmap pathmap;
 		struct pathmap_hash_entry *e;
 		struct hashmap_iter iter;
@@ -259,6 +259,12 @@ struct bloom_filter *get_or_compute_bloom_filter(struct repository *r,
 			diff_free_filepair(diff_queued_diff.queue[i]);
 		}
 
+		if (hashmap_get_size(&pathmap) > settings->max_changed_paths) {
+			if (computed)
+				*computed |= BLOOM_TRUNC_LARGE;
+			goto cleanup;
+		}
+
 		filter->len = (hashmap_get_size(&pathmap) * settings->bits_per_entry + BITS_PER_WORD - 1) / BITS_PER_WORD;
 		filter->data = xcalloc(filter->len, sizeof(unsigned char));
 
@@ -268,6 +274,7 @@ struct bloom_filter *get_or_compute_bloom_filter(struct repository *r,
 			add_key_to_filter(&key, filter, settings);
 		}
 
+	cleanup:
 		hashmap_free_entries(&pathmap, struct pathmap_hash_entry, entry);
 	} else {
 		for (i = 0; i < diff_queued_diff.nr; i++)
diff --git a/diff.h b/diff.h
index e0c0af6286..1d32b71885 100644
--- a/diff.h
+++ b/diff.h
@@ -287,8 +287,6 @@ struct diff_options {
 
 	/* If non-zero, then stop computing after this many changes. */
 	int max_changes;
-	/* For internal use only. */
-	int num_changes;
 
 	int ita_invisible_in_index;
 /* white-space error highlighting */
diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh
index 47ddf2641f..f375e752cd 100755
--- a/t/t4216-log-bloom.sh
+++ b/t/t4216-log-bloom.sh
@@ -182,20 +182,90 @@ test_expect_success 'persist filter settings' '
 '
 
 test_expect_success 'correctly report changes over limit' '
-	git init 513changes &&
+	git init limits &&
 	(
-		cd 513changes &&
-		for i in $(test_seq 1 513)
+		cd limits &&
+		mkdir d &&
+		mkdir d/e &&
+
+		for i in $(test_seq 1 2)
 		do
-			echo $i >file$i.txt || return 1
+			printf $i >d/file$i.txt &&
+			printf $i >d/e/file$i.txt || return 1
 		done &&
-		git add . &&
+
+		mkdir mode &&
+		printf bash >mode/script.sh &&
+
+		mkdir foo &&
+		touch foo/bar &&
+		touch foo.txt &&
+
+		git add d foo foo.txt mode &&
 		git commit -m "files" &&
-		git commit-graph write --reachable --changed-paths &&
-		for i in $(test_seq 1 513)
+
+		# Commit has 7 file and 4 directory adds
+		GIT_TEST_BLOOM_SETTINGS_MAX_CHANGED_PATHS=10 \
+			GIT_TRACE2_EVENT="$(pwd)/trace" \
+			git commit-graph write --reachable --changed-paths &&
+		grep "\"max_changed_paths\":10" trace &&
+		grep "\"filter_computed\":1" trace &&
+		grep "\"filter_trunc_large\":1" trace &&
+
+		for path in $(git ls-tree -r --name-only HEAD)
 		do
-			git -c core.commitGraph=false log -- file$i.txt >expect &&
-			git log -- file$i.txt >actual &&
+			git -c commitGraph.readChangedPaths=false log \
+				-- $path >expect &&
+			git log -- $path >actual &&
+			test_cmp expect actual || return 1
+		done &&
+
+		# Make a variety of path changes
+		printf new1 >d/e/file1.txt &&
+		printf new2 >d/file2.txt &&
+		rm d/e/file2.txt &&
+		rm -r foo &&
+		printf text >foo &&
+		mkdir f &&
+		printf new1 >f/file1.txt &&
+
+		# including a mode-only change (counts as modified)
+		git update-index --chmod=+x mode/script.sh &&
+
+		git add foo d f &&
+		git commit -m "complicated" &&
+
+		# start from scratch and rebuild
+		rm -f .git/objects/info/commit-graph &&
+		GIT_TEST_BLOOM_SETTINGS_MAX_CHANGED_PATHS=10 \
+			GIT_TRACE2_EVENT="$(pwd)/trace-edit" \
+			git commit-graph write --reachable --changed-paths &&
+		grep "\"max_changed_paths\":10" trace-edit &&
+		grep "\"filter_computed\":2" trace-edit &&
+		grep "\"filter_trunc_large\":2" trace-edit &&
+
+		for path in $(git ls-tree -r --name-only HEAD)
+		do
+			git -c commitGraph.readChangedPaths=false log \
+				-- $path >expect &&
+			git log -- $path >actual &&
+			test_cmp expect actual || return 1
+		done &&
+
+		# start from scratch and rebuild
+		rm -f .git/objects/info/commit-graph &&
+		GIT_TEST_BLOOM_SETTINGS_MAX_CHANGED_PATHS=11 \
+			GIT_TRACE2_EVENT="$(pwd)/trace-update" \
+			git commit-graph write --reachable --changed-paths &&
+		grep "\"max_changed_paths\":11" trace-update &&
+		grep "\"filter_computed\":2" trace-update &&
+		grep "\"filter_trunc_large\":0" trace-update &&
+
+		for path in $(git ls-tree -r --name-only HEAD)
+		do
+			git -c commitGraph.readChangedPaths=false log \
+				-- $path >expect &&
+			git log -- $path >actual &&
 			test_cmp expect actual || return 1
 		done
 	)
diff --git a/tree-diff.c b/tree-diff.c
index 6ebad1a46f..7cebbb327e 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -434,7 +434,7 @@ static struct combine_diff_path *ll_diff_tree_paths(
 		if (diff_can_quit_early(opt))
 			break;
 
-		if (opt->max_changes && opt->num_changes > opt->max_changes)
+		if (opt->max_changes && diff_queued_diff.nr > opt->max_changes)
 			break;
 
 		if (opt->pathspec.nr) {
@@ -521,7 +521,6 @@ static struct combine_diff_path *ll_diff_tree_paths(
 
 			/* t↓ */
 			update_tree_entry(&t);
-			opt->num_changes++;
 		}
 
 		/* t > p[imin] */
@@ -539,7 +538,6 @@ static struct combine_diff_path *ll_diff_tree_paths(
 		skip_emit_tp:
 			/* ∀ pi=p[imin]  pi↓ */
 			update_tp_entries(tp, nparent);
-			opt->num_changes++;
 		}
 	}
 
@@ -557,7 +555,6 @@ struct combine_diff_path *diff_tree_paths(
 	const struct object_id **parents_oid, int nparent,
 	struct strbuf *base, struct diff_options *opt)
 {
-	opt->num_changes = 0;
 	p = ll_diff_tree_paths(p, oid, parents_oid, nparent, base, opt);
 
 	/*

-- 
2.28.0.462.g4ff11cec37
Previous: Taylor BlauNext: Taylor Blau
Message 21 of 75 in “more miscellaneous Bloom filter improvements, redux”
  1. 00/12 more miscellaneous Bloom filter improvements, reduxTaylor Blau, Sep 9, 2020
  2. 03/12 commit-graph: pass a 'struct repository *' in more placesTaylor Blau, Sep 9, 2020
  3. 11/12 commit-graph: rename 'split_commit_graph_opts'Taylor Blau, Sep 9, 2020
  4. 12/12 builtin/commit-graph.c: introduce '--max-new-filters=<n>'Taylor Blau, Sep 9, 2020
  5. Jeff KingSep 11, 2020
  6. Taylor BlauSep 11, 2020
  7. Taylor BlauSep 11, 2020
  8. Taylor BlauSep 14, 2020
  9. Derrick StoleeSep 14, 2020
  10. Taylor BlauSep 14, 2020
  11. Derrick StoleeSep 15, 2020
  12. Taylor BlauSep 15, 2020
  13. Junio C HamanoSep 15, 2020
  14. Taylor BlauSep 15, 2020
  15. Jeff KingSep 11, 2020
  16. Junio C HamanoSep 11, 2020
  17. 04/12 t/helper/test-read-graph.c: prepare repo settingsTaylor Blau, Sep 9, 2020
  18. 05/12 commit-graph: respect 'commitGraph.readChangedPaths'Taylor Blau, Sep 9, 2020
  19. 08/12 bloom: use provided 'struct bloom_filter_settings'Taylor Blau, Sep 9, 2020
  20. 06/12 commit-graph.c: store maximum changed pathsTaylor Blau, Sep 9, 2020
  21. 09/12 bloom/diff: properly short-circuit on max_changesTaylor Blau, Sep 9, 2020
  22. 10/12 bloom: encode out-of-bounds filters as non-emptyTaylor Blau, Sep 9, 2020
  23. Taylor BlauSep 10, 2020
  24. Taylor BlauSep 10, 2020
  25. Derrick StoleeSep 11, 2020
  26. 07/12 bloom: split 'get_bloom_filter()' in twoTaylor Blau, Sep 9, 2020
  27. 02/12 t4216: use an '&&'-chainTaylor Blau, Sep 9, 2020
  28. 01/12 commit-graph: introduce 'get_bloom_filter_settings()'Taylor Blau, Sep 9, 2020
  29. 00/13 more miscellaneous Bloom filter improvements, reduxTaylor Blau, Sep 16, 2020
  30. 04/13 t/helper/test-read-graph.c: prepare repo settingsTaylor Blau, Sep 16, 2020
  31. 03/13 commit-graph: pass a 'struct repository *' in more placesTaylor Blau, Sep 16, 2020
  32. 05/13 commit-graph: respect 'commitGraph.readChangedPaths'Taylor Blau, Sep 16, 2020
  33. 06/13 commit-graph.c: store maximum changed pathsTaylor Blau, Sep 16, 2020
  34. 02/13 t4216: use an '&&'-chainTaylor Blau, Sep 16, 2020
  35. 01/13 commit-graph: introduce 'get_bloom_filter_settings()'Taylor Blau, Sep 16, 2020
  36. 07/13 bloom: split 'get_bloom_filter()' in twoTaylor Blau, Sep 16, 2020
  37. 08/13 bloom: use provided 'struct bloom_filter_settings'Taylor Blau, Sep 16, 2020
  38. 10/13 bloom: encode out-of-bounds filters as non-emptyTaylor Blau, Sep 16, 2020
  39. SZEDER GáborSep 17, 2020
  40. Taylor BlauSep 17, 2020
  41. Junio C HamanoSep 18, 2020
  42. Taylor BlauSep 18, 2020
  43. 11/13 commit-graph: rename 'split_commit_graph_opts'Taylor Blau, Sep 16, 2020
  44. 12/13 builtin/commit-graph.c: introduce '--max-new-filters=<n>'Taylor Blau, Sep 16, 2020
  45. SZEDER GáborSep 18, 2020
  46. Taylor BlauSep 18, 2020
  47. 09/13 bloom/diff: properly short-circuit on max_changesTaylor Blau, Sep 16, 2020
  48. 13/13 commit-graph: introduce 'commitGraph.maxNewFilters'Taylor Blau, Sep 16, 2020
  49. Derrick StoleeSep 16, 2020
  50. Junio C HamanoSep 16, 2020
  51. Taylor BlauSep 17, 2020
  52. Junio C HamanoSep 17, 2020
  53. Taylor BlauSep 17, 2020
  54. Taylor BlauSep 17, 2020
  55. Derrick StoleeSep 17, 2020
  56. 00/13 more miscellaneous Bloom filter improvements, reduxTaylor Blau, Sep 18, 2020
  57. 01/13 commit-graph: introduce 'get_bloom_filter_settings()'Taylor Blau, Sep 18, 2020
  58. 03/13 commit-graph: pass a 'struct repository *' in more placesTaylor Blau, Sep 18, 2020
  59. 02/13 t4216: use an '&&'-chainTaylor Blau, Sep 18, 2020
  60. 05/13 commit-graph: respect 'commitGraph.readChangedPaths'Taylor Blau, Sep 18, 2020
  61. 04/13 t/helper/test-read-graph.c: prepare repo settingsTaylor Blau, Sep 18, 2020
  62. 08/13 bloom: use provided 'struct bloom_filter_settings'Taylor Blau, Sep 18, 2020
  63. SZEDER GáborSep 18, 2020
  64. Taylor BlauSep 18, 2020
  65. 07/13 bloom: split 'get_bloom_filter()' in twoTaylor Blau, Sep 18, 2020
  66. 06/13 commit-graph.c: store maximum changed pathsTaylor Blau, Sep 18, 2020
  67. 10/13 bloom: encode out-of-bounds filters as non-emptyTaylor Blau, Sep 18, 2020
  68. 11/13 commit-graph: rename 'split_commit_graph_opts'Taylor Blau, Sep 18, 2020
  69. 13/13 commit-graph: introduce 'commitGraph.maxNewFilters'Taylor Blau, Sep 18, 2020
  70. Taylor BlauSep 18, 2020
  71. Junio C HamanoSep 18, 2020
  72. 12/13 builtin/commit-graph.c: introduce '--max-new-filters=<n>'Taylor Blau, Sep 18, 2020
  73. 09/13 bloom/diff: properly short-circuit on max_changesTaylor Blau, Sep 18, 2020
  74. Taylor BlauSep 18, 2020
  75. Taylor BlauSep 18, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.