bloom: make bloom-filter slab initialization idempotent

Before using any of the commit-graph bloom-filter code, somebody needs
to call init_bloom_filters(). This initializes the commit-slab we use
for storing filter information. But we don't want to call it twice
(without a matching deinit call in the middle), since it overwrites the
existing slab pointers, leaking the old values.

Usually this init call is done lazily by parse_commit_graph() when we
read a graph file that contains bloom data. But this can lead to some
oddities:

  1. We may call parse_commit_graph() multiple times when we have a
     split commit graph. I think this doesn't produce any user-visible
     bug, because we parse all of the files back-to-back. So even though
     we call init_bloom_filters() multiple times, we never look up any
     commits in between, so the slab is always empty and initializing it
     again happens to do nothing. This is a little sketchy to rely on,
     though.

  2. We call init_bloom_filters() directly in the "test-tool bloom"
     helper so we can call get_or_compute_bloom_filter(). Normally this
     is OK, as there is no bloom data in the on-disk graph file. But if
     you build with SANITIZE=leak and run:

       GIT_TEST_COMMIT_GRAPH=1 \
       GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=1 \
       ./t0095-bloom.sh

     there's a leak that happens like this:

       a. Our direct init_bloom_filters() sets up the slab.

       b. In get_or_compute_bloom_filter() we look in the slab for a
	  cached entry. We won't find anything yet, but since we don't
	  use the read-only "peek" accessor (since we'll fill in the
	  entry if not present), this actually populates the slab with
	  an allocated chunk.

       c. Now we look for an entry in the graph files. So we have to
	  load them and end up in parse_commit_graph(), which calls
	  init_bloom_filters() again. That trashes our existing slab
	  allocation, which is now leaked.

  3. There's a similar case in write_commit_graph(), which calls
     init_bloom_filters() before get_or_compute_bloom_filter(). I think
     this code path is lucky to avoid the leak because it reads the
     graph files first, then calls its init_bloom_filters(), and then
     starts filling in entries. So even though it has the same overwrite
     problem, we'd never actually allocate any slab entries between
     overwrites.

The easiest solution here is just to make initialization of the slab
idempotent using an extra flag.

We could actually get away without using the extra flag, for example by
checking whether bloom_filters.stride has been set. But it's probably
better to avoid being too intimate with the commit-slab details.
Likewise we don't actually need to re-initialize after a deinit call;
the slab-clearing function leaves things in a usable state. But it
seemed less surprising to pair the init/deinit calls explicitly.

I suspect this could all be cleaned up a bit more, but it's tricky. The
only function which uses the slab is get_or_compute_bloom_filter(), so
it would be much simpler if it just lazy-initialized the slab itself.
But I think there is a subtle dependency here: we usually only
initialize the slab when we find a graph file that has bloom entries. So
if we were to lose that signal, then even repos without on-disk bloom
data would start trying to populate the slab, wasting memory that will
never get entries filled in from the disk. So we'd need some other way
of signaling "it is worth considering bloom entries at all".

This patch takes a smaller and more direct route to just dealing with
the potential leak issue.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/bloom.c b/bloom.c
index a805ac0c296b37754f43bf0ce29af9ba19e76ea0..c98d1672adb71ad3061b957bbeafa854cdb74826 100644 (file)
--- a/bloom.c
+++ b/bloom.c
@@ -16,6 +16,7 @@
 define_commit_slab(bloom_filter_slab, struct bloom_filter);
 
 static struct bloom_filter_slab bloom_filters;
+static int bloom_filter_slab_initialized;
 
 struct pathmap_hash_entry {
     struct hashmap_entry entry;
@@ -263,7 +264,10 @@ void add_key_to_filter(const struct bloom_key *key,
 
 void init_bloom_filters(void)
 {
+       if (bloom_filter_slab_initialized)
+               return;
        init_bloom_filter_slab(&bloom_filters);
+       bloom_filter_slab_initialized = 1;
 }
 
 static void free_one_bloom_filter(struct bloom_filter *filter)
@@ -276,6 +280,7 @@ static void free_one_bloom_filter(struct bloom_filter *filter)
 void deinit_bloom_filters(void)
 {
        deep_clear_bloom_filter_slab(&bloom_filters, free_one_bloom_filter);
+       bloom_filter_slab_initialized = 0;
 }
 
 struct bloom_keyvec *bloom_keyvec_new(const char *path, size_t len,