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

[Garima Singh <garimasigit@gmail.com>]

On 2/20/2020 1:48 PM, Jakub Narebski wrote:
> "Garima Singh via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Garima Singh <garima.singh@microsoft.com>
>>
>> Read previously computed Bloom filters from the commit-graph file if
>> possible to avoid recomputing during commit-graph write.
> 
> All right, what is written makes sense for this point in patch series.
> 
> But it my opinion it is more important to state that this commit adds
> "parsing" of the Bloom filter data from commit-graph file.  This means
> that it needs to be calculated only once, then stored in commit-graph,
> ready to be re-used.
> 

Good point. Incorporated in v3.

>>
>> See Documentation/technical/commit-graph-format for the format in which
>> the Bloom filter information is written to the commit graph file.
>>
>> To read Bloom filter for a given commit with lexicographic position
>> 'i' we need to:
>> 1. Read BIDX[i] which essentially gives us the starting index in BDAT for
>>    filter of commit i+1. It is essentially the index past the end
>>    of the filter of commit i. It is called end_index in the code.
>>
>> 2. For i>0, read BIDX[i-1] which will give us the starting index in BDAT
>>    for filter of commit i. It is called the start_index in the code.
>>    For the first commit, where i = 0, Bloom filter data starts at the
>>    beginning, just past the header in the BDAT chunk. Hence, start_index
>>    will be 0.
>>
>> 3. The length of the filter will be end_index - start_index, because
>>    BIDX[i] gives the cumulative 8-byte words including the ith
>>    commit's filter.
>>
>> We toggle whether Bloom filters should be recomputed based on the
>> compute_if_null flag.
> 
> Nitpick: the flag (the parameter) is called compute_if_not_present, not
> compute_if_null.
> 
Oops. Fixed in v3. 

>> +
>> +	end_index = get_be32(g->chunk_bloom_indexes + 4 * lex_pos);
>> +
>> +	if (lex_pos)
> 
> Wouldn't it be better to be more explicit, and write
> 
>   +	if (lex_pos > 0)
> 
> 

Sure. 

>> +		start_index = get_be32(g->chunk_bloom_indexes + 4 * (lex_pos - 1));
>> +	else
>> +		start_index = 0;
> 
> All right, here we find start_index and end_index.
> 
> It might be good idea to at least assert() that start_index <= end_index,
> though that should not happen (that is why I propose for this check to
> be compiled on only for debug builds).
> 

I will look into this. Thanks! 


>> @@ -1304,7 +1304,7 @@ static void compute_bloom_filters(struct write_commit_graph_context *ctx)
>>  
>>  	for (i = 0; i < ctx->commits.nr; i++) {
>>  		struct commit *c = sorted_by_pos[i];
>> -		struct bloom_filter *filter = get_bloom_filter(ctx->r, c);
>> +		struct bloom_filter *filter = get_bloom_filter(ctx->r, c, 1);
>>  		ctx->total_bloom_filter_data_size += sizeof(uint64_t) * filter->len;
>>  		display_progress(progress, i + 1);
>>  	}
>> @@ -2314,6 +2314,7 @@ void free_commit_graph(struct commit_graph *g)
>>  		g->data = NULL;
>>  		close(g->graph_fd);
>>  	}
>> +	free(g->bloom_filter_settings);
>>  	free(g->filename);
>>  	free(g);
> 
> Shouldn't this fixup be added to earlier commit?
> 

Yes. 

>>  }
>> diff --git a/t/helper/test-bloom.c b/t/helper/test-bloom.c
>> index 331957011b..9b4be97f75 100644
>> --- a/t/helper/test-bloom.c
>> +++ b/t/helper/test-bloom.c
>> @@ -47,7 +47,7 @@ static void get_bloom_filter_for_commit(const struct object_id *commit_oid)
>>  	struct bloom_filter *filter;
>>  	setup_git_directory();
>>  	c = lookup_commit(the_repository, commit_oid);
>> -	filter = get_bloom_filter(the_repository, c);
>> +	filter = get_bloom_filter(the_repository, c, 1);
>>  	print_bloom_filter(filter);
>>  }
> 
> I would like to see some tests, but that needs to wait for patch that
> adds --changed-paths option to the 'write' subcommand.
> 
> Things to be tested:
> 1. That after reading commit-graph with Bloom filter:
>    - that commit(s) in commit-graph have Bloom filter
>    - that commits outside commit-graph do not have Bloom filter
> 2. That incremental commit-graph feature works:
>    - for commits in deeper layer that have Bloom filter chunks
>    - for commits in deeper layer that do not have Bloom filter chunks
> 

Included in later commits. 

> Best,
>