git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Derrick Stolee <stolee@gmail.com>
Cc: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, me@ttaylorr.com, jnareb@gmail.com,
	garimasigit@gmail.com, Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH 1/3] revision: complicated pathspecs disable filters
Date: Wed, 15 Apr 2020 12:32:42 -0700	[thread overview]
Message-ID: <xmqqr1wo5yud.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <99e0ae2c-6b65-24e4-3d2b-1dff619a5daa@gmail.com> (Derrick Stolee's message of "Wed, 15 Apr 2020 14:37:40 -0400")

Derrick Stolee <stolee@gmail.com> writes:

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

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

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

  reply	other threads:[~2020-04-15 19:33 UTC|newest]

Thread overview: 48+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-11  1:02 [PATCH 0/3] Integrate changed-path Bloom filters with 'git blame' Derrick Stolee via GitGitGadget
2020-04-11  1:02 ` [PATCH 1/3] revision: complicated pathspecs disable filters Derrick Stolee via GitGitGadget
2020-04-11 21:40   ` Junio C Hamano
2020-04-13 11:49     ` Derrick Stolee
2020-04-14 18:25       ` Junio C Hamano
2020-04-15 13:27         ` Derrick Stolee
2020-04-15 18:37           ` Derrick Stolee
2020-04-15 19:32             ` Junio C Hamano [this message]
2020-04-15 19:39               ` Junio C Hamano
2020-04-15 21:25             ` Junio C Hamano
2020-04-16  0:56               ` Taylor Blau
2020-04-15 22:18             ` Jakub Narębski
2020-04-16  0:52               ` Taylor Blau
2020-04-16 13:26                 ` Derrick Stolee
2020-04-16 16:33                   ` Taylor Blau
2020-04-16 18:02                     ` Junio C Hamano
2020-04-12 22:22   ` Taylor Blau
2020-04-12 22:30     ` Junio C Hamano
2020-04-13  0:07       ` Taylor Blau
2020-04-13 11:54         ` Derrick Stolee
2020-04-11  1:03 ` [PATCH 2/3] commit: write commit-graph with bloom filters Derrick Stolee via GitGitGadget
2020-04-11 21:57   ` Junio C Hamano
2020-04-12 20:51     ` Taylor Blau
2020-04-13 12:08       ` Derrick Stolee
2020-04-13 22:11         ` Junio C Hamano
2020-04-11  1:03 ` [PATCH 3/3] blame: use changed-path Bloom filters Derrick Stolee via GitGitGadget
2020-04-11 22:03   ` Junio C Hamano
2020-04-12  7:39     ` Eric Sunshine
2020-04-11 21:30 ` [PATCH 0/3] Integrate changed-path Bloom filters with 'git blame' Junio C Hamano
2020-04-13 14:45 ` [PATCH v2 0/4] " Derrick Stolee via GitGitGadget
2020-04-13 14:45   ` [PATCH v2 1/4] revision: complicated pathspecs disable filters Derrick Stolee via GitGitGadget
2020-04-13 16:09     ` Taylor Blau
2020-04-13 22:18       ` Junio C Hamano
2020-04-13 14:45   ` [PATCH v2 2/4] commit: write commit-graph with Bloom filters Derrick Stolee via GitGitGadget
2020-04-13 16:12     ` Taylor Blau
2020-04-13 22:21       ` Junio C Hamano
2020-04-14 15:04         ` Derrick Stolee
2020-04-14 17:26           ` Junio C Hamano
2020-04-14 17:40             ` Derrick Stolee
2020-04-15  0:17               ` Taylor Blau
2020-04-13 14:45   ` [PATCH v2 3/4] commit-graph: write commit-graph in more tests Derrick Stolee via GitGitGadget
2020-04-13 14:45   ` [PATCH v2 4/4] blame: use changed-path Bloom filters Derrick Stolee via GitGitGadget
2020-04-13 16:21   ` [PATCH v2 0/4] Integrate changed-path Bloom filters with 'git blame' Taylor Blau
2020-04-16 20:14   ` [PATCH v3 0/3] " Derrick Stolee via GitGitGadget
2020-04-16 20:14     ` [PATCH v3 1/3] revision: complicated pathspecs disable filters Derrick Stolee via GitGitGadget
2020-06-07 20:33       ` SZEDER Gábor
2020-04-16 20:14     ` [PATCH v3 2/3] tests: write commit-graph with Bloom filters Derrick Stolee via GitGitGadget
2020-04-16 20:14     ` [PATCH v3 3/3] blame: use changed-path " Derrick Stolee via GitGitGadget

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqr1wo5yud.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=dstolee@microsoft.com \
    --cc=garimasigit@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=jnareb@gmail.com \
    --cc=me@ttaylorr.com \
    --cc=stolee@gmail.com \
    --subject='Re: [PATCH 1/3] revision: complicated pathspecs disable filters' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).