Re: [PATCH 1/5] decorate: allow storing values instead of pointers

From: Jeff King <peff@peff.net>
Date: Jul 11, 2011, 21:20 UTC
Message-ID: <20110711212032.GB30155@sigill.intra.peff.net>
In-Reply-To: <7vr55wd1hw.fsf@alter.siamese.dyndns.org>

On Mon, Jul 11, 2011 at 12:06:08PM -0700, Junio C Hamano wrote:

Show 25 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> >  static void grow_decoration(struct decoration *n)
> >  {
> > -	int i;
> > -	int old_size = n->size;
> > -	struct object_decoration *old_hash = n->hash;
> > +	unsigned char *old_hash = n->hash;
> > +	unsigned char *old_end = n->end;
> > +	unsigned char *p;
> >  
> > -	n->size = (old_size + 1000) * 3 / 2;
> > -	n->hash = xcalloc(n->size, sizeof(struct object_decoration));
> > +	n->stride = sizeof(struct object_decoration) + WIDTH(n);
> 
> This value should not change once it is initialized, or all h*ll breaks
> loose while accessing the old-hash, right?  Just wondering if it makes the
> intention clearer if the function had something like this in it:
> 
> 	if (!old_size) {
> 		/* initial */
>                 n->stride = ...
> 	} else {
> 		/* rehash to grow */
> 	}

Yes, things will break horribly if you ever change the width. I wonder if we can get some language support with:

  struct decoration {
     ...
     const int width;
     ...
  };

which would require that calling code set the width during initialization. Doing anything else is more or less insane, although it does make trouble for the object-cache code, which uses a run-time function to initialize itself. But as it's somewhat tightly coupled with decorations and knows what it's doing, I would not be opposed to casting away the constness in that one spot with a comment as a workaround.

Show 9 quoted lines

> I am mostly worried about both width and stride being assignable
> fields. An alternative may be to expose
> 
> 	int decoration_stride(struct decoration *n)
>         {
>         	return sizeof(struct object_decoration) + WIDTH(n);
> 	}
> 
> to the outside callers and drop "stride" field.

Yeah, I considered that (the "end" field is calculable, too). I just didn't want the overhead of:

  for (p = d->hash; p < decoration_end(d); p += decoration_stride(d))
     ...

But I suppose we can inline both of those and let the optimizer deal with hoisting the actual calculations them out of the loop. Or maybe I'm just being stupid and optimizing too early. Speed is definitely one of my criteria for this series, though.

-Peff