Re: [RFC/PATCHv2 2/6] add metadata-cache infrastructure

From: Jeff King <peff@peff.net>
Date: Jul 13, 2011, 20:25 UTC
Message-ID: <20110713202536.GE31965@sigill.intra.peff.net>
In-Reply-To: <7vipr66kmz.fsf@alter.siamese.dyndns.org>

On Wed, Jul 13, 2011 at 12:33:24PM -0700, Junio C Hamano wrote:

Show 8 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > +`metadata_cache_lookup_uint32`::
> > +`metadata_cache_add_uint32`::
> 
> I think these are "uint31" functions, as you cannot signal missing entry
> by returning a value with the MSB set if higher-end of uint32 range can be
> a valid value.

No, look at their definitions. I am careful not to assume we have a sentinel value for "not found", and you must use them like:

  uint32_t value;
  if (metadata_cache_lookup_uint32(c, obj, &value))
          printf("value is %"PRIu32", value);
  else
          printf("value is not there");

Show 15 quoted lines

> > +	if (c->validity_fun) {
> > +		c->validity_fun(validity);
> > +		if (hashcmp(validity, p))
> > +			return NULL;
> > +	}
> 
> Two comments.
> 
>  - I would have expected that c->validity_check() would be a way for a
>    caller to implement a boolean function to check the validity of the
>    cache, with another hook c->validity_token() to generate/update the
>    token. I could then use the 20-byte space to store a timestamp and
>    check can say "It was still 3-days ago? fresh enough", or something
>    like that. But this is not a complaint--such a scheme I wrote in the
>    above four lines may be _too_ flexible to be useful.

I started with exactly that interface, and came to the conclusion that it was unneeded flexibility. I can switch it back, but there's really not a need to at this point. The fact that there are 20 opaque bytes in the file is what will live with us from version to version. But if new code wants to be more flexible about how it checks the bytes, that is something that can be changed easily when the new code is added.

>  - I wonder if validity_fn() callback wants a callback parameter (the
>    pointer "c" itself, after adding an extra field to metadata_cache that
>    stores the callback parameter pointer of type "void *" and adding a
>    parameter to METADATA_CACHE_INIT() macro to initialize it).

Neither this use nor my proposed patch-id cache would have a need for it, so I didn't bother. And like above, it can be easily changed later.

> Other than that, this is looking fun ;-)

Thanks. I'm pleased with the performance numbers I'm getting. I'm still a bit iffy on the alignment and aliasing issues in the first two patches.

-Peff