Integrate changed-path Bloom filters with 'git blame' #609

derrickstolee · 2020-04-10T21:35:06Z

If the changed-path Bloom filters are relatively stable, then I propose trying to build upon them as a way to discover any deficiencies. Also, it's good to use them when we can.

The goal I set out to achieve was to use Bloom filters as much as possible in git blame. I think I have achieved most of that, except I did not integrate it with the -C mode. In that case, the blob-diff computation takes a majority of the computation time, so short-circuiting the tree diff using Bloom filters. Also, it's just really complicated. If someone else thinks there is an easy win there, then please go ahead and give it a try with the extra logic presented here in PATCH 3.

While I was playing around with Bloom filters and git blame, I purposefully got it working with some scenarios but not all. Then I tried to trigger a failing build in the blame tests using GIT_TEST_COMMIT_GRAPH=1 and GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=1. But the tests all succeeded!

Examining the data, I see that the commit-graph didn't have the Bloom filter chunks at all. This is because we are not setting the flag to write them in the right spot. The GIT_TEST_COMMIT_GRAPH=1 variable triggers a commit-graph write during git commit, so we need to update the code there instead of just inspecting the variable in git commit-graph write. (This is PATCH 2.)

By updating this variable, I saw some test failures in other tests regarding non-exact pathspecs. I fixed these in PATCH 1 so we keep clean builds.

I based this change on [1] but it would apply cleanly (and logically) on gs/commit-graph-path-filter

Updates in v2:

Added PATCH 3 to write commit-graph files during 'git merge' if GIT_TEST_COMMIT_GRAPH is enabled.
Updated PATCH 1 with the simplification recommended by Taylor.
Fixed the lower-case "bloom" in the commit message.

Thanks,
-Stolee

[1] https://lore.kernel.org/git/pull.601.v2.git.1586437211842.gitgitgadget@gmail.com/

Cc: me@ttaylorr.com, jnareb@gmail.com, garimasigit@gmail.com

derrickstolee · 2020-04-11T01:02:10Z

/submit

gitgitgadget · 2020-04-11T01:03:14Z

Submitted as pull.609.git.1586566981.gitgitgadget@gmail.com

gitgitgadget · 2020-04-11T21:44:41Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> The goal I set out to achieve was to use Bloom filters as much as possible
> in git blame.

Exciting.  Uncomfortably exciting.

Thanks.

gitgitgadget · 2020-04-11T21:44:42Z

revision.c

@@ -661,6 +661,15 @@ static void prepare_to_use_bloom_filter(struct rev_info *revs)
 	if (!revs->commits)


On the Git mailing list, Junio C Hamano wrote (reply to this):

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Derrick Stolee <dstolee@microsoft.com> > > The changed-path Bloom filters work only when we can compute an > explicit Bloom filter key in advance. When a pathspec is given > that allows case-insensitive checks or wildcard matching, we > must disable the Bloom filter performance checks. How often do we want to do case-insensitive path matching, I wonder. As somebody who never touches case-insensitive filesystem, I would be a bad judge, and I would imagine that I would be looking for a pathspec "[Mm]akefile" rather than ":(icase)makefile" myself if there are projects that may have either/both, so I do not mind disabling the bloom filter for case insensitivity myself. But if users may use icase pathspec very often, it may be worth considering to build the bloom filter after downcasing the paths, perhaps? Given that many projects extract their source code to a case insensitive filesystem, I would imagine that downcasing paths would map two originally different paths into the same thing only rarely, if ever, so there may not be much downside to do so. > By checking the pathspec in prepare_to_use_bloom_filters(), we > avoid setting up the Bloom filter data and thus revert to the > usual logic. > > Before this change, the following tests would fail*: > > t6004-rev-list-path-optim.sh (Tests 6-7) > t6130-pathspec-noglob.sh (Tests 3-6) > t6131-pathspec-icase.sh (Tests 3-5) > > *These tests would fail when using GIT_TEST_COMMIT_GRAPH and > GIT_TEST_COMMIT_GRAPH_BLOOM_FILTERS except that the latter > environment variable was not set up correctly to write the changed- > path Bloom filters in the test suite. That will be fixed in the > next change. Thanks. This is exciting.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 4/11/2020 5:40 PM, Junio C Hamano wrote: > "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: Derrick Stolee <dstolee@microsoft.com> >> >> The changed-path Bloom filters work only when we can compute an >> explicit Bloom filter key in advance. When a pathspec is given >> that allows case-insensitive checks or wildcard matching, we >> must disable the Bloom filter performance checks. > > How often do we want to do case-insensitive path matching, I wonder. > > As somebody who never touches case-insensitive filesystem, I would > be a bad judge, and I would imagine that I would be looking for a > pathspec "[Mm]akefile" rather than ":(icase)makefile" myself if > there are projects that may have either/both, so I do not mind > disabling the bloom filter for case insensitivity myself. > > But if users may use icase pathspec very often, it may be worth > considering to build the bloom filter after downcasing the paths, > perhaps? Given that many projects extract their source code to a > case insensitive filesystem, I would imagine that downcasing paths > would map two originally different paths into the same thing only > rarely, if ever, so there may not be much downside to do so. This behavior could be extended later, and carefully. My initial thought was that the case check would happen on every commit. If the :(icase) check only happens at the walk tip(s), then we could compute a single Bloom key at the start. Thanks, -Stolee

On the Git mailing list, Junio C Hamano wrote (reply to this):

Derrick Stolee <stolee@gmail.com> writes: >> But if users may use icase pathspec very often, it may be worth >> considering to build the bloom filter after downcasing the paths, >> perhaps? Given that many projects extract their source code to a >> case insensitive filesystem, I would imagine that downcasing paths >> would map two originally different paths into the same thing only >> rarely, if ever, so there may not be much downside to do so. > > This behavior could be extended later, and carefully. My initial > thought was that the case check would happen on every commit. If > the :(icase) check only happens at the walk tip(s), then we could > compute a single Bloom key at the start. Sorry, I am not sure what you mean. Do you mean that we notice that the user wants to match 'foo' case insensitively, and tell the logic that uses changed-path records in the graph file that commits that cannot possibly have touched any or the paths 'foo', 'foO', 'fOo', ... (all 8 case permutations) are not interesting? I guess that would work, but I was wondering if it is simpler without much downside if the changed-path records in the graph file are prepared on paths after they are normalized to a single case. That would lose information (e.g. you no longer can say "commits that touch the path 'foo' is interesting, but those that touch the path 'Foo' are not"), but makes the side that queries much simpler (i.e. you do not have to prepare all 8 case permutations---you only ask about 'foo'). And because the Bloom filter is used only for performance to cull commits that can never possibly match, allowing a false positive that would be discarded by actually running tree-diff anyway, the only potential downside happens when the project has too many paths that are different only in cases by increased collisions and by reducing our chances to skip running tree-diff (and never affects correctness). But this is not the "could be extended later" kind of behaviour, I am afraid. It is baked in the data stored in the graph file. It all depends on how often people want :(icase) pathspec matches in the history, I suspect. My point was that we need to declare that :(icase) won't matter in real life (hence we won't optimize our data to support that use case), before the way in which the data stored in the graph file is computed is cast in stone. Thanks.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 4/14/2020 2:25 PM, Junio C Hamano wrote: > Derrick Stolee <stolee@gmail.com> writes: > >>> But if users may use icase pathspec very often, it may be worth >>> considering to build the bloom filter after downcasing the paths, >>> perhaps? Given that many projects extract their source code to a >>> case insensitive filesystem, I would imagine that downcasing paths >>> would map two originally different paths into the same thing only >>> rarely, if ever, so there may not be much downside to do so. >> >> This behavior could be extended later, and carefully. My initial >> thought was that the case check would happen on every commit. If >> the :(icase) check only happens at the walk tip(s), then we could >> compute a single Bloom key at the start. > > Sorry, I am not sure what you mean. That's my fault. There are a couple of things I misunderstood here. 1. Thinking about "git blame" we would need to "collapse" a pathspec to a specific file before starting history. But blame doesn't allow ":(icase)" anyway. 2. With that context of "git blame" in my head, I was thinking (incorrectly) that "git log" would collapse the pathspec based on what file(s) match the pattern at HEAD. The tests in t6131-pathspec-icase.sh clearly show that this is wrong. In fact, if we apply the following diff to this patch, then we can get failures with the changed-path filters: diff --git a/revision.c b/revision.c index f78c636e4d..a02be25feb 100644 --- a/revision.c +++ b/revision.c @@ -652,13 +652,14 @@ static void trace2_bloom_filter_statistics_atexit(void) static int forbid_bloom_filters(struct pathspec *spec) { + int allowed_flags = PATHSPEC_LITERAL | PATHSPEC_ICASE; if (spec->has_wildcard) return 1; if (spec->nr > 1) return 1; - if (spec->magic & ~PATHSPEC_LITERAL) + if (spec->magic & ~allowed_flags) return 1; - if (spec->nr && (spec->items[0].magic & ~PATHSPEC_LITERAL)) + if (spec->nr && (spec->items[0].magic & ~allowed_flags)) return 1; return 0; > Do you mean that we notice that the user wants to match 'foo' case > insensitively, and tell the logic that uses changed-path records in > the graph file that commits that cannot possibly have touched any or > the paths 'foo', 'foO', 'fOo', ... (all 8 case permutations) are not > interesting? > > I guess that would work, but I was wondering if it is simpler > without much downside if the changed-path records in the graph file > are prepared on paths after they are normalized to a single case. > That would lose information (e.g. you no longer can say "commits > that touch the path 'foo' is interesting, but those that touch the > path 'Foo' are not"), but makes the side that queries much simpler > (i.e. you do not have to prepare all 8 case permutations---you only > ask about 'foo'). > > And because the Bloom filter is used only for performance to cull > commits that can never possibly match, allowing a false positive > that would be discarded by actually running tree-diff anyway, the > only potential downside happens when the project has too many paths > that are different only in cases by increased collisions and by > reducing our chances to skip running tree-diff (and never affects > correctness). > > But this is not the "could be extended later" kind of behaviour, I > am afraid. It is baked in the data stored in the graph file. Since the feature is not released, we still have time to update the format if we so desired. With the current format, we would need to disable the filters when using an :(icase) pathspec as the current patch does. I'm not against the idea. Logically, collapsing case before hashing the Bloom keys should not increase the probabilities of false positives except in the situations where we have case conflicts. There is a small cost in the pre-hashing step to change the case of the paths, but that should be much lower than the cost of the hash itself and the tree parsing to find the changed paths. > It all depends on how often people want :(icase) pathspec matches in > the history, I suspect. My point was that we need to declare that > :(icase) won't matter in real life (hence we won't optimize our data > to support that use case), before the way in which the data stored > in the graph file is computed is cast in stone. My earlier statement can be summarized as "we could make this happen" and you ask here "is it worth doing?" I will play around with how complicated the change would be while the community considers the "is it worth doing?" question. Thanks, -Stolee

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 4/15/2020 9:27 AM, Derrick Stolee wrote: > > I'm not against the idea. Logically, collapsing case before hashing > the Bloom keys should not increase the probabilities of false > positives except in the situations where we have case conflicts. > There is a small cost in the pre-hashing step to change the case of > the paths, but that should be much lower than the cost of the hash > itself and the tree parsing to find the changed paths. > >> It all depends on how often people want :(icase) pathspec matches in >> the history, I suspect. My point was that we need to declare that >> :(icase) won't matter in real life (hence we won't optimize our data >> to support that use case), before the way in which the data stored >> in the graph file is computed is cast in stone. > > My earlier statement can be summarized as "we could make this happen" > and you ask here "is it worth doing?" > > I will play around with how complicated the change would be while > the community considers the "is it worth doing?" question. I played a bit, and it wasn't terrible to cover everything by adjusting fill_bloom_key(). I also added a test that we will want anyways. There is more work to be done if this is the direction we want to go, including double-checking that this doesn't cause significant performance degradation. The point of me sending this patch is to make the proposed direction very clear how it would work. I'm still not sure how I feel about it. Thanks, -Stolee -->8-- From 89beb9598daabb19e3c896bbceeb0fc1b9ccc6ca Mon Sep 17 00:00:00 2001 From: Derrick Stolee <dstolee@microsoft.com> Date: Wed, 15 Apr 2020 18:04:25 +0000 Subject: [PATCH] bloom: compute all Bloom hashes from lowercase The changed-path Bloom filters currently hash path strings using the exact string for the path. This makes it difficult* to use the filters when restricting to case-insensitive pathspecs. * I say "difficult" because it is possible to generate all 2^n options for the case of a path and test them all, but this is a bad idea and should not be done. "Impossible" is an appropriate alternative. THIS IS A BREAKING CHANGE. Commit-graph files with changed-path Bloom filters computed by a previous commit will not be compatible with the filters computed in this commit, nor will we get correct results when testing across these incompatible versions. Normally, this would be a completely unacceptable change, but the filters have not been released and hence are still possible to update before release. TODO: If we decide to move in this direction, then the following steps should be done (and some of them should be done anyway): * We need to document the Bloom filter format to specify exactly how we compute the filter data. The details should be careful enough that someone can reproduce the exact file format without looking at the C code. * That document would include the tolower() transformation that is being done here. Signed-off-by: Derrick Stolee <dstolee@microsoft.com> --- bloom.c | 16 ++++++++++++++-- revision.c | 5 +++-- t/t6131-pathspec-icase.sh | 18 ++++++++++++++++++ 3 files changed, 35 insertions(+), 4 deletions(-) diff --git a/bloom.c b/bloom.c index dd9bab9bbd..c919c60f12 100644 --- a/bloom.c +++ b/bloom.c @@ -130,8 +130,20 @@ void fill_bloom_key(const char *data, int i; const uint32_t seed0 = 0x293ae76f; const uint32_t seed1 = 0x7e646e2c; - const uint32_t hash0 = murmur3_seeded(seed0, data, len); - const uint32_t hash1 = murmur3_seeded(seed1, data, len); + uint32_t hash0, hash1; + static struct strbuf icase_data = STRBUF_INIT; + char *cur; + + strbuf_setlen(&icase_data, 0); + strbuf_addstr(&icase_data, data); + + for (cur = icase_data.buf; cur && *cur; cur++) { + if (isupper(*cur)) + *cur = tolower(*cur); + } + + hash0 = murmur3_seeded(seed0, icase_data.buf, len); + hash1 = murmur3_seeded(seed1, icase_data.buf, len); key->hashes = (uint32_t *)xcalloc(settings->num_hashes, sizeof(uint32_t)); for (i = 0; i < settings->num_hashes; i++) diff --git a/revision.c b/revision.c index f78c636e4d..a02be25feb 100644 --- a/revision.c +++ b/revision.c @@ -652,13 +652,14 @@ static void trace2_bloom_filter_statistics_atexit(void) static int forbid_bloom_filters(struct pathspec *spec) { + int allowed_flags = PATHSPEC_LITERAL | PATHSPEC_ICASE; if (spec->has_wildcard) return 1; if (spec->nr > 1) return 1; - if (spec->magic & ~PATHSPEC_LITERAL) + if (spec->magic & ~allowed_flags) return 1; - if (spec->nr && (spec->items[0].magic & ~PATHSPEC_LITERAL)) + if (spec->nr && (spec->items[0].magic & ~allowed_flags)) return 1; return 0; diff --git a/t/t6131-pathspec-icase.sh b/t/t6131-pathspec-icase.sh index 39fc3f6769..ee0766bd91 100755 --- a/t/t6131-pathspec-icase.sh +++ b/t/t6131-pathspec-icase.sh @@ -106,4 +106,22 @@ test_expect_success '"git diff" can take magic :(icase) pathspec' ' test_cmp expect actual ' +test_expect_success '"git log" takes the magic :(icase) pathspec' ' + cat >expect <<-\EOF && + FOO/BAR + FOO/bAr + FOO/bar + fOo/BAR + fOo/bAr + fOo/bar + foo/BAR + foo/bAr + foo/bar + EOF + git log --pretty=%s HEAD -- ":(icase)foo/bar" >actual && + test_cmp expect actual && + git log --pretty=%s HEAD -- ":(icase)foo" >actual && + test_cmp expect actual +' + test_done

On the Git mailing list, Taylor Blau wrote (reply to this):

On Thu, Apr 16, 2020 at 12:18:33AM +0200, Jakub Narębski wrote: > On Wed, 15 Apr 2020 at 20:37, Derrick Stolee <stolee@gmail.com> wrote: > [...] > > -->8-- > > From 89beb9598daabb19e3c896bbceeb0fc1b9ccc6ca Mon Sep 17 00:00:00 2001 > > From: Derrick Stolee <dstolee@microsoft.com> > > Date: Wed, 15 Apr 2020 18:04:25 +0000 > > Subject: [PATCH] bloom: compute all Bloom hashes from lowercase > > > > The changed-path Bloom filters currently hash path strings using > > the exact string for the path. This makes it difficult* to use the > > filters when restricting to case-insensitive pathspecs. > > > > * I say "difficult" because it is possible to generate all 2^n > > options for the case of a path and test them all, but this is > > a bad idea and should not be done. "Impossible" is an appropriate > > alternative. > > > > THIS IS A BREAKING CHANGE. Commit-graph files with changed-path > > Bloom filters computed by a previous commit will not be compatible > > with the filters computed in this commit, nor will we get correct > > results when testing across these incompatible versions. Normally, > > this would be a completely unacceptable change, but the filters > > have not been released and hence are still possible to update > > before release. > > > > TODO: If we decide to move in this direction, then the following > > steps should be done (and some of them should be done anyway): > > > > * We need to document the Bloom filter format to specify exactly > > how we compute the filter data. The details should be careful > > enough that someone can reproduce the exact file format without > > looking at the C code. > > > > * That document would include the tolower() transformation that is > > being done here. > > Why not modify the BDAT chunk to include version of > case folding transformation or other collation algorithm > (other transformation).that is done prior to computing > the Bloom filter key? Though that might be unnecessary > flexibility... If this ends up being something that we want to do, I agree with Stolee's reasoning that this should be a breaking change. If we were, say, several months into having Bloom filters in a release and decided at that point to make the change, then: sure, supporting both by writing a bit in the BDAT chunk makes sense. But, we're many months away from that state yet, and so I don't think the cost of rebuilding what few commit-graphs exist with bloom filters in them today to support both ordinary and lower-cased paths in the filter. Anyway, I'm still not sold on this idea in general (nor do I understand it that others are), so I'll respond in more detail in another part of the thread... > For example the value of 0x00 in such field of BDAT > chunk header would mean no transformation, while > the value of 0x01 would mean per-character tolower() > or Unicode equivalent of it. > > Best, > -- > Jakub Narębski Thanks, Taylor

On the Git mailing list, Taylor Blau wrote (reply to this):

On Wed, Apr 15, 2020 at 02:25:48PM -0700, Junio C Hamano wrote: > Derrick Stolee <stolee@gmail.com> writes: > > > THIS IS A BREAKING CHANGE. Commit-graph files with changed-path > > Bloom filters computed by a previous commit will not be compatible > > with the filters computed in this commit, nor will we get correct > > results when testing across these incompatible versions. Normally, > > this would be a completely unacceptable change, but the filters > > have not been released and hence are still possible to update > > before release. > > Sure, it hasn't even hit 'next' yet. > > But I think we are both sort-of leaning towards concluding that it > does not help all that much. So I think it is OK. Yeah, I think that the only thing that it is potentially helping is the :(icase) magic. In a repository that doesn't have colliding paths in case-insensitive filesystems (i.e., having both 'foo' and 'FOO' in the same tree), this doesn't seem to be obviously hurting anything. But, it is at least semi-harmful to repositories that do have such trees, since we now can't answer "probably yes" or "definitely not" for colliding paths unless both produce the same fingerprint. That seems like a clear downside. Now, how common is that against people who would benefit from changed-path Bloom filters in their commit-graphs? I don't know one way or the other. But, the upside seems to be minimal compared to the potential cost, so I think that it may be better to just leave this one alone. > > TODO: If we decide to move in this direction, then the following > > steps should be done (and some of them should be done anyway): > > Even if we decide not to do this "downcase before hashing" thing, we > should document how exactly we compute, I think. > > And if we decide do change our mind later, it is not the end of the > world. We should be able to use a different chunk type to store the > filters computed over downcased paths. > > > * We need to document the Bloom filter format to specify exactly > > how we compute the filter data. The details should be careful > > enough that someone can reproduce the exact file format without > > looking at the C code. > > > > * That document would include the tolower() transformation that is > > being done here. > > As the tree-diff comparison done internally while running "git > blame" does not take an end-user specified pathspec in any > meaningful way, this does not matter in practice, but there is > another possible complication we would want to consider when we > extend the support to "git log" and friends---negative pathspecs > (e.g. "git log ':(exclude)COPYING'"). A commit that cannot possibly > have touched the COPYING file would be eligible for output without > actually running tree-diff between it and its parent (as long as the > trees of the two commits are different, we know it must be shown). > > Thanks. Thanks, Taylor

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 4/15/2020 8:52 PM, Taylor Blau wrote: > On Thu, Apr 16, 2020 at 12:18:33AM +0200, Jakub Narębski wrote: >> On Wed, 15 Apr 2020 at 20:37, Derrick Stolee <stolee@gmail.com> wrote: >> [...] >>> -->8-- >>> From 89beb9598daabb19e3c896bbceeb0fc1b9ccc6ca Mon Sep 17 00:00:00 2001 >>> From: Derrick Stolee <dstolee@microsoft.com> >>> Date: Wed, 15 Apr 2020 18:04:25 +0000 >>> Subject: [PATCH] bloom: compute all Bloom hashes from lowercase >>> >>> The changed-path Bloom filters currently hash path strings using >>> the exact string for the path. This makes it difficult* to use the >>> filters when restricting to case-insensitive pathspecs. >>> >>> * I say "difficult" because it is possible to generate all 2^n >>> options for the case of a path and test them all, but this is >>> a bad idea and should not be done. "Impossible" is an appropriate >>> alternative. >>> >>> THIS IS A BREAKING CHANGE. Commit-graph files with changed-path >>> Bloom filters computed by a previous commit will not be compatible >>> with the filters computed in this commit, nor will we get correct >>> results when testing across these incompatible versions. Normally, >>> this would be a completely unacceptable change, but the filters >>> have not been released and hence are still possible to update >>> before release. >>> >>> TODO: If we decide to move in this direction, then the following >>> steps should be done (and some of them should be done anyway): >>> >>> * We need to document the Bloom filter format to specify exactly >>> how we compute the filter data. The details should be careful >>> enough that someone can reproduce the exact file format without >>> looking at the C code. >>> >>> * That document would include the tolower() transformation that is >>> being done here. >> >> Why not modify the BDAT chunk to include version of >> case folding transformation or other collation algorithm >> (other transformation).that is done prior to computing >> the Bloom filter key? Though that might be unnecessary >> flexibility... > > If this ends up being something that we want to do, I agree with > Stolee's reasoning that this should be a breaking change. If we were, > say, several months into having Bloom filters in a release and decided > at that point to make the change, then: sure, supporting both by writing > a bit in the BDAT chunk makes sense. > > But, we're many months away from that state yet, and so I don't think > the cost of rebuilding what few commit-graphs exist with bloom filters > in them today to support both ordinary and lower-cased paths in the > filter. > > Anyway, I'm still not sold on this idea in general (nor do I understand > it that others are), so I'll respond in more detail in another part of > the thread... I agree that this is not a good direction to go. I created the patch because I was curious how difficult it would be, and it is good to have a record of the possible direction. However, it complicates the file format and will have unpredictable effects on the entropy (or on the performance of history for case-colliding paths). It is good that we have the capability to extend the filter data in the future if we really need to. I'll make a TODO item for myself to try writing that detailed Bloom filter format documentation as a follow-up. In the meantime, I'll try to close this out by responding to the feedback we have so far. Thanks, -Stolee

On the Git mailing list, Taylor Blau wrote (reply to this):

On Thu, Apr 16, 2020 at 09:26:49AM -0400, Derrick Stolee wrote: > On 4/15/2020 8:52 PM, Taylor Blau wrote: > > On Thu, Apr 16, 2020 at 12:18:33AM +0200, Jakub Narębski wrote: > >> On Wed, 15 Apr 2020 at 20:37, Derrick Stolee <stolee@gmail.com> wrote: > >> [...] > >>> -->8-- > >>> From 89beb9598daabb19e3c896bbceeb0fc1b9ccc6ca Mon Sep 17 00:00:00 2001 > >>> From: Derrick Stolee <dstolee@microsoft.com> > >>> Date: Wed, 15 Apr 2020 18:04:25 +0000 > >>> Subject: [PATCH] bloom: compute all Bloom hashes from lowercase > >>> > >>> The changed-path Bloom filters currently hash path strings using > >>> the exact string for the path. This makes it difficult* to use the > >>> filters when restricting to case-insensitive pathspecs. > >>> > >>> * I say "difficult" because it is possible to generate all 2^n > >>> options for the case of a path and test them all, but this is > >>> a bad idea and should not be done. "Impossible" is an appropriate > >>> alternative. > >>> > >>> THIS IS A BREAKING CHANGE. Commit-graph files with changed-path > >>> Bloom filters computed by a previous commit will not be compatible > >>> with the filters computed in this commit, nor will we get correct > >>> results when testing across these incompatible versions. Normally, > >>> this would be a completely unacceptable change, but the filters > >>> have not been released and hence are still possible to update > >>> before release. > >>> > >>> TODO: If we decide to move in this direction, then the following > >>> steps should be done (and some of them should be done anyway): > >>> > >>> * We need to document the Bloom filter format to specify exactly > >>> how we compute the filter data. The details should be careful > >>> enough that someone can reproduce the exact file format without > >>> looking at the C code. > >>> > >>> * That document would include the tolower() transformation that is > >>> being done here. > >> > >> Why not modify the BDAT chunk to include version of > >> case folding transformation or other collation algorithm > >> (other transformation).that is done prior to computing > >> the Bloom filter key? Though that might be unnecessary > >> flexibility... > > > > If this ends up being something that we want to do, I agree with > > Stolee's reasoning that this should be a breaking change. If we were, > > say, several months into having Bloom filters in a release and decided > > at that point to make the change, then: sure, supporting both by writing > > a bit in the BDAT chunk makes sense. > > > > But, we're many months away from that state yet, and so I don't think > > the cost of rebuilding what few commit-graphs exist with bloom filters > > in them today to support both ordinary and lower-cased paths in the > > filter. > > > > Anyway, I'm still not sold on this idea in general (nor do I understand > > it that others are), so I'll respond in more detail in another part of > > the thread... > > I agree that this is not a good direction to go. I created the patch > because I was curious how difficult it would be, and it is good to have > a record of the possible direction. However, it complicates the file > format and will have unpredictable effects on the entropy (or on the > performance of history for case-colliding paths). > > It is good that we have the capability to extend the filter data in > the future if we really need to. > > I'll make a TODO item for myself to try writing that detailed Bloom > filter format documentation as a follow-up. In the meantime, I'll try > to close this out by responding to the feedback we have so far. Sounds good, and thanks for investigating. > Thanks, > -Stolee Thanks, Taylor

On the Git mailing list, Junio C Hamano wrote (reply to this):

Taylor Blau <me@ttaylorr.com> writes: >> It is good that we have the capability to extend the filter data in >> the future if we really need to. >> >> I'll make a TODO item for myself to try writing that detailed Bloom >> filter format documentation as a follow-up. In the meantime, I'll try >> to close this out by responding to the feedback we have so far. > > Sounds good, and thanks for investigating. Yeah, thanks, all.

gitgitgadget · 2020-04-11T21:59:39Z

builtin/commit.c

@@ -1701,7 +1701,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		      "not exceeded, and then \"git restore --staged :/\" to recover."));


On the Git mailing list, Junio C Hamano wrote (reply to this):

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > diff --git a/builtin/commit.c b/builtin/commit.c > index d3e7781e658..d2fdfdc4363 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -1701,7 +1701,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix) > "not exceeded, and then \"git restore --staged :/\" to recover.")); > > if (git_env_bool(GIT_TEST_COMMIT_GRAPH, 0) && > - write_commit_graph_reachable(the_repository->objects->odb, 0, NULL)) > + write_commit_graph_reachable(the_repository->objects->odb, > + git_env_bool(GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS, 0) ? COMMIT_GRAPH_WRITE_BLOOM_FILTERS : 0, > + NULL)) Yuck. That is doubly mouthful. I wonder how much value we are getting by having this call here in the first place. This function being cmd_commit(), by definition we won't be updating the graph file unless the test script does "git commit". We won't update the graph file upon "git rebase", "git merge", "git pull", "git fetch", etc., so it is not like with this hack, the test coverage for various traversal using the graph file would magically be perfect. We'd still need an explicit call to "commit-graph write" at strategic places in the test scripts, no? How are we testing with and without bitmaps, and do we have a kludge like this one for the feature, or do we use a different mechanism to help writing tests easier to gain better coverage? In any case, I can see why we want a separate knob specific to the CHANGED_PATHS until the path filter part becomes as stable as the rest of the graph file, but after some time, we should remove that knob, and at that point, we always use the path filter whenever we use commit-graph, so that we won't have to suffer from the ugliness. Wait. I wonder if we can tweak the arrangement a bit so that this layer does not need to know any more than GIT_TEST_COMMIT_GRAPH? For example, in commit-graph.c::write_commit_graph(), can we test the CHANGED_PATHS environment variable and flip the .changed_paths bit, no matter what the 'flags' argument to the function says? Thanks. > return 1; > > repo_rerere(the_repository, 0);

On the Git mailing list, Taylor Blau wrote (reply to this):

On Sat, Apr 11, 2020 at 02:57:18PM -0700, Junio C Hamano wrote: > "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > diff --git a/builtin/commit.c b/builtin/commit.c > > index d3e7781e658..d2fdfdc4363 100644 > > --- a/builtin/commit.c > > +++ b/builtin/commit.c > > @@ -1701,7 +1701,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix) > > "not exceeded, and then \"git restore --staged :/\" to recover.")); > > > > if (git_env_bool(GIT_TEST_COMMIT_GRAPH, 0) && > > - write_commit_graph_reachable(the_repository->objects->odb, 0, NULL)) > > + write_commit_graph_reachable(the_repository->objects->odb, > > + git_env_bool(GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS, 0) ? COMMIT_GRAPH_WRITE_BLOOM_FILTERS : 0, > > + NULL)) > > Yuck. That is doubly mouthful. Yeah, this is quite a mouthful indeed. I think the most conservative fix would be something like: if (git_env_bool(GIT_TEST_COMMIT_GRAPH, 0)) { enum commit_graph_write_flags flags = 0; if (git_env_bool(GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS, 0)) flags |= COMMIT_GRAPH_WRITE_BLOOM_FILTERS; if (write_commit_graph_reachable(the_repository->objects->odb, flags, NULL)) { /* ... */ } } and then ripping this knob out (as Junio suggests below, which I think is a wise suggestion) out would be straightforward. > I wonder how much value we are getting by having this call here in > the first place. This function being cmd_commit(), by definition we > won't be updating the graph file unless the test script does "git > commit". We won't update the graph file upon "git rebase", "git > merge", "git pull", "git fetch", etc., so it is not like with this > hack, the test coverage for various traversal using the graph file > would magically be perfect. We'd still need an explicit call to > "commit-graph write" at strategic places in the test scripts, no? Yeah. It's definitely not a silver bullet for the reasons you mention, but I think that it is helping out in some common cases. For example, if we moved this check out to 'test_commit', then we'd be relying on tests to use that instead of 'git commit'. On the other hand, this catches either, since they both wind up in this builtin. I guess you could push this check down much further to when we're about to write a new object, and--if that new object is a commit--update the commit-graph. That sounds awfully slow (and feels to me like a hack, but I can't justify whether it's more or less of a hack than we already have), but I think it would be OK, since this isn't the normal way to run tests. > How are we testing with and without bitmaps, and do we have a kludge > like this one for the feature, or do we use a different mechanism > to help writing tests easier to gain better coverage? As far as I know after a brief search, we have no similar such mechanism for bitmaps, so commit-graphs are a unique instance. > In any case, I can see why we want a separate knob specific to the > CHANGED_PATHS until the path filter part becomes as stable as the > rest of the graph file, but after some time, we should remove that > knob, and at that point, we always use the path filter whenever we > use commit-graph, so that we won't have to suffer from the ugliness. > > Wait. I wonder if we can tweak the arrangement a bit so that this > layer does not need to know any more than GIT_TEST_COMMIT_GRAPH? > > For example, in commit-graph.c::write_commit_graph(), can we test > the CHANGED_PATHS environment variable and flip the .changed_paths > bit, no matter what the 'flags' argument to the function says? It may be cleaner to have 'GIT_TEST_COMMIT_GRAPH' specify the flags that it wants as its value, but the additional coupling makes me somewhat uncomfortable. > Thanks. > > > return 1; > > > > repo_rerere(the_repository, 0); Thanks, Taylor

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 4/12/2020 4:51 PM, Taylor Blau wrote: > On Sat, Apr 11, 2020 at 02:57:18PM -0700, Junio C Hamano wrote: >> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: >> >>> diff --git a/builtin/commit.c b/builtin/commit.c >>> index d3e7781e658..d2fdfdc4363 100644 >>> --- a/builtin/commit.c >>> +++ b/builtin/commit.c >>> @@ -1701,7 +1701,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix) >>> "not exceeded, and then \"git restore --staged :/\" to recover.")); >>> >>> if (git_env_bool(GIT_TEST_COMMIT_GRAPH, 0) && >>> - write_commit_graph_reachable(the_repository->objects->odb, 0, NULL)) >>> + write_commit_graph_reachable(the_repository->objects->odb, >>> + git_env_bool(GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS, 0) ? COMMIT_GRAPH_WRITE_BLOOM_FILTERS : 0, >>> + NULL)) >> >> Yuck. That is doubly mouthful. > > Yeah, this is quite a mouthful indeed. I think the most conservative fix > would be something like: > > if (git_env_bool(GIT_TEST_COMMIT_GRAPH, 0)) { > enum commit_graph_write_flags flags = 0; > if (git_env_bool(GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS, 0)) > flags |= COMMIT_GRAPH_WRITE_BLOOM_FILTERS; > > if (write_commit_graph_reachable(the_repository->objects->odb, > flags, NULL)) { > /* ... */ > } > } > > and then ripping this knob out (as Junio suggests below, which I think > is a wise suggestion) out would be straightforward. > >> I wonder how much value we are getting by having this call here in >> the first place. This function being cmd_commit(), by definition we >> won't be updating the graph file unless the test script does "git >> commit". We won't update the graph file upon "git rebase", "git >> merge", "git pull", "git fetch", etc., so it is not like with this >> hack, the test coverage for various traversal using the graph file >> would magically be perfect. We'd still need an explicit call to >> "commit-graph write" at strategic places in the test scripts, no? > > Yeah. It's definitely not a silver bullet for the reasons you mention, > but I think that it is helping out in some common cases. For example, > if we moved this check out to 'test_commit', then we'd be relying on > tests to use that instead of 'git commit'. On the other hand, this > catches either, since they both wind up in this builtin. > > I guess you could push this check down much further to when we're about > to write a new object, and--if that new object is a commit--update the > commit-graph. That sounds awfully slow (and feels to me like a hack, but > I can't justify whether it's more or less of a hack than we already > have), but I think it would be OK, since this isn't the normal way to > run tests. If we keep this simple, or extract the process to a "write_commit_graph_for_tests()" macro inside builtin.h, then we could insert a commit-graph write in more places. However, I think there is value in testing the "not every commit is included in the commit-graph" case, which our current setup does quite nicely. The one exception is that 'git merge' could benefit from this snippet, so we cap off the commit-graph whenever a test script "constructs" a repository to match a data shape. >> How are we testing with and without bitmaps, and do we have a kludge >> like this one for the feature, or do we use a different mechanism >> to help writing tests easier to gain better coverage? > > As far as I know after a brief search, we have no similar such mechanism > for bitmaps, so commit-graphs are a unique instance. I would not be against a similar mechanism for bitmaps, but this is really important for commit-graphs. The difference lies in which users have commit-graphs versus bitmaps. Now that commit-graphs are on by default and written by 'git gc', client users get commit-graphs unless they opt-out. We need to test as many scenarios as possible to avoid causing them disruption. Who uses bitmaps? Primarily Git servers. The scenarios run on those repos are much more restricted, and those scenarios are tested with bitmaps explicitly. >> In any case, I can see why we want a separate knob specific to the >> CHANGED_PATHS until the path filter part becomes as stable as the >> rest of the graph file, but after some time, we should remove that >> knob, and at that point, we always use the path filter whenever we >> use commit-graph, so that we won't have to suffer from the ugliness. >> >> Wait. I wonder if we can tweak the arrangement a bit so that this >> layer does not need to know any more than GIT_TEST_COMMIT_GRAPH? >> >> For example, in commit-graph.c::write_commit_graph(), can we test >> the CHANGED_PATHS environment variable and flip the .changed_paths >> bit, no matter what the 'flags' argument to the function says? > > It may be cleaner to have 'GIT_TEST_COMMIT_GRAPH' specify the flags that > it wants as its value, but the additional coupling makes me somewhat > uncomfortable. It would be easy to insert it here: diff --git a/commit-graph.c b/commit-graph.c index 77668629e2..3e8f36ce5c 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -1968,6 +1968,8 @@ int write_commit_graph(struct object_directory *odb, ctx->check_oids = flags & COMMIT_GRAPH_WRITE_CHECK_OIDS ? 1 : 0; ctx->split_opts = split_opts; ctx->changed_paths = flags & COMMIT_GRAPH_WRITE_BLOOM_FILTERS ? 1 : 0; + if (git_env_bool(GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS, 0)) + ctx->changed_paths = 1; ctx->total_bloom_filter_data_size = 0; if (ctx->split) { The problem here is that if GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=1 and someone runs "git commit-graph write --no-changed-paths" then the negation of that option is ignored. But this is a GIT_TEST_* variable, and any test that requires that check could disable the enviroment variable first. Thanks, -Stolee

On the Git mailing list, Junio C Hamano wrote (reply to this):

Derrick Stolee <stolee@gmail.com> writes: > If we keep this simple, or extract the process to a > "write_commit_graph_for_tests()" macro inside builtin.h, then we could > insert a commit-graph write in more places. As long as the check necessary is cheap enough to realize that we are in production mode, we should be able to keep the run-time overhead to the minimum. Sprinkling such a call all over the place, however, might add to the overhead of reading code, though. > However, I think there is value in testing the "not every commit is > included in the commit-graph" case, which our current setup does quite > nicely. Yeah, that is also a good point. > The one exception is that 'git merge' could benefit from this > snippet, so we cap off the commit-graph whenever a test script > "constructs" a repository to match a data shape. Sounds good. > The problem here is that if GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=1 and > someone runs "git commit-graph write --no-changed-paths" then the > negation of that option is ignored. But this is a GIT_TEST_* variable, > and any test that requires that check could disable the enviroment > variable first. Yeah, that sounds good.

gitgitgadget · 2020-04-11T22:05:38Z

blame.c

@@ -9,6 +9,8 @@
 #include "blame.h"


On the Git mailing list, Junio C Hamano wrote (reply to this):

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Derrick Stolee <dstolee@microsoft.com> > Subject: Re: [PATCH 3/3] blame: use changed-path Bloom filters Ahh, I almost forgot we spell Bloom with capital B, so I should go back and amend the title of [2/3]. > When computing a blame, there is a section in find_origin() that > computes a diff between a commit and one of its parents. When this > is the first parent, we can check the Bloom filters before calling > diff_tree_oid(). > > In order to make this work with the blame machinery, we need to > initialize a struct bloom_key with the initial path. But also, we > need to add more keys to a list if a rename is detected. We then > check to see if _any_ of these keys answer "maybe" in the diff. Yes. I think after gaining experience with this technique, we may be able to speed up the "git log --follow" while correcting its semantics at the same time. The prospect is unnervingly exciting. > Generally, this is a performance enhancement and should not > change the behavior of 'git blame' in any way. Absolutely. > The lack of improvement for the MAINTAINERS file and the relatively > modest improvement for the other examples can be easily explained. > The blame machinery needs to compute line-level diffs to determine > which lines were changed by each commit. That makes up a large > proportion of the computation time, and this change does not > attempt to improve on that section of the algorithm. The > MAINTAINERS file is large and changed often, so it takes time to > determine which lines were updated by which commit. In contrast, > the code files are much smaller, and it takes longer to comute > the line-by-line diff for a single patch on the Linux mailing > lists. Yup, tree-diff for a deeper path would benefit the most, and your numbers were indeed impressive. > Signed-off-by: Derrick Stolee <dstolee@microsoft.com> > --- > blame.c | 139 ++++++++++++++++++++++++++++++++++++++++++++---- > blame.h | 6 +++ > builtin/blame.c | 10 ++++ > 3 files changed, 146 insertions(+), 9 deletions(-) I am kind-a surprised how little additional code it takes to do this. Good job.

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Sat, Apr 11, 2020 at 6:03 PM Junio C Hamano <gitster@pobox.com> wrote: > "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > > The lack of improvement for the MAINTAINERS file and the relatively > > modest improvement for the other examples can be easily explained. > > The blame machinery needs to compute line-level diffs to determine > > which lines were changed by each commit. That makes up a large > > proportion of the computation time, and this change does not > > attempt to improve on that section of the algorithm. The > > MAINTAINERS file is large and changed often, so it takes time to > > determine which lines were updated by which commit. In contrast, > > the code files are much smaller, and it takes longer to comute > > the line-by-line diff for a single patch on the Linux mailing > > lists. s/comute/compute/ (Sorry for replying to Junio's reply -- the original is no longer in my mailbox.)

gitgitgadget · 2020-04-12T20:51:27Z

This branch is now known as ds/blame-on-bloom.

gitgitgadget · 2020-04-12T20:51:28Z

This patch series was integrated into pu via git@9dd05ca.

gitgitgadget · 2020-04-12T22:24:04Z

revision.c

@@ -661,6 +661,15 @@ static void prepare_to_use_bloom_filter(struct rev_info *revs)
 	if (!revs->commits)


On the Git mailing list, Taylor Blau wrote (reply to this):

Hi Stolee, On Sat, Apr 11, 2020 at 01:02:59AM +0000, Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee <dstolee@microsoft.com> > > The changed-path Bloom filters work only when we can compute an > explicit Bloom filter key in advance. When a pathspec is given > that allows case-insensitive checks or wildcard matching, we > must disable the Bloom filter performance checks. > > By checking the pathspec in prepare_to_use_bloom_filters(), we > avoid setting up the Bloom filter data and thus revert to the > usual logic. All makes sense to me, and this seems like the only reasonable thing *to* do in this situation. That's fine, since we're not regressing, we're just not using Bloom filters. > Before this change, the following tests would fail*: > > t6004-rev-list-path-optim.sh (Tests 6-7) > t6130-pathspec-noglob.sh (Tests 3-6) > t6131-pathspec-icase.sh (Tests 3-5) > > *These tests would fail when using GIT_TEST_COMMIT_GRAPH and > GIT_TEST_COMMIT_GRAPH_BLOOM_FILTERS except that the latter > environment variable was not set up correctly to write the changed- > path Bloom filters in the test suite. That will be fixed in the > next change. Nicely done. > Signed-off-by: Derrick Stolee <dstolee@microsoft.com> > --- > revision.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/revision.c b/revision.c > index 2b06ee739c8..e37b5b06108 100644 > --- a/revision.c > +++ b/revision.c > @@ -661,6 +661,15 @@ static void prepare_to_use_bloom_filter(struct rev_info *revs) > if (!revs->commits) > return; > I certainly wouldn't complain about a comment here explaining these three checks, but I suppose that the rationale is only a 'git blame' away (and I guess that is faster now after this series ;-)). > + if (revs->prune_data.has_wildcard) > + return; > + if (revs->prune_data.nr > 1) > + return; > + if (revs->prune_data.magic || > + (revs->prune_data.nr && > + revs->prune_data.items[0].magic)) > + return; > + > repo_parse_commit(revs->repo, revs->commits->item); > > if (!revs->repo->objects->commit_graph) > -- > gitgitgadget Reviewed-by: Taylor Blau <me@ttaylorr.com> Thanks, Taylor

On the Git mailing list, Junio C Hamano wrote (reply to this):

Taylor Blau <me@ttaylorr.com> writes: > I certainly wouldn't complain about a comment here explaining these > three checks, but I suppose that the rationale is only a 'git blame' > away (and I guess that is faster now after this series ;-)). > >> + if (revs->prune_data.has_wildcard) >> + return; >> + if (revs->prune_data.nr > 1) >> + return; >> + if (revs->prune_data.magic || >> + (revs->prune_data.nr && >> + revs->prune_data.items[0].magic)) This says "any magic", but it is overly pessimistic to disable the optimization for "literal" magic. That magic is the one that lets well written scripts to say "I have in a '$variable' that the user gave me as a pathname (not pathspec), and it may have a wildcard letter in it, but please treat it as a literal string" by prefixing ":(literal)" before that user-supplied data, so it is punishing well disciplined folks. >> + return; >> + >> repo_parse_commit(revs->repo, revs->commits->item); >> >> if (!revs->repo->objects->commit_graph) >> -- >> gitgitgadget > > Reviewed-by: Taylor Blau <me@ttaylorr.com> > > Thanks, > Taylor

On the Git mailing list, Taylor Blau wrote (reply to this):

On Sun, Apr 12, 2020 at 03:30:07PM -0700, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > > > I certainly wouldn't complain about a comment here explaining these > > three checks, but I suppose that the rationale is only a 'git blame' > > away (and I guess that is faster now after this series ;-)). > > > >> + if (revs->prune_data.has_wildcard) > >> + return; > >> + if (revs->prune_data.nr > 1) > >> + return; > >> + if (revs->prune_data.magic || > >> + (revs->prune_data.nr && > >> + revs->prune_data.items[0].magic)) > > This says "any magic", but it is overly pessimistic to disable the > optimization for "literal" magic. That magic is the one that lets > well written scripts to say "I have in a '$variable' that the user > gave me as a pathname (not pathspec), and it may have a wildcard > letter in it, but please treat it as a literal string" by prefixing > ":(literal)" before that user-supplied data, so it is punishing well > disciplined folks. I hadn't thought of that, but it makes sense to me. How about something like this squashed into this patch? I moved the if-chain that Stolee introduced out to its own function, at least since they seem well-contained and related to one another. I figure that this simplifies the implementation of 'prepare_to_use_bloom_filter' by giving the reader less to think about up-front. diff --git a/revision.c b/revision.c index 534c0bf996..15bf4ccff5 100644 --- a/revision.c +++ b/revision.c @@ -654,6 +654,18 @@ static void trace2_bloom_filter_statistics_atexit(void) jw_release(&jw); } +static int has_bloom_key(struct pathspec *spec) +{ + if (spec->has_wildcard) + return 0; + if (spec->nr > 1) + return 0; + if ((spec->magic & ~PATHSPEC_LITERAL) || + (spec->nr && spec->items[0].magic & ~PATHSPEC_LITERAL)) + return 0; + return 1; +} + static void prepare_to_use_bloom_filter(struct rev_info *revs) { struct pathspec_item *pi; @@ -665,13 +677,7 @@ static void prepare_to_use_bloom_filter(struct rev_info *revs) if (!revs->commits) return; - if (revs->prune_data.has_wildcard) - return; - if (revs->prune_data.nr > 1) - return; - if (revs->prune_data.magic || - (revs->prune_data.nr && - revs->prune_data.items[0].magic)) + if (!has_bloom_key(&revs->prune_data)) return; repo_parse_commit(revs->repo, revs->commits->item); > >> + return; > >> + > >> repo_parse_commit(revs->repo, revs->commits->item); > >> > >> if (!revs->repo->objects->commit_graph) > >> -- > >> gitgitgadget > > > > Reviewed-by: Taylor Blau <me@ttaylorr.com> > > > > Thanks, > > Taylor Thanks, Taylor

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 4/12/2020 8:07 PM, Taylor Blau wrote: > On Sun, Apr 12, 2020 at 03:30:07PM -0700, Junio C Hamano wrote: >> Taylor Blau <me@ttaylorr.com> writes: >> >>> I certainly wouldn't complain about a comment here explaining these >>> three checks, but I suppose that the rationale is only a 'git blame' >>> away (and I guess that is faster now after this series ;-)). >>> >>>> + if (revs->prune_data.has_wildcard) >>>> + return; >>>> + if (revs->prune_data.nr > 1) >>>> + return; >>>> + if (revs->prune_data.magic || >>>> + (revs->prune_data.nr && >>>> + revs->prune_data.items[0].magic)) >> >> This says "any magic", but it is overly pessimistic to disable the >> optimization for "literal" magic. That magic is the one that lets >> well written scripts to say "I have in a '$variable' that the user >> gave me as a pathname (not pathspec), and it may have a wildcard >> letter in it, but please treat it as a literal string" by prefixing >> ":(literal)" before that user-supplied data, so it is punishing well >> disciplined folks. This is a good point. I'm unfamiliar with these advanced pathspec tricks. > I hadn't thought of that, but it makes sense to me. How about something > like this squashed into this patch? I moved the if-chain that Stolee > introduced out to its own function, at least since they seem > well-contained and related to one another. I figure that this simplifies > the implementation of 'prepare_to_use_bloom_filter' by giving the reader > less to think about up-front. > > diff --git a/revision.c b/revision.c > index 534c0bf996..15bf4ccff5 100644 > --- a/revision.c > +++ b/revision.c > @@ -654,6 +654,18 @@ static void trace2_bloom_filter_statistics_atexit(void) > jw_release(&jw); > } > > +static int has_bloom_key(struct pathspec *spec) > +{ > + if (spec->has_wildcard) > + return 0; > + if (spec->nr > 1) > + return 0; > + if ((spec->magic & ~PATHSPEC_LITERAL) || > + (spec->nr && spec->items[0].magic & ~PATHSPEC_LITERAL)) > + return 0; > + return 1; > +} > + Perhaps flip this on its head? +static int forbids_bloom_key(struct pathspec *spec) +{ + if (spec->has_wildcard) + return 1; + if (spec->nr > 1) + return 1; + if (spec->magic & ~PATHSPEC_LITERAL) + return 1; + if (spec->nr && (spec->items[0].magic & ~PATHSPEC_LITERAL)) + return 1; + return 0; +} + > static void prepare_to_use_bloom_filter(struct rev_info *revs) > { > struct pathspec_item *pi; > @@ -665,13 +677,7 @@ static void prepare_to_use_bloom_filter(struct rev_info *revs) > if (!revs->commits) > return; > > - if (revs->prune_data.has_wildcard) > - return; > - if (revs->prune_data.nr > 1) > - return; > - if (revs->prune_data.magic || > - (revs->prune_data.nr && > - revs->prune_data.items[0].magic)) > + if (!has_bloom_key(&revs->prune_data)) > return; Then this would be "if (forbids_bloom_key(&revs->prune_data))" Generally, I like pulling this stuff out as a method to isolate and label its purpose. If we wanted to allow certain :(icase) things later, then we know what to modify in order to "allow" it. Thanks, -Stolee

The changed-path Bloom filters work only when we can compute an explicit Bloom filter key in advance. When a pathspec is given that allows case-insensitive checks or wildcard matching, we must disable the Bloom filter performance checks. By checking the pathspec in prepare_to_use_bloom_filters(), we avoid setting up the Bloom filter data and thus revert to the usual logic. Before this change, the following tests would fail*: t6004-rev-list-path-optim.sh (Tests 6-7) t6130-pathspec-noglob.sh (Tests 3-6) t6131-pathspec-icase.sh (Tests 3-5) *These tests would fail when using GIT_TEST_COMMIT_GRAPH and GIT_TEST_COMMIT_GRAPH_BLOOM_FILTERS except that the latter environment variable was not set up correctly to write the changed- path Bloom filters in the test suite. That will be fixed in the next change. Helped-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

derrickstolee · 2020-04-13T14:44:29Z

/submit

gitgitgadget · 2020-04-13T14:45:46Z

Submitted as pull.609.v2.git.1586789126.gitgitgadget@gmail.com

gitgitgadget · 2020-04-13T16:12:48Z

revision.c

@@ -650,6 +650,20 @@ static void trace2_bloom_filter_statistics_atexit(void)
 	jw_release(&jw);


On the Git mailing list, Taylor Blau wrote (reply to this):

On Mon, Apr 13, 2020 at 02:45:23PM +0000, Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee <dstolee@microsoft.com> > > The changed-path Bloom filters work only when we can compute an > explicit Bloom filter key in advance. When a pathspec is given > that allows case-insensitive checks or wildcard matching, we > must disable the Bloom filter performance checks. > > By checking the pathspec in prepare_to_use_bloom_filters(), we > avoid setting up the Bloom filter data and thus revert to the > usual logic. > > Before this change, the following tests would fail*: > > t6004-rev-list-path-optim.sh (Tests 6-7) > t6130-pathspec-noglob.sh (Tests 3-6) > t6131-pathspec-icase.sh (Tests 3-5) > > *These tests would fail when using GIT_TEST_COMMIT_GRAPH and > GIT_TEST_COMMIT_GRAPH_BLOOM_FILTERS except that the latter > environment variable was not set up correctly to write the changed- > path Bloom filters in the test suite. That will be fixed in the > next change. > > Helped-by: Taylor Blau <me@ttaylorr.com> > Signed-off-by: Derrick Stolee <dstolee@microsoft.com> > --- > revision.c | 19 ++++++++++++++++++- > 1 file changed, 18 insertions(+), 1 deletion(-) > > diff --git a/revision.c b/revision.c > index 2b06ee739c8..f78c636e4d0 100644 > --- a/revision.c > +++ b/revision.c > @@ -650,6 +650,20 @@ static void trace2_bloom_filter_statistics_atexit(void) > jw_release(&jw); > } > > +static int forbid_bloom_filters(struct pathspec *spec) > +{ > + if (spec->has_wildcard) > + return 1; > + if (spec->nr > 1) > + return 1; > + if (spec->magic & ~PATHSPEC_LITERAL) > + return 1; > + if (spec->nr && (spec->items[0].magic & ~PATHSPEC_LITERAL)) > + return 1; > + > + return 0; > +} > + > static void prepare_to_use_bloom_filter(struct rev_info *revs) > { > struct pathspec_item *pi; > @@ -659,7 +673,10 @@ static void prepare_to_use_bloom_filter(struct rev_info *revs) > int len; > > if (!revs->commits) > - return; > + return; > + > + if (forbid_bloom_filters(&revs->prune_data)) > + return; > > repo_parse_commit(revs->repo, revs->commits->item); > > -- > gitgitgadget > Nicely done, this looks good to me. Thanks. Thanks, Taylor

On the Git mailing list, Junio C Hamano wrote (reply to this):

Taylor Blau <me@ttaylorr.com> writes: >> +static int forbid_bloom_filters(struct pathspec *spec) >> +{ >> + if (spec->has_wildcard) >> + return 1; >> + if (spec->nr > 1) >> + return 1; >> + if (spec->magic & ~PATHSPEC_LITERAL) >> + return 1; >> + if (spec->nr && (spec->items[0].magic & ~PATHSPEC_LITERAL)) >> + return 1; >> + >> + return 0; >> +} >> + >> static void prepare_to_use_bloom_filter(struct rev_info *revs) >> { >> struct pathspec_item *pi; >> @@ -659,7 +673,10 @@ static void prepare_to_use_bloom_filter(struct rev_info *revs) >> int len; >> >> if (!revs->commits) >> - return; >> + return; >> + >> + if (forbid_bloom_filters(&revs->prune_data)) >> + return; >> >> repo_parse_commit(revs->repo, revs->commits->item); >> >> -- >> gitgitgadget >> > > Nicely done, this looks good to me. Thanks. Likewise. Very exciting. Will queue.

gitgitgadget · 2020-04-13T16:15:50Z

commit-graph.c

@@ -1968,6 +1968,8 @@ int write_commit_graph(struct object_directory *odb,
 	ctx->check_oids = flags & COMMIT_GRAPH_WRITE_CHECK_OIDS ? 1 : 0;


On the Git mailing list, Taylor Blau wrote (reply to this):

On Mon, Apr 13, 2020 at 02:45:24PM +0000, Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee <dstolee@microsoft.com> > > The GIT_TEST_COMMIT_GRAPH environment variable updates the commit- > graph file whenever "git commit" is run, ensuring that we always > have an updated commit-graph throughout the test suite. The > GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS environment variable was > introduced to write the changed-path Bloom filters whenever "git > commit-graph write" is run. However, the GIT_TEST_COMMIT_GRAPH > trick doesn't launch a separate process and instead writes it > directly. > > Update the "git commit" builtin to write changed-path Bloom filters > when both GIT_TEST_COMMIT_GRAPH and GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS > are enabled. > > Signed-off-by: Derrick Stolee <dstolee@microsoft.com> > --- > commit-graph.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/commit-graph.c b/commit-graph.c > index 77668629e27..3e8f36ce5c8 100644 > --- a/commit-graph.c > +++ b/commit-graph.c > @@ -1968,6 +1968,8 @@ int write_commit_graph(struct object_directory *odb, > ctx->check_oids = flags & COMMIT_GRAPH_WRITE_CHECK_OIDS ? 1 : 0; > ctx->split_opts = split_opts; > ctx->changed_paths = flags & COMMIT_GRAPH_WRITE_BLOOM_FILTERS ? 1 : 0; > + if (git_env_bool(GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS, 0)) > + ctx->changed_paths = 1; Hmm. I'm not crazy about a library function looking at 'GIT_TEST_*' environment variables and potentially ignoring its arguments, but given the discussion we had in v1, I don't feel strongly enough to recommend that you change this. For what it's worth, I think that 'write_commit_graph' could behave more purely if callers checked 'GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS' and set 'flags' appropriately from the outside, but I can understand also why it's preferred to do it in this style, too. > ctx->total_bloom_filter_data_size = 0; > > if (ctx->split) { > -- > gitgitgadget > Thanks, Taylor

On the Git mailing list, Junio C Hamano wrote (reply to this):

Taylor Blau <me@ttaylorr.com> writes: > Hmm. I'm not crazy about a library function looking at 'GIT_TEST_*' > environment variables and potentially ignoring its arguments, but given > the discussion we had in v1, I don't feel strongly enough to recommend > that you change this. > > For what it's worth, I think that 'write_commit_graph' could behave more > purely if callers checked 'GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS' and set > 'flags' appropriately from the outside,... Yeah, I agree that it would be a lot cleaner if that is easy to arrange. I have a suspicion that Derrick already tried and the resulting code looked messier and was discarded?

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 4/13/2020 6:21 PM, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > >> Hmm. I'm not crazy about a library function looking at 'GIT_TEST_*' >> environment variables and potentially ignoring its arguments, but given >> the discussion we had in v1, I don't feel strongly enough to recommend >> that you change this. >> >> For what it's worth, I think that 'write_commit_graph' could behave more >> purely if callers checked 'GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS' and set >> 'flags' appropriately from the outside,... > > Yeah, I agree that it would be a lot cleaner if that is easy to > arrange. I have a suspicion that Derrick already tried and the > resulting code looked messier and was discarded? Perhaps I could fix both concerns by 1. Use a macro instead of a library call. 2. Check the _CHANGED_PATHS variable in the macro. The macro would look like this: #define git_test_write_commit_graph_or_die() \ if (git_env_bool(GIT_TEST_COMMIT_GRAPH, 0)) { \ int flags = 0; \ if (git_env_bool(GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS, 0)) \ flags = COMMIT_GRAPH_WRITE_BLOOM_FILTERS; \ if (write_commit_graph_reachable( \ the_repository->objects->odb, flags, NULL)) \ die("failed to write commit-graph under GIT_TEST_COMMIT_GRAPH"); \ } Thanks, -Stolee

On the Git mailing list, Junio C Hamano wrote (reply to this):

Derrick Stolee <stolee@gmail.com> writes: > On 4/13/2020 6:21 PM, Junio C Hamano wrote: >> Taylor Blau <me@ttaylorr.com> writes: >> >>> Hmm. I'm not crazy about a library function looking at 'GIT_TEST_*' >>> environment variables and potentially ignoring its arguments, but given >>> the discussion we had in v1, I don't feel strongly enough to recommend >>> that you change this. >>> >>> For what it's worth, I think that 'write_commit_graph' could behave more >>> purely if callers checked 'GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS' and set >>> 'flags' appropriately from the outside,... >> >> Yeah, I agree that it would be a lot cleaner if that is easy to >> arrange. I have a suspicion that Derrick already tried and the >> resulting code looked messier and was discarded? > > Perhaps I could fix both concerns by > > 1. Use a macro instead of a library call. > > 2. Check the _CHANGED_PATHS variable in the macro. I am not sure how use of a macro "fixes" purity, though. And what is the other concern? How widely would this "if we are testing, write out the graph file" call be sprinkled over the codebase? I am hoping that it won't be "everywhere", but only at strategic places (like "just once before we leave a subcommand that creates one or bunch of commits"). And how often would they be called? I am also hoping that it won't be "inside a tight loop". In short, I am wondering if we can promise our codebase that - git_test_write_commit_graph_or_die() calls won't be an eyesore (and/or distraction) for developers too much. - git_env_bool() call won't have performance impact. > The macro would look like this: > > > #define git_test_write_commit_graph_or_die() \ > if (git_env_bool(GIT_TEST_COMMIT_GRAPH, 0)) { \ > int flags = 0; \ > if (git_env_bool(GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS, 0)) \ > flags = COMMIT_GRAPH_WRITE_BLOOM_FILTERS; \ > if (write_commit_graph_reachable( \ > the_repository->objects->odb, flags, NULL)) \ > die("failed to write commit-graph under GIT_TEST_COMMIT_GRAPH"); \ > } > > Thanks, > -Stolee

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 4/14/2020 1:26 PM, Junio C Hamano wrote: > Derrick Stolee <stolee@gmail.com> writes: > >> On 4/13/2020 6:21 PM, Junio C Hamano wrote: >>> Taylor Blau <me@ttaylorr.com> writes: >>> >>>> Hmm. I'm not crazy about a library function looking at 'GIT_TEST_*' >>>> environment variables and potentially ignoring its arguments, but given >>>> the discussion we had in v1, I don't feel strongly enough to recommend >>>> that you change this. >>>> >>>> For what it's worth, I think that 'write_commit_graph' could behave more >>>> purely if callers checked 'GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS' and set >>>> 'flags' appropriately from the outside,... >>> >>> Yeah, I agree that it would be a lot cleaner if that is easy to >>> arrange. I have a suspicion that Derrick already tried and the >>> resulting code looked messier and was discarded? >> >> Perhaps I could fix both concerns by >> >> 1. Use a macro instead of a library call. >> >> 2. Check the _CHANGED_PATHS variable in the macro. > > I am not sure how use of a macro "fixes" purity, though. And what > is the other concern? The concern was (1) checking the environment and (2) die()ing in the library. > How widely would this "if we are testing, write out the graph file" > call be sprinkled over the codebase? I am hoping that it won't be > "everywhere", but only at strategic places (like "just once before > we leave a subcommand that creates one or bunch of commits"). And > how often would they be called? I am also hoping that it won't be > "inside a tight loop". In short, I am wondering if we can promise > our codebase that > > - git_test_write_commit_graph_or_die() calls won't be an eyesore > (and/or distraction) for developers too much. > > - git_env_bool() call won't have performance impact. I could add a comment to the header file to say "this is only for improving coverage of optional features in the test suite. Do not call this method unless you know what you are doing." My intention right now is to only include this in 'git commit' and 'git merge'. Earlier discussion included thoughts about 'git rebase' and similar, but those are more rarely used when constructing an "example repo" in the test scripts. -Stolee

On the Git mailing list, Taylor Blau wrote (reply to this):

On Tue, Apr 14, 2020 at 01:40:53PM -0400, Derrick Stolee wrote: > On 4/14/2020 1:26 PM, Junio C Hamano wrote: > > Derrick Stolee <stolee@gmail.com> writes: > > > >> On 4/13/2020 6:21 PM, Junio C Hamano wrote: > >>> Taylor Blau <me@ttaylorr.com> writes: > >>> > >>>> Hmm. I'm not crazy about a library function looking at 'GIT_TEST_*' > >>>> environment variables and potentially ignoring its arguments, but given > >>>> the discussion we had in v1, I don't feel strongly enough to recommend > >>>> that you change this. > >>>> > >>>> For what it's worth, I think that 'write_commit_graph' could behave more > >>>> purely if callers checked 'GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS' and set > >>>> 'flags' appropriately from the outside,... > >>> > >>> Yeah, I agree that it would be a lot cleaner if that is easy to > >>> arrange. I have a suspicion that Derrick already tried and the > >>> resulting code looked messier and was discarded? > >> > >> Perhaps I could fix both concerns by > >> > >> 1. Use a macro instead of a library call. > >> > >> 2. Check the _CHANGED_PATHS variable in the macro. > > > > I am not sure how use of a macro "fixes" purity, though. And what > > is the other concern? > > The concern was (1) checking the environment and (2) die()ing in the > library. I think that we might as well use a plain old function here instead of a macro, especially since the macro is just expanding out to what that function would be if we had written it that way. For what it's worth, my concern was more that I don't mind dying, but I'd rather do so in a calling function, and that by the time we're calling 'write_commit_graph', that we should be using return values instead of 'die()' or 'error()'. > > How widely would this "if we are testing, write out the graph file" > > call be sprinkled over the codebase? I am hoping that it won't be > > "everywhere", but only at strategic places (like "just once before > > we leave a subcommand that creates one or bunch of commits"). And > > how often would they be called? I am also hoping that it won't be > > "inside a tight loop". In short, I am wondering if we can promise > > our codebase that > > > > - git_test_write_commit_graph_or_die() calls won't be an eyesore > > (and/or distraction) for developers too much. > > > > - git_env_bool() call won't have performance impact. > > I could add a comment to the header file to say "this is only for > improving coverage of optional features in the test suite. Do not > call this method unless you know what you are doing." Works for me. > My intention right now is to only include this in 'git commit' > and 'git merge'. Earlier discussion included thoughts about > 'git rebase' and similar, but those are more rarely used when > constructing an "example repo" in the test scripts. Ditto. I think that there's benefit in having it in some places but not all (as Stolee pointed out earlier in the thread), and that trying to have it everywhere is a fool's errand. > -Stolee Thanks, Taylor

gitgitgadget · 2020-04-13T16:24:51Z

On the Git mailing list, Taylor Blau wrote (reply to this):

On Mon, Apr 13, 2020 at 02:45:22PM +0000, Derrick Stolee via GitGitGadget wrote:
> Updates in v2:
>
>  * Added PATCH 3 to write commit-graph files during 'git merge' if
>    GIT_TEST_COMMIT_GRAPH is enabled.
>
>
>  * Updated PATCH 1 with the simplification recommended by Taylor.
>
>
>  * Fixed the lower-case "bloom" in the commit message.

This updated version looks great to me. Thanks for looking at all of my
suggestions :-).

  Reviewed-by: Taylor Blau <me@ttaylorr.com>

Thanks,
Taylor

gitgitgadget · 2020-04-13T23:57:45Z

This patch series was integrated into pu via git@d184cc1.

gitgitgadget · 2020-04-14T18:29:23Z

This patch series was integrated into pu via git@368328a.

gitgitgadget · 2020-04-15T20:04:42Z

This patch series was integrated into pu via git@b2833cb.

gitgitgadget · 2020-04-15T23:23:57Z

This patch series was integrated into pu via git@1ae0009.

The GIT_TEST_COMMIT_GRAPH environment variable updates the commit- graph file whenever "git commit" is run, ensuring that we always have an updated commit-graph throughout the test suite. The GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS environment variable was introduced to write the changed-path Bloom filters whenever "git commit-graph write" is run. However, the GIT_TEST_COMMIT_GRAPH trick doesn't launch a separate process and instead writes it directly. To expand the number of tests that have commits in the commit-graph file, add a helper method that computes the commit-graph and place that helper inside "git commit" and "git merge". In the helper method, check GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS to ensure we are writing changed-path Bloom filters whenever possible. Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

The changed-path Bloom filters help reduce the amount of tree parsing required during history queries. Before calculating a diff, we can ask the filter if a path changed between a commit and its first parent. If the filter says "no" then we can move on without parsing trees. If the filter says "maybe" then we parse trees to discover if the answer is actually "yes" or "no". When computing a blame, there is a section in find_origin() that computes a diff between a commit and one of its parents. When this is the first parent, we can check the Bloom filters before calling diff_tree_oid(). In order to make this work with the blame machinery, we need to initialize a struct bloom_key with the initial path. But also, we need to add more keys to a list if a rename is detected. We then check to see if _any_ of these keys answer "maybe" in the diff. During development, I purposefully left out this "add a new key when a rename is detected" to see if the test suite would catch my error. That is how I discovered the issues with GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS from the previous change. With that change, we can feel some confidence in the coverage of this change. If a user requests copy detection using "git blame -C", then there are more places where the set of "important" files can expand. I do not know enough about how this happens in the blame machinery. Thus, the Bloom filter integration is explicitly disabled in this mode. A later change could expand the bloom_key data with an appropriate call (or calls) to add_bloom_key(). If we did not disable this mode, then the following tests would fail: t8003-blame-corner-cases.sh t8011-blame-split-file.sh Generally, this is a performance enhancement and should not change the behavior of 'git blame' in any way. If a repo has a commit-graph file with computed changed-path Bloom filters, then they should notice improved performance for their 'git blame' commands. Here are some example timings that I found by blaming some paths in the Linux kernel repository: git blame arch/x86/kernel/topology.c >/dev/null Before: 0.83s After: 0.24s git blame kernel/time/time.c >/dev/null Before: 0.72s After: 0.24s git blame tools/perf/ui/stdio/hist.c >/dev/null Before: 0.27s After: 0.11s I specifically looked for "deep" paths that were also edited many times. As a counterpoint, the MAINTAINERS file was edited many times but is located in the root tree. This means that the cost of computing a diff relative to the pathspec is very small. Here are the timings for that command: git blame MAINTAINERS >/dev/null Before: 20.1s After: 18.0s These timings are the best of five. The worst-case runs were on the order of 2.5 minutes for both cases. Note that the MAINTAINERS file has 18,740 lines across 17,000+ commits. This happens to be one of the cases where this change provides the least improvement. The lack of improvement for the MAINTAINERS file and the relatively modest improvement for the other examples can be easily explained. The blame machinery needs to compute line-level diffs to determine which lines were changed by each commit. That makes up a large proportion of the computation time, and this change does not attempt to improve on that section of the algorithm. The MAINTAINERS file is large and changed often, so it takes time to determine which lines were updated by which commit. In contrast, the code files are much smaller, and it takes longer to comute the line-by-line diff for a single patch on the Linux mailing lists. Outside of the "-C" integration, I believe there is little more to gain from the changed-path Bloom filters for 'git blame' after this patch. Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

derrickstolee · 2020-04-16T20:13:07Z

/submit

gitgitgadget · 2020-04-16T20:14:16Z

Submitted as pull.609.v3.git.1587068044.gitgitgadget@gmail.com

gitgitgadget · 2020-04-18T00:05:22Z

This patch series was integrated into pu via git@b885ca7.

gitgitgadget · 2020-04-20T18:35:06Z

This patch series was integrated into pu via git@f59ea28.

gitgitgadget · 2020-04-21T02:22:58Z

This patch series was integrated into pu via git@ad13d6d.

gitgitgadget · 2020-04-24T23:52:21Z

This patch series was integrated into pu via git@f223966.

gitgitgadget · 2020-04-24T23:52:23Z

This patch series was integrated into next via git@dc4f24e.

gitgitgadget · 2020-06-07T20:36:33Z

revision.c

@@ -650,6 +650,20 @@ static void trace2_bloom_filter_statistics_atexit(void)
 	jw_release(&jw);


On the Git mailing list, SZEDER Gábor wrote (reply to this):

On Thu, Apr 16, 2020 at 08:14:02PM +0000, Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee <dstolee@microsoft.com> > > The changed-path Bloom filters work only when we can compute an > explicit Bloom filter key in advance. When a pathspec is given > that allows case-insensitive checks or wildcard matching, we > must disable the Bloom filter performance checks. > > By checking the pathspec in prepare_to_use_bloom_filters(), we > avoid setting up the Bloom filter data and thus revert to the > usual logic. > > Before this change, the following tests would fail*: > > t6004-rev-list-path-optim.sh (Tests 6-7) > t6130-pathspec-noglob.sh (Tests 3-6) > t6131-pathspec-icase.sh (Tests 3-5) > > *These tests would fail when using GIT_TEST_COMMIT_GRAPH and > GIT_TEST_COMMIT_GRAPH_BLOOM_FILTERS except that the latter > environment variable was not set up correctly to write the changed- > path Bloom filters in the test suite. That will be fixed in the > next change. > > Helped-by: Taylor Blau <me@ttaylorr.com> > Signed-off-by: Derrick Stolee <dstolee@microsoft.com> > --- > revision.c | 19 ++++++++++++++++++- > 1 file changed, 18 insertions(+), 1 deletion(-) > > diff --git a/revision.c b/revision.c > index 2b06ee739c8..f78c636e4d0 100644 > --- a/revision.c > +++ b/revision.c > @@ -650,6 +650,20 @@ static void trace2_bloom_filter_statistics_atexit(void) > jw_release(&jw); > } > > +static int forbid_bloom_filters(struct pathspec *spec) > +{ > + if (spec->has_wildcard) > + return 1; > + if (spec->nr > 1) > + return 1; > + if (spec->magic & ~PATHSPEC_LITERAL) Nit: spec->magic is the bitwise OR combination of all spec->items[i].magic, so checking the latter below is unnecessary. > + return 1; > + if (spec->nr && (spec->items[0].magic & ~PATHSPEC_LITERAL)) > + return 1; > + > + return 0; > +} > + > static void prepare_to_use_bloom_filter(struct rev_info *revs) > { > struct pathspec_item *pi; > @@ -659,7 +673,10 @@ static void prepare_to_use_bloom_filter(struct rev_info *revs) > int len; > > if (!revs->commits) > - return; > + return; > + > + if (forbid_bloom_filters(&revs->prune_data)) > + return; > > repo_parse_commit(revs->repo, revs->commits->item); > > -- > gitgitgadget >

derrickstolee force-pushed the bloom-blame branch from 4ceadd5 to 431fde6 Compare April 11, 2020 00:59

gitgitgadget bot reviewed Apr 11, 2020

View reviewed changes

gitgitgadget bot added the pu label Apr 12, 2020

gitgitgadget bot reviewed Apr 12, 2020

View reviewed changes

derrickstolee force-pushed the bloom-blame branch from 431fde6 to 4ae196d Compare April 13, 2020 12:57

gitgitgadget bot reviewed Apr 13, 2020

View reviewed changes

derrickstolee added 2 commits April 16, 2020 13:37

derrickstolee force-pushed the bloom-blame branch from 4ae196d to 463d6bf Compare April 16, 2020 13:39

gitgitgadget bot added the next label Apr 24, 2020

derrickstolee closed this Apr 27, 2020

gitgitgadget bot reviewed Jun 7, 2020

View reviewed changes

		@@ -661,6 +661,15 @@ static void prepare_to_use_bloom_filter(struct rev_info *revs)
		if (!revs->commits)

		@@ -1701,7 +1701,9 @@ int cmd_commit(int argc, const char *argv, const char prefix)
		"not exceeded, and then \"git restore --staged :/\" to recover."));

		@@ -650,6 +650,20 @@ static void trace2_bloom_filter_statistics_atexit(void)
		jw_release(&jw);

		@@ -1968,6 +1968,8 @@ int write_commit_graph(struct object_directory *odb,
		ctx->check_oids = flags & COMMIT_GRAPH_WRITE_CHECK_OIDS ? 1 : 0;

Integrate changed-path Bloom filters with 'git blame' #609

Integrate changed-path Bloom filters with 'git blame' #609

Conversation

derrickstolee commented Apr 10, 2020 • edited Loading

derrickstolee commented Apr 11, 2020

gitgitgadget bot commented Apr 11, 2020

gitgitgadget bot commented Apr 11, 2020

gitgitgadget bot Apr 11, 2020

Choose a reason for hiding this comment

gitgitgadget bot Apr 13, 2020

Choose a reason for hiding this comment

gitgitgadget bot Apr 14, 2020

Choose a reason for hiding this comment

gitgitgadget bot Apr 15, 2020

Choose a reason for hiding this comment

gitgitgadget bot Apr 15, 2020

Choose a reason for hiding this comment

gitgitgadget bot Apr 16, 2020

Choose a reason for hiding this comment

gitgitgadget bot Apr 16, 2020

Choose a reason for hiding this comment

gitgitgadget bot Apr 16, 2020

Choose a reason for hiding this comment

gitgitgadget bot Apr 16, 2020

Choose a reason for hiding this comment

gitgitgadget bot Apr 16, 2020

Choose a reason for hiding this comment

gitgitgadget bot Apr 11, 2020

Choose a reason for hiding this comment

gitgitgadget bot Apr 12, 2020

Choose a reason for hiding this comment

gitgitgadget bot Apr 13, 2020

Choose a reason for hiding this comment

gitgitgadget bot Apr 13, 2020

Choose a reason for hiding this comment

gitgitgadget bot Apr 11, 2020

Choose a reason for hiding this comment

gitgitgadget bot Apr 12, 2020

Choose a reason for hiding this comment

gitgitgadget bot commented Apr 12, 2020

gitgitgadget bot commented Apr 12, 2020

gitgitgadget bot Apr 12, 2020

Choose a reason for hiding this comment

gitgitgadget bot Apr 12, 2020

Choose a reason for hiding this comment

gitgitgadget bot Apr 13, 2020

Choose a reason for hiding this comment

gitgitgadget bot Apr 13, 2020

Choose a reason for hiding this comment

derrickstolee commented Apr 13, 2020

gitgitgadget bot commented Apr 13, 2020

gitgitgadget bot Apr 13, 2020

Choose a reason for hiding this comment

gitgitgadget bot Apr 13, 2020

Choose a reason for hiding this comment

gitgitgadget bot Apr 13, 2020

Choose a reason for hiding this comment

gitgitgadget bot Apr 13, 2020

Choose a reason for hiding this comment

gitgitgadget bot Apr 14, 2020

Choose a reason for hiding this comment

gitgitgadget bot Apr 14, 2020

Choose a reason for hiding this comment

gitgitgadget bot Apr 14, 2020

Choose a reason for hiding this comment

gitgitgadget bot Apr 15, 2020

Choose a reason for hiding this comment

gitgitgadget bot commented Apr 13, 2020

gitgitgadget bot commented Apr 13, 2020

gitgitgadget bot commented Apr 14, 2020

gitgitgadget bot commented Apr 15, 2020

gitgitgadget bot commented Apr 15, 2020

derrickstolee commented Apr 16, 2020

gitgitgadget bot commented Apr 16, 2020

gitgitgadget bot commented Apr 18, 2020

gitgitgadget bot commented Apr 20, 2020

gitgitgadget bot commented Apr 21, 2020

gitgitgadget bot commented Apr 24, 2020

gitgitgadget bot commented Apr 24, 2020

gitgitgadget bot Jun 7, 2020

derrickstolee commented Apr 10, 2020 •

edited

Loading