Re: [PATCH] sha1_name: cache readdir(3) results in find_short_object_filename()

From: Jeff King <peff@peff.net>
Date: Jun 22, 2017, 23:10 UTC
Message-ID: <20170622231041.2zdjypviwmndjnsb@sigill.intra.peff.net>
In-Reply-To: <d06fb033-294e-f364-3dde-394624e83cd6@web.de>

On Thu, Jun 22, 2017 at 08:19:48PM +0200, René Scharfe wrote:

> Read each loose object subdirectory at most once when looking for unique
> abbreviated hashes.  This speeds up commands like "git log --pretty=%h"
> considerably, which previously caused one readdir(3) call for each
> candidate, even for subdirectories that were visited before.

Is it worth adding a perf test that's heavy on abbreviations?

Show 11 quoted lines

> The new cache is kept until the program ends and never invalidated.  The
> same is already true for pack indexes.  The inherent racy nature of
> finding unique short hashes makes it still fit for this purpose -- a
> conflicting new object may be added at any time.  Tasks with higher
> consistency requirements should not use it, though.
> 
> The cached object names are stored in an oid_array, which is quite
> compact.  The bitmap for remembering which subdir was already read is
> stored as a char array, with one char per directory -- that's not quite
> as compact, but really simple and incurs only an overhead equivalent to
> 11 hashes after all.

Sounds reasonable. The code itself looks good, with one minor nit below.

Show 14 quoted lines

> @@ -1593,6 +1594,16 @@ extern struct alternate_object_database {
>  	struct strbuf scratch;
>  	size_t base_len;
>  
> +	/*
> +	 * Used to store the results of readdir(3) calls when searching
> +	 * for unique abbreviated hashes.  This cache is never
> +	 * invalidated, thus it's racy and not necessarily accurate.
> +	 * That's fine for its purpose; don't use it for tasks requiring
> +	 * greater accuracy!
> +	 */
> +	char loose_objects_subdir_seen[256];
> +	struct oid_array loose_objects_cache;
> +

Thanks for adding this comment.

Show 10 quoted lines

> @@ -1811,6 +1822,12 @@ typedef int each_loose_cruft_fn(const char *basename,
>  typedef int each_loose_subdir_fn(int nr,
>  				 const char *path,
>  				 void *data);
> +int for_each_file_in_obj_subdir(int subdir_nr,
> +				struct strbuf *path,
> +				each_loose_object_fn obj_cb,
> +				each_loose_cruft_fn cruft_cb,
> +				each_loose_subdir_fn subdir_cb,
> +				void *data);

Now that this is becoming public, I think we need to document what should be in "path" here. It is the complete path, including the 2-hex subdir.

That's redundant with "subdir_nr". Should we push that logic down into the function, and basically do:

  if (subdir_nr < 0 || subdir_nr > 256)
	BUG("object subdir number out of range");
  orig_len = path->len;
  strbuf_addf(path, "/%02x", subdir_nr);
  baselen = path->len;

...

  strbuf_setlen(path, orig_len);

That's one less thing for the caller to worry about, and it's easy to explain the argument as "the path to the object directory root".

Show 7 quoted lines

> +		if (!alt->loose_objects_subdir_seen[subdir_nr]) {
> +			struct strbuf *buf = alt_scratch_buf(alt);
> +			strbuf_addf(buf, "%02x/", subdir_nr);
> +			for_each_file_in_obj_subdir(subdir_nr, buf,
> +						    append_loose_object,
> +						    NULL, NULL,
> +						    &alt->loose_objects_cache);

I think the trailing slash here is superfluous. If you take my suggestion above, that line goes away. But then the front slash it adds becomes superfluous. It should probably just do:

  strbuf_complete(path, '/');
  strbuf_addf(path, "%02x", subdir_nr);

-Peff