Re: [PATCH v8 1/4] sha1_file.c: support reading from a loose object of unknown type

From

karthik nayak <karthik.188@gmail.com>

Date

Apr 17, 2015, 18:45 UTC

Message-ID

<553154C8.5090001@gmail.com>

In-Reply-To

<20150417142310.GA12479@peff.net>

On 04/17/2015 07:53 PM, Jeff King wrote:

> On Wed, Apr 15, 2015 at 06:18:24PM -0400, Jeff King wrote:
>
> >> This _might_ have some performance impact in that strbuf_addch()
> >> involves strbuf_grow(*, 1), which does "does it overflow to
> >> increment sb->len by one?"; I would say it should be unmeasurable
> >> because the function is expected to be used only on loose objects
> >> and you shouldn't have very many of them without packing in your
> >> repository in the first place.
> >>
> >> I guess Peff's c1822d4f (strbuf: add an optimized 1-character
> >> strbuf_grow, 2015-04-04) may want to teach strbuf_addch() to use his
> >> new strbuf_grow_ch(), and once that happens the performance worry
> >> would disappear without this code to be changed at all.
> >
> > I haven't re-rolled that series yet, but the discussion there showed
> > that strbuf_grow_ch() is unnecessary; call-sites can just check:
> >
> >    if (!strbuf_avail(sb))
> >     strbuf_grow(sb, 1);
> >
> > to get the fast inline check. Since we go to the trouble to inline
> > strbuf_addch, we should probably teach it the same trick. It would be
> > nice to identify a place with a tight strbuf_addch() loop that could
> > demonstrate the speed increase, though.
>
> Just for reference, I did teach strbuf_addch this trick. And the config
> code is the tight loop to test it with. Results are here:
>
>    http://article.gmane.org/gmane.comp.version-control.git/267266
>
> In this code path, we are typically only seeing short strings (the
> original code used a 10-byte static buffer). So I doubt it makes all
> that much difference.
>
> If there _is_ a performance implication to worry about here, I think it
> would be that we are doing an extra malloc/free. I'm not sure I
> understand why we are copying it at all. The original code copied from
> the hdr into type[10] so that we could NUL-terminate it, which was
> required for type_from_string().
>
> But now we use type_from_string_gently, which can accept a length[1]. So
> we could just count the bytes to the first space and pass the original
> buffer along with that length, no?

Something like this perhaps :

@@ -1614,27 +1642,40 @@ static void *unpack_sha1_rest(git_zstream
*stream, void *buffer, unsigned long s
    * too permissive for what we want to check. So do an anal
    * object header parse by hand.
    */
-int parse_sha1_header(const char *hdr, unsigned long *sizep)
+static int parse_sha1_header_extended(const char *hdr, struct
object_info *oi,
+                              unsigned int flags)
   {
-       char type[10];
-       int i;
+       const char *buf = hdr;
          unsigned long size;
+       int type, len = 0;

          /*
-        * The type can be at most ten bytes (including the
-        * terminating '\0' that we add), and is followed by
+        * The type can be of any size but is followed by
           * a space.
           */
-       i = 0;
          for (;;) {
                  char c = *hdr++;
                  if (c == ' ')
                          break;
-               type[i++] = c;
-               if (i >= sizeof(type))
-                       return -1;
+               len++;
          }
-       type[i] = 0;
+
+       type = type_from_string_gently(buf, len, 1);
+       if (oi->typename) {
+               strbuf_add(oi->typename, buf, len);
+               strbuf_addch(oi->typename, '\0');
+       }
+       /*
+        * Set type to 0 if its an unknown object and
+        * we're obtaining the type using '--literally'
+        * option.
+        */
+       if ((flags & LOOKUP_LITERALLY) && (type == -1))
+               type = 0;
+       else if (type == -1)
+               die("invalid object type");
+       if (oi->typep)
+               *oi->typep = type;

          /*
           * The length must follow immediately, and be in canonical

>
> -Peff
>
> [1] Not related to your patch, but it looks like type_from_string_gently
>      is overly lax. It does a strncmp() with the length of the candidate
>      name, but does not check that we consumed all of the matching name.
>      So "tr" would match "tree", "comm" would match "commit", and so
>      forth.
>
Thanks for the patch re-roll on strbuf_addch() and the patch on
type_from_string_gently().