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?
Yes, we could, that would eliminate "struct strbuf typename =
STRBUF_INIT".
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().
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
