Re: [PATCH 2/2] lookup_commit_reference_gently: do not read non-{tag,commit}

From: Thomas Rast <trast@inf.ethz.ch>
Date: May 31, 2013, 08:16 UTC
Message-ID: <87fvx37edv.fsf@linux-k42r.v.cablecom.net>
In-Reply-To: <CALkWK0nL4hPio74Hm+ctObNNFg9+=9brKXFK2ymGB=sPTAk1Hg@mail.gmail.com>

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Show 7 quoted lines

> Thomas Rast wrote:
>> +       struct object *obj;
>> +       int type = sha1_object_info(sha1, NULL);
>> +       /* If it's neither tag nor commit, parsing the object is wasted effort */
>> +       if (type != OBJ_TAG && type != OBJ_COMMIT)
>> +               return NULL;
>> +       obj = deref_tag(parse_object(sha1), NULL, 0);

[...]

Show 10 quoted lines

> In contrast, parse_object() first calls lookup_object() to look it up
> in some hashtable to get the type -- the packfile idx, presumably?
> Why don't you also do that instead of sha1_object_info()?  Or, why
> don't you wrap parse_object() in an API that doesn't go beyond the
> first blob check (and not execute parse_object_buffer())?
>
> Also, does this patch fix the bug Alex reported?
>
> Apologies if I've misunderstood something horribly (which does seem to
> be the case).

Yes, it does fix the bug.  (It's not really buggy, just slow.)

However, you implicitly point out an important point: If we have the object, and it was already parsed (obj->parsed is set), parse_object() is essentially free. But sha1_object_info is not, it will in particular unconditionally dig through long delta chains to discover the base type of an object that has already been unpacked.

As for your original questions: lookup_object() is "do we have it in our big object hashtable?" -- the one that holds many[1] objects, that Peff recently sped up.

sha1_object_info() and read_object() are in many ways parallel functions that do approximately the following:

  check all pack indexes for this object
  if we found a hit:
    attempt to unpack by recursively going through deltas
    (for _info, no need to unpack, but we still go through the delta
    chain because the type of object is determined by the innermost
    delta base)
  try to load it as a loose object
  it could have been repacked and pruned while we were looking, so:
    reload pack index information
    try the packs again (search indexes, then unpack)
  complain

[1] blobs in particular are frequently not stored in that hash table, because it is an insert-only table

-- 
Thomas Rast
trast@{inf,student}.ethz.ch