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

Re: [PATCH v2 00/11] More commit-graph/Bloom filter improvements

From
Derrick Stolee <stolee@gmail.com>
Date
Jun 25, 2020, 13:38 UTC
Message-ID
<4b08360d-04ab-b614-196b-76124f1d54ef@gmail.com>
In-Reply-To
<xmqq7dvw5699.fsf@gitster.c.googlers.com>
On 6/24/2020 8:38 PM, Junio C Hamano wrote:
Show 43 quoted lines
> Derrick Stolee <stolee@gmail.com> writes:
> 
>> On 6/24/2020 7:11 PM, Junio C Hamano wrote:
>>> This does not seem to play well with what is in flight.  Tests seem
>>> to pass with topics up to es/config-hooks merged but not with this
>>> topic merged on top.
>>>
>>>     1b5d3d8260 Merge branch 'ds/commit-graph-bloom-updates' into seen
>>>     32169c595c Merge branch 'es/config-hooks' into seen
>>>     ...
>>>
>>> $ sh t4216-log-bloom.sh -i -v
>>>
>>> ends like so:
>>>
>>> ok 133 - Use Bloom filters if they exist in the latest but not all commit graphs in the chain.
>>>
>>> expecting success of 4216.134 'persist filter settings':
>>>         test_when_finished rm -rf .git/objects/info/commit-graph* &&
>>>         GIT_TRACE2_EVENT="$(pwd)/trace2.txt" git commit-graph write --reachable --changed-paths &&
>>>         grep "{\"hash_version\":1,\"num_hashes\":7,\"bits_per_entry\":10}" trace2.txt &&
>>>         cp .git/objects/info/commit-graph commit-graph-before &&
>>>         corrupt_graph $BASE_K_BYTE_OFFSET "\09" &&
>>>         corrupt_graph $BASE_LEN_BYTE_OFFSET "\0F" &&
>>>         cp .git/objects/info/commit-graph commit-graph-after &&
>>>         test_commit c18 A/corrupt &&
>>>         GIT_TRACE2_EVENT="$(pwd)/trace2.txt" git commit-graph write --reachable --changed-paths &&
>>>         grep "{\"hash_version\":1,\"num_hashes\":57,\"bits_per_entry\":70}" trace2.txt
>>>
>>> not ok 134 - persist filter settings
>>> # ...
>>>
>>> Thanks.
>>
>> Thanks for letting me know. I'll investigate carefully with the
>> rest of the 'seen' branch. This test is a bit fragile due to
>> computed values for which bytes to replace, so anything that
>> could have changed the length or order of chunks would lead to
>> a failure here.
>>
>> Sorry for the disruption.
> 
> Oh, not at all.  Thanks for helping

I'll squash the patch into my v3, but here it is now to make 'seen' pass tests again.

The _real_ reason for the failure was that some changes in trace2 pushed the events out of the nesting limits. I also think it is a good idea to make the test less brittle. Adding GIT_TEST_* variables will also help anyone who wants to adjust the Bloom filter settings for testing.

Question: Should these be GIT_BLOOM_SETTINGS_* instead of GIT_TEST_...?
I ask because this _could_ be a way to allow user customization,
without making it as public as a config option. Or, should I just do
the work and add config settings in this series?

Thanks, -Stolee

-- >8 --
From 9245d31f0431eceec60f0b7a90900d2825787530 Mon Sep 17 00:00:00 2001
From: Derrick Stolee <dstolee@microsoft.com>
Date: Thu, 25 Jun 2020 09:19:13 -0400
Subject: [PATCH] fixup! commit-graph: persist existence of changed-paths

The previous version of this test was too fragile to subtle changes in the commit-graph file size. This version now uses two environment variables to customize the Bloom filter settings before rewriting without those environment variables. This demonstrates that we persist the settings correctly.

The issue with the 'seen' branch is due to es/trace-log-progress adding trace2 regions in the progress indicators. This pushed the trace2 data that the test was expecting outside the nesting limit. Set GIT_TRACE2_EVENT_NESTING to ensure we still record those items.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 commit-graph.c       |  9 +++++++--
 t/t4216-log-bloom.sh | 34 +++++++++++-----------------------
 2 files changed, 18 insertions(+), 25 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 1de6800d74..026ec63d38 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1576,10 +1576,15 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 	int num_chunks = 3;
 	uint64_t chunk_offset;
 	struct object_id file_hash;
-	const struct bloom_filter_settings bloom_settings = DEFAULT_BLOOM_FILTER_SETTINGS;
+	struct bloom_filter_settings bloom_settings = DEFAULT_BLOOM_FILTER_SETTINGS;
 
-	if (!ctx->bloom_settings)
+	if (!ctx->bloom_settings) {
+		bloom_settings.bits_per_entry = git_env_ulong("GIT_TEST_BLOOM_SETTINGS_BITS_PER_ENTRY",
+							      bloom_settings.bits_per_entry);
+		bloom_settings.num_hashes = git_env_ulong("GIT_TEST_BLOOM_SETTINGS_NUM_HASHES",
+							  bloom_settings.num_hashes);
 		ctx->bloom_settings = &bloom_settings;
+	}
 
 	if (ctx->split) {
 		struct strbuf tmp_file = STRBUF_INIT;
diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh
index 84f95972ca..d7dd717347 100755
--- a/t/t4216-log-bloom.sh
+++ b/t/t4216-log-bloom.sh
@@ -156,31 +156,19 @@ test_expect_success 'Use Bloom filters if they exist in the latest but not all c
 	test_bloom_filters_used_when_some_filters_are_missing "-- A/B"
 '
 
-BASE_BDAT_OFFSET=2240
-BASE_K_BYTE_OFFSET=$((BASE_BDAT_OFFSET + 10))
-BASE_LEN_BYTE_OFFSET=$((BASE_BDAT_OFFSET + 14))
-
-corrupt_graph() {
-	pos=$1
-	data="${2:-\0}"
-	grepstr=$3
-	orig_size=$(wc -c < .git/objects/info/commit-graph) &&
-	zero_pos=${4:-${orig_size}} &&
-	printf "$data" | dd of=".git/objects/info/commit-graph" bs=1 seek="$pos" conv=notrunc &&
-	dd of=".git/objects/info/commit-graph" bs=1 seek="$zero_pos" if=/dev/null
-}
-
 test_expect_success 'persist filter settings' '
 	test_when_finished rm -rf .git/objects/info/commit-graph* &&
-	GIT_TRACE2_EVENT="$(pwd)/trace2.txt" git commit-graph write --reachable --changed-paths &&
-	grep "{\"hash_version\":1,\"num_hashes\":7,\"bits_per_entry\":10}" trace2.txt &&
-	cp .git/objects/info/commit-graph commit-graph-before &&
-	corrupt_graph $BASE_K_BYTE_OFFSET "\09" &&
-	corrupt_graph $BASE_LEN_BYTE_OFFSET "\0F" &&
-	cp .git/objects/info/commit-graph commit-graph-after &&
-	test_commit c18 A/corrupt &&
-	GIT_TRACE2_EVENT="$(pwd)/trace2.txt" git commit-graph write --reachable --changed-paths &&
-	grep "{\"hash_version\":1,\"num_hashes\":57,\"bits_per_entry\":70}" trace2.txt
+	rm -rf .git/objects/info/commit-graph* &&
+	GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
+		GIT_TRACE2_EVENT_NESTING=5 \
+		GIT_TEST_BLOOM_SETTINGS_NUM_HASHES=9 \
+		GIT_TEST_BLOOM_SETTINGS_BITS_PER_ENTRY=15 \
+		git commit-graph write --reachable --changed-paths &&
+	grep "{\"hash_version\":1,\"num_hashes\":9,\"bits_per_entry\":15}" trace2.txt &&
+	GIT_TRACE2_EVENT="$(pwd)/trace2-auto.txt" \
+		GIT_TRACE2_EVENT_NESTING=5 \
+		git commit-graph write --reachable --changed-paths &&
+	grep "{\"hash_version\":1,\"num_hashes\":9,\"bits_per_entry\":15}" trace2-auto.txt
 '
 
 test_done
\ No newline at end of file

-- 
2.27.0
Previous: Junio C HamanoNext: Junio C Hamano
Message 46 of 76 in “More commit-graph/Bloom filter improvements”
  1. 0/8 More commit-graph/Bloom filter improvementsDerrick Stolee via GitGitGadget, Jun 15, 2020
  2. 2/8 commit-graph: unify the signatures of all write_graph_chunk_*() functionsSZEDER Gábor via GitGitGadget, Jun 15, 2020
  3. René ScharfeJun 18, 2020
  4. 1/8 commit-graph: place bloom_settings in contextDerrick Stolee via GitGitGadget, Jun 15, 2020
  5. René ScharfeJun 18, 2020
  6. Derrick StoleeJun 19, 2020
  7. 3/8 commit-graph: simplify chunk writes into loopSZEDER Gábor via GitGitGadget, Jun 15, 2020
  8. René ScharfeJun 18, 2020
  9. 6/8 bloom: enforce a minimum size of 8 bytesDerrick Stolee via GitGitGadget, Jun 15, 2020
  10. 7/8 commit-graph: change test to die on parse, not loadDerrick Stolee via GitGitGadget, Jun 15, 2020
  11. 8/8 commit-graph: persist existence of changed-pathsDerrick Stolee via GitGitGadget, Jun 15, 2020
  12. 5/8 commit-graph: check all leading directories in changed path Bloom filtersSZEDER Gábor via GitGitGadget, Jun 15, 2020
  13. René ScharfeJun 18, 2020
  14. René ScharfeJun 19, 2020
  15. Taylor BlauJun 19, 2020
  16. Taylor BlauJun 19, 2020
  17. Derrick StoleeJun 23, 2020
  18. 4/8 commit-graph: check chunk sizes after writingSZEDER Gábor via GitGitGadget, Jun 15, 2020
  19. Junio C HamanoJun 17, 2020
  20. Derrick StoleeJun 18, 2020
  21. 00/11 More commit-graph/Bloom filter improvementsDerrick Stolee via GitGitGadget, Jun 23, 2020
  22. 01/11 commit-graph: place bloom_settings in contextDerrick Stolee via GitGitGadget, Jun 23, 2020
  23. 03/11 bloom: get_bloom_filter() cleanupsDerrick Stolee via GitGitGadget, Jun 23, 2020
  24. René ScharfeJun 25, 2020
  25. 02/11 commit-graph: change test to die on parse, not loadDerrick Stolee via GitGitGadget, Jun 23, 2020
  26. 04/11 commit-graph: persist existence of changed-pathsDerrick Stolee via GitGitGadget, Jun 23, 2020
  27. 05/11 commit-graph: unify the signatures of all write_graph_chunk_*() functionsSZEDER Gábor via GitGitGadget, Jun 23, 2020
  28. René ScharfeJun 25, 2020
  29. 06/11 commit-graph: simplify chunk writes into loopSZEDER Gábor via GitGitGadget, Jun 23, 2020
  30. René ScharfeJun 25, 2020
  31. Derrick StoleeJun 25, 2020
  32. 08/11 revision.c: fix whitespaceDerrick Stolee via GitGitGadget, Jun 23, 2020
  33. 09/11 revision: empty pathspecs should not use Bloom filtersTaylor Blau via GitGitGadget, Jun 23, 2020
  34. 10/11 commit-graph: check all leading directories in changed path Bloom filtersSZEDER Gábor via GitGitGadget, Jun 23, 2020
  35. René ScharfeJun 25, 2020
  36. Derrick StoleeJun 25, 2020
  37. SZEDER GáborJun 26, 2020
  38. Derrick StoleeJun 26, 2020
  39. 11/11 bloom: enforce a minimum size of 8 bytesDerrick Stolee via GitGitGadget, Jun 23, 2020
  40. 07/11 commit-graph: check chunk sizes after writingSZEDER Gábor via GitGitGadget, Jun 23, 2020
  41. René ScharfeJun 25, 2020
  42. Derrick StoleeJun 25, 2020
  43. Junio C HamanoJun 24, 2020
  44. Derrick StoleeJun 24, 2020
  45. Junio C HamanoJun 25, 2020
  46. Derrick StoleeJun 25, 2020
  47. Junio C HamanoJun 25, 2020
  48. 00/10 More commit-graph/Bloom filter improvementsDerrick Stolee via GitGitGadget, Jun 26, 2020
  49. 02/10 commit-graph: change test to die on parse, not loadDerrick Stolee via GitGitGadget, Jun 26, 2020
  50. 01/10 commit-graph: place bloom_settings in contextDerrick Stolee via GitGitGadget, Jun 26, 2020
  51. 03/10 bloom: fix logic in get_bloom_filter()Derrick Stolee via GitGitGadget, Jun 26, 2020
  52. SZEDER GáborJun 27, 2020
  53. Derrick StoleeJun 29, 2020
  54. 04/10 commit-graph: persist existence of changed-pathsDerrick Stolee via GitGitGadget, Jun 26, 2020
  55. 05/10 commit-graph: unify the signatures of all write_graph_chunk_*() functionsSZEDER Gábor via GitGitGadget, Jun 26, 2020
  56. 06/10 commit-graph: simplify chunk writes into loopSZEDER Gábor via GitGitGadget, Jun 26, 2020
  57. 07/10 commit-graph: check chunk sizes after writingSZEDER Gábor via GitGitGadget, Jun 26, 2020
  58. 10/10 commit-graph: check all leading directories in changed path Bloom filtersSZEDER Gábor via GitGitGadget, Jun 26, 2020
  59. 09/10 revision: empty pathspecs should not use Bloom filtersTaylor Blau via GitGitGadget, Jun 26, 2020
  60. 08/10 revision.c: fix whitespaceDerrick Stolee via GitGitGadget, Jun 26, 2020
  61. 00/10 More commit-graph/Bloom filter improvementsDerrick Stolee via GitGitGadget, Jul 1, 2020
  62. 01/10 commit-graph: place bloom_settings in contextDerrick Stolee via GitGitGadget, Jul 1, 2020
  63. 02/10 commit-graph: change test to die on parse, not loadDerrick Stolee via GitGitGadget, Jul 1, 2020
  64. 03/10 bloom: fix logic in get_bloom_filter()Derrick Stolee via GitGitGadget, Jul 1, 2020
  65. 05/10 commit-graph: unify the signatures of all write_graph_chunk_*() functionsSZEDER Gábor via GitGitGadget, Jul 1, 2020
  66. 06/10 commit-graph: simplify chunk writes into loopSZEDER Gábor via GitGitGadget, Jul 1, 2020
  67. 07/10 commit-graph: check chunk sizes after writingSZEDER Gábor via GitGitGadget, Jul 1, 2020
  68. 10/10 commit-graph: check all leading directories in changed path Bloom filtersSZEDER Gábor via GitGitGadget, Jul 1, 2020
  69. 09/10 revision: empty pathspecs should not use Bloom filtersTaylor Blau via GitGitGadget, Jul 1, 2020
  70. 04/10 commit-graph: persist existence of changed-pathsDerrick Stolee via GitGitGadget, Jul 1, 2020
  71. SZEDER GáborOct 15, 2020
  72. Taylor BlauOct 15, 2020
  73. Derrick StoleeOct 16, 2020
  74. Taylor BlauOct 16, 2020
  75. Derrick StoleeOct 16, 2020
  76. 08/10 revision.c: fix whitespaceDerrick Stolee via GitGitGadget, Jul 1, 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.