On Mon, Jul 11, 2011 at 12:06:08PM -0700, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> 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.
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html