Subject: Re: [PATCH v2 00/11] Changed Paths Bloom Filters

From: Jakub Narebski <[email-masked]>
Date: Sun, 09 Feb 2020 00:04:41 +0100

"Garima Singh via GitGitGadget" <[email-masked]> writes:

> Hey! 
>
> The commit graph feature brought in a lot of performance improvements across
> multiple commands. However, file based history continues to be a performance
> pain point, especially in large repositories. 
>
> Adopting changed path Bloom filters has been discussed on the list before,
> and a prototype version was worked on by SZEDER Gábor, Jonathan Tan and Dr.
> Derrick Stolee [1]. This series is based on Dr. Stolee's proof of
> concept in [2].

Sidenote: I wondered why it did use MurmurHash3 (64-bit version), which
requires adding its implementation, instead of reusing FNV-1 hash
(Fowler–Noll–Vo hash function) used by Git hashmap implementation, see
https://github.com/git/git/blob/228f53135a4a41a37b6be8e4d6e2b6153db4a8ed/hashmap.h#L109
Beside the fact that everyone is using MurmurHash for Bloom filters ;-)

It turns out that in various benchmark MurmurHash is faster and also
slightly better as a hash than FNV-1 or FNV-1b.

I wonder then if it would be a good idea (in the future) to make it easy
to use hashmap with MurmurHash3 instead of FNV-1, or maybe to even make
it the default for hashing strings.

>
> Performance Gains: We tested the performance of git log -- path on the git
> repo, the linux repo and some internal large repos, with a variety of paths
> of varying depths.

As I wrote in reply to previous version of this series, a good public
repository (and thus being able to use by anyone) to test the Bloom
filter performance improvements could be AOSP (Android) base:

  https://android.googlesource.com/platform/frameworks/base/

which is a large repository with long path depths (due to Java file
naming conventions).

>
> On the git and linux repos: We observed a 2x to 5x speed up.
>
> On a large internal repo with files seated 6-10 levels deep in the tree: We
> observed 10x to 20x speed ups, with some paths going up to 28 times faster.

Very nice! Good work!

What is the cost of this feature, that is how long it takes to generate
Bloom filters, and how much larger commit-graph file gets?  It would be
nice to know.

>
> Future Work (not included in the scope of this series):
>
>  1. Supporting multiple path based revision walk

Shouldn't then tests that were added in v2 mark use of Bloom filters
with multiple paths revision walking as _not working *yet*_
(test_expect_failure), and not expected to not work (test_expect_success
with test_bloom_filters_not_used)?

>  2. Adopting it in git blame logic. 
>  3. Interactions with line log git log -L
>
>
> ----------------------------------------------------------------------------
>
> Updates since the last submission
>
>  * Removed all the RFC callouts, this is a ready for full review version
>  * Added unit tests for the bloom filter computation layer
>  * Added more evolved functional tests for git log
>  * Fixed a lot of the bugs found by the tests
>  * Reacted to other miscellaneous feedback on the RFC series. 
>
> Cheers! Garima Singh
>
> [1] https://lore.kernel.org/git/[email-masked]/
> [2] https://lore.kernel.org/git/[email-masked]/
>
> Derrick Stolee (2):
>   diff: halt tree-diff early after max_changes
>   commit-graph: examine commits by generation number
>
> Garima Singh (8):
>   commit-graph: use MAX_NUM_CHUNKS
>   bloom: core Bloom filter implementation for changed paths
>   commit-graph: compute Bloom filters for changed paths
>   commit-graph: write Bloom filters to commit graph file
>   commit-graph: reuse existing Bloom filters during write.
>   commit-graph: add --changed-paths option to write subcommand
>   revision.c: use Bloom filters to speed up path based revision walks
>   commit-graph: add GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS test flag
>
> Jeff King (1):
>   commit-graph: examine changed-path objects in pack order

The shortlog summary is a fine tool to show contributors to the patch
series, but is not as useful to show patch series as a whole: splitting
of patches and their ordering.

I will review each of patches individually, but now I would like to say
a few things about the series as a whole.

- [PATCH v2 01/11] commit-graph: use MAX_NUM_CHUNKS

  Simple and non-controversial patch, improvement to existing code with
  the goal of helping future development (including further patches).

- [PATCH v2 02/11] bloom: core Bloom filter implementation for changed paths

  In my opinion this patch could be split into three individual pieces,
  though one might think it is not worth it.

  a. Add implementation of MurmurHash v3 (64-bit)
  
  Include tests based on test-tool (creating file similar to the
  t/helper/test-hash.c, or enhancing to that file) that the
  implementation is correct, for example that 'The quick brown fox jumps
  over the lazy dog' with given seed (for example the default feed of 0)
  hashes to the same value as other implementations.

  b. Add implementation of Bloom filter

  Include generic Bloom filter tests i.e. that it correctly answers
  "yes" and "maybe" (create filter, save it or print it, then use stored
  filter), and tests specific to our implementation, namely that the
  size of the filter behaves as it should.

  c. Bloom filter implementation for changed paths

  Here include tests that use 'test-tool bloom get_filter_for_commit',
  that filter for commit with no changes and for commit with more than
  512 fies changed works correctly, that directories are added along the
  paths, etc.

- [PATCH v2 03/11] diff: halt tree-diff early after max_changes

  I think keeping this patch as a separate step makes individual commits
  easier to understand and review.

- [PATCH v2 04/11] commit-graph: compute Bloom filters for changed paths

  Here we compute Bloom filters for changed paths for each commit in the
  commit-graph file, without writing it to file; as a side-effect we
  calculate total Bloom filters data size.

  This doesn't make much sense as a standalone patch, but it is nice,
  easy to understand incremental step in building the feature.

- [PATCH v2 05/11] commit-graph: examine changed-path objects in pack order
- [PATCH v2 06/11] commit-graph: examine commits by generation number

  Those two are performance improvements of previous step.  It is good
  to keep them as separate commits, makes it easier to understand (and
  easier to catch error via git-bisect, if there would be any)

- [PATCH v2 07/11] commit-graph: write Bloom filters to commit graph file

  This commit includes the documentation of the two new chunks of
  commit-graph file format.

  I wonder if the 9th patch in this series, namely
  commit-graph: add --changed-paths option to write subcommand
  should not precede this commit.  Otherwise we have this new code but
  no way of testing it.  On the other hand it makes it easier to
  review.  On the gripping hand, you can't really test that writing
  works without the ability to parse Bloom filter data out of
  commit-graph file... which is the next commit.

- [PATCH v2 08/11] commit-graph: reuse existing Bloom filters during write

  This implements reading Bloom filters data from commit-graph file.
  Is it a good split?  I think it makes it easier to review the single
  patch, but itt also makes them less standalone.

- [PATCH v2 09/11] commit-graph: add --changed-paths option to write subcommand

  One thing we could test there is that we are writing two new chunks to
  the commit-graph file (and perhaps checking that they are correctly
  formatted, and have correct shape).

- [PATCH v2 10/11] revision.c: use Bloom filters to speed up path based revision walks

  This is quite a big and involved patch, which in my opinion could be
  split in two or three parts:

  a. Add a bare bones implementation, like in v2

  This limits amount of testing we can do; the only thing we can really
  test is that we get the same results with and without Bloom filters.

  b.1. Add trace2 Bloom filter statistics
  b.2. Use said trace2 statistics to test use of Bloom filters

- [PATCH v2 11/11] commit-graph: add GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS test flag

  This one is for (optional) exhaustive testing of the feature.

Feel free to disagree with those ideas.

Best,
-- 
Jakub Narębski