Re: [PATCH 1/3] revision: complicated pathspecs disable filters

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 15, 2020, 19:32 UTC
Message-ID: <xmqqr1wo5yud.fsf@gitster.c.googlers.com>
In-Reply-To: <99e0ae2c-6b65-24e4-3d2b-1dff619a5daa@gmail.com>

Derrick Stolee <stolee@gmail.com> writes:

Show 16 quoted lines

> 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);

Perhaps

	strbuf_reset(&icase_data);
	strbuf_add(&icase_data, data, len);

Or do we know bloom keys are always NUL-terminated string?

I am not sure how reusable bloom.c::fill_bloom_key() is designed to be. If it is for the sole use of the changed-paths, then it is OK to assume that our data is NUL-terminated string and that keys wants to be always computed after downcasing (assuming that icase search is something we want to optimize for, which I find is a bit iffy).

If not, obviously we would want these two things done on the caller's side (or perhaps a new helper function whose callers can make these assumption, fill_bloom_path(), may want to be inserted between fill_bloom_key() and its callers).

Show 10 quoted lines

> +	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++)

In any case, the update to the above function seems fairly isolated. Nicely done.

Show 21 quoted lines

> 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;

And thanks to the refactoring done to invent this helper function, the side that uses the Bloom data is quite straight-forward.

As you are, I am on the fence.

I do not think :(icase) pathspec is something we want to optimize for, but I still like this new hash function primarily because I suspect that it will increase the number of paths that you can cram into the filter without getting their hashes collided (hence getting false positive), under the assumption that real projects won't try to store too many pair of paths that are only different in their case, and if that is the case, it would help performance. So if we were to benchmark, we'd also pay attention to that side, in addition to the obvious downside of having to allocate and downcase.

Thanks.