Re: [PATCH 1/4] odb: decouple source path comparisons from `the_repository`

On 26/08/10 03:33PM, Patrick Steinhardt wrote:
> When registering alternates we deduplicate object database sources by
> their path so that the same source won't be added twice. Ever since
> cf2dc1c238 (speed up alt_odb_usable() with many alternates, 2021-07-07)
> this duplicate check is backed by a map keyed by the source's path,
> using `fspathhash()` and `fspatheq()` as hash and equality functions,
> respectively.
> 
> These functions are problematic in this context for two reasons:
> 
>   - They implicitly depend on `the_repository` instead of the
>     repository that owns the object database.
> 
>   - They derive case-sensitivity from `repo_ignore_case()`, which
>     returns a default value in case the repository's configuration has
>     not been parsed yet. Object database sources may be registered
>     before that is the case, so the answer may flip depending on when a
>     source gets registered.

Are alternates currently always registered after repository
configuration has been parsed? Or is this an existing bug?

> Fix this by making the comparison self-contained in the object
> database. Instead of using `fspathhash()` and `fspatheq()` we resolve
> "core.ignoreCase" manually and then use the correct comparison function
> based on the result. This requires us to migrate to a `struct hashmap`,
> as the khash interface does not give us the ability to change these
> functions.
> 
> Note that we can unconditionally use `strihash()` to compute entry
> hashes regardless of case sensitivity: a hash function only needs to
> guarantee that equal keys have equal hashes, and a case-insensitive
> hash satisfies this requirement for both case-sensitive and
> case-insensitive equality.

Ok IIUC, even if we want to be case-sensitive, its ok to use
`strihash()` and have hash collisions because the compare function will
still properly distinguish between the cases. Makes sense.

> Overall it's quite debatable whether all of this complexity really is
> worth it, or whether we should just linearly search through all sources
> to find duplicates. But the mentioned commit cares about cases with
> thousands of alternates, and a linear search would of course regress
> performance quite a bit. This doesn't really feel like a reasonable case
> to care about though, but I don't feel comfortable regressing it anyway.

Ya, my first though here was also whether all of this song and dance is
really needed for alternates. There may be someone out there with tons
of alternates I guess though. Probably good to be on the safe side.

> Signed-off-by: Patrick Steinhardt <[email protected]>
> ---
>  odb.c        | 63 ++++++++++++++++++++++++++++++++++++++++--------------------
>  odb.h        | 15 ++++++++++++++-
>  odb/source.h |  7 +++++++
>  3 files changed, 63 insertions(+), 22 deletions(-)
> 
> diff --git a/odb.c b/odb.c
> index bd02d8ad54..51da386f22 100644
> --- a/odb.c
> +++ b/odb.c
> @@ -2,11 +2,10 @@
>  #include "abspath.h"
>  #include "commit-graph.h"
>  #include "config.h"
> -#include "dir.h"
>  #include "environment.h"
>  #include "gettext.h"
> +#include "hashmap.h"
>  #include "hex.h"
> -#include "khash.h"
>  #include "lockfile.h"
>  #include "loose.h"
>  #include "midx.h"
> @@ -29,8 +28,32 @@
>  #include "trace2.h"
>  #include "write-or-die.h"
>  
> -KHASH_INIT(odb_path_map, const char * /* key: odb_path */,
> -	struct odb_source *, 1, fspathhash, fspatheq)
> +static int odb_source_paths_cmp(struct object_database *o,
> +				const char *a, const char *b)
> +{
> +	if (o->source_paths_icase < 0) {
> +		int icase = 0;
> +		repo_config_get_bool(o->repo, "core.ignorecase", &icase);
> +		o->source_paths_icase = icase;
> +	}

We now parse ignorecase configuration here directly and store the result
in `source_paths_icase`. This ensures configuration is correctly applied
regardless of whether repository configuration has been fully read yet.

> +	return o->source_paths_icase ? strcasecmp(a, b) : strcmp(a, b);
> +}
> +
> +static int odb_source_by_path_cmp(const void *cb_data,
> +				  const struct hashmap_entry *entry,
> +				  const struct hashmap_entry *entry_or_key,
> +				  const void *keydata)
> +{
> +	struct object_database *o = (struct object_database *)cb_data;
> +	const struct odb_source *source = container_of(entry, const struct odb_source, by_path_entry);
> +	const char *path = keydata;
> +
> +	if (!path)
> +		path = container_of(entry_or_key, const struct odb_source, by_path_entry)->path;
> +
> +	return odb_source_paths_cmp(o, source->path, path);
> +}

Here is the comparison callback that is used for the hashmap.

>  int odb_mkstemp(struct object_database *odb,
>  		struct strbuf *temp_filename, const char *pattern)
> @@ -58,8 +81,8 @@ int odb_mkstemp(struct object_database *odb,
>   */
>  static bool odb_is_source_usable(struct object_database *o, const char *path)
>  {
> -	int r;
>  	struct strbuf normalized_objdir = STRBUF_INIT;
> +	struct hashmap_entry key;
>  	bool usable = false;
>  
>  	strbuf_realpath(&normalized_objdir, o->sources->path, 1);
> @@ -76,20 +99,18 @@ static bool odb_is_source_usable(struct object_database *o, const char *path)
>  	 * Prevent the common mistake of listing the same
>  	 * thing twice, or object directory itself.
>  	 */
> -	if (!o->source_by_path) {
> -		khiter_t p;
> -
> -		o->source_by_path = kh_init_odb_path_map();
> +	if (!hashmap_get_size(&o->source_by_path)) {
>  		assert(!o->sources->next);
> -		p = kh_put_odb_path_map(o->source_by_path, o->sources->path, &r);
> -		assert(r == 1); /* never used */
> -		kh_value(o->source_by_path, p) = o->sources;
> +		hashmap_entry_init(&o->sources->by_path_entry,
> +				   strihash(o->sources->path));
> +		hashmap_add(&o->source_by_path, &o->sources->by_path_entry);

The hashmap is lazily set up with the primary source. I do find some of
the variable names like "source_by_path" a bit vague, but that isn't
really anything new here.

>  	}
>  
> -	if (fspatheq(path, normalized_objdir.buf))
> +	if (!odb_source_paths_cmp(o, path, normalized_objdir.buf))
>  		goto out;

If the path matches the first entry in the sources list then we know it
is not an alternate.

>  
> -	if (kh_get_odb_path_map(o->source_by_path, path) < kh_end(o->source_by_path))
> +	hashmap_entry_init(&key, strihash(path));
> +	if (hashmap_get(&o->source_by_path, &key, path))
>  		goto out;

If the alternates source cannot be found for the given path, then we
also know it is not a usuable alternate.

>  	usable = true;
> @@ -172,8 +193,6 @@ static struct odb_source *odb_add_alternate_recursively(struct object_database *
>  {
>  	struct odb_source *alternate = NULL;
>  	struct strvec sources = STRVEC_INIT;
> -	khiter_t pos;
> -	int ret;
>  
>  	if (!odb_is_source_usable(odb, source))
>  		goto error;
> @@ -184,10 +203,11 @@ static struct odb_source *odb_add_alternate_recursively(struct object_database *
>  	*odb->sources_tail = alternate;
>  	odb->sources_tail = &(alternate->next);
>  
> -	pos = kh_put_odb_path_map(odb->source_by_path, alternate->path, &ret);
> -	if (!ret)
> +	hashmap_entry_init(&alternate->by_path_entry, strihash(alternate->path));
> +	if (hashmap_get(&odb->source_by_path, &alternate->by_path_entry,
> +			alternate->path))
>  		BUG("source must not yet exist");
> -	kh_value(odb->source_by_path, pos) = alternate;
> +	hashmap_add(&odb->source_by_path, &alternate->by_path_entry);

Here is where alternates get registered and added to the hashmap. Makes
sense.

Overall this patch looks good.

-Justin