Re: [RFC PATCH 4/6] commit-graph.c: unconditionally load Bloom filters
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Aug 11, 2023, 22:00 UTC
- Message-ID
- <20230811220030.3329161-1-jonathantanmy@google.com>
- In-Reply-To
- <6676afe56db74828ba2532f3460e9b73a3ff20fd.1691426160.git.me@ttaylorr.com>
Taylor Blau <me@ttaylorr.com> writes:
Show 13 quoted lines
> diff --git a/commit-graph.c b/commit-graph.c
> index 38edb12669..60e5f9ada7 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -317,12 +317,6 @@ static int graph_read_bloom_data(const unsigned char *chunk_start,
> uint32_t hash_version;
> hash_version = get_be32(chunk_start);
>
> - if (*c->commit_graph_changed_paths_version == -1) {
> - *c->commit_graph_changed_paths_version = hash_version;
> - } else if (hash_version != *c->commit_graph_changed_paths_version) {
> - return 0;
> - }Lots of things to notice in this patch, but the summary is that this patch looks correct.
Here, we remove (1) the autodetection of changed paths version and (2) the storage of the result of that autodetection. (1) is restored in a subsequent code hunk, but (2) is never restored.
Prior to this patch set, we needed (2) because we only wanted to load Bloom filters that match a given version, so if we autodetected a version, that version must be in effect for all future loads (which is why we needed to store that result immediately). But with this patch set, we support the loading of Bloom filters of varying versions, so storing it immediately is no longer needed.
(Also, I don't think we need the commit_graph_changed_paths_version variable anymore.)
Show 7 quoted lines
> @@ -2390,8 +2384,7 @@ int write_commit_graph(struct object_directory *odb, > r->settings.commit_graph_changed_paths_version); > return 0; > } > - bloom_settings.hash_version = r->settings.commit_graph_changed_paths_version == 2 > - ? 2 : 1; > + bloom_settings.hash_version = r->settings.commit_graph_changed_paths_version;
As stated in the commit message, normalization of the hash version is performed later.
Show 12 quoted lines
> @@ -2423,10 +2416,18 @@ int write_commit_graph(struct object_directory *odb,
> /* We have changed-paths already. Keep them in the next graph */
> if (g && g->bloom_filter_settings) {
> ctx->changed_paths = 1;
> - ctx->bloom_settings = g->bloom_filter_settings;
> +
> + /* don't propagate the hash_version unless unspecified */
> + if (bloom_settings.hash_version == -1)
> + bloom_settings.hash_version = g->bloom_filter_settings->hash_version;
> + bloom_settings.bits_per_entry = g->bloom_filter_settings->bits_per_entry;
> + bloom_settings.num_hashes = g->bloom_filter_settings->num_hashes;
> + bloom_settings.max_changed_paths = g->bloom_filter_settings->max_changed_paths;Here is where the autodetection is restored.
Prior to this patch set, this part of the code does not perform autodetection - every hash_version in memory matches the version in commit_graph_changed_paths_version, so we're just copying over the value in that variable. But the nature of this part of the code has changed due to this patch set: all the g->bloom_filter_settings in memory may not have the same hash version, and may not match what the user specified in config. To compensate, we are more selective in what we propagate from g->bloom_filter_settings.
> + bloom_settings.hash_version = bloom_settings.hash_version == 2 ? 2 : 1;
And here we restore the normalization.
Thanks - up to here looks good.