From: Junio C Hamano <gitster@pobox.com>
To: "René Scharfe" <rene.scharfe@lsrfire.ath.cx>
Cc: Marco Costalba <mcostalba@gmail.com>,
Git Mailing List <git@vger.kernel.org>,
Johannes Schindelin <Johannes.Schindelin@gmx.de>
Subject: Re: [PATCH] Avoid a useless prefix lookup in strbuf_expand()
Date: Thu, 03 Jan 2008 01:08:56 -0800 [thread overview]
Message-ID: <7vr6gztgwn.fsf@gitster.siamese.dyndns.org> (raw)
In-Reply-To: <477C301D.7060509@lsrfire.ath.cx> (René Scharfe's message of "Thu, 03 Jan 2008 01:45:17 +0100")
René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> The loop makes implementing the callback function a bit easier, since
> you don't need to cover all cases; the input is already checked by
> strbuf_expand().
>
> Anyway, here's your patch again with a few small changes: the
> placeholders array is gone as you suggested, the cases for %Cx, %ax and
> %cx are check for unknown placeholders and the callback function returns
> the number of bytes it consumed as size_t.
>
> All in all: less code, slightly more complex callback functions (needs
> to return the length of the consumed placeholder or 0 if the input
> doesn't match a placeholder) and increased speed. I have to admit that
> I start to like it. :-)
I'll let Marco bench it and hopefully Ack with an updated
(final) commit log message.
I think Dscho and Marco's earlier prefixcmp() optimization to
avoid strlen() can stay, but with "inline" removed. That should
be equivalent to the version before the optimization, both from
the point of view of the code footprint and callchain length,
but still avoid strlen() cost.
Due to lack of better place the patch below moves it to strbuf.c
which probably is the closest collection of "stringy" stuff.
$ size git ;# with the attached patch
text data bss dec hex filename
731144 13456 263464 1008064 f61c0 git
$ size ../git.build/git ;# before Dscho's patch
text data bss dec hex filename
731272 13456 263464 1008192 f6240 ../git.build/git
$ size ~/bin/git ;# with Dscho's patch
text data bss dec hex filename
740736 13456 263464 1017656 f8738 /home/junio/bin/git
You earlier said 2,620,938 vs 2,640,450 and I think you meant
what "ls -l" reports. I suspect it is not a very good measure,
but the numbers here are:
$ ls -l git ;# with the attached patch
-rwxrwxr-x 83 junio src 3345237 2008-01-03 00:53 git
$ ls -l ../git.build/git ;# before Dscho's patch
-rwxrwxr-x 83 junio src 3364803 2008-01-03 01:01 ../git.build/git
$ ls -l ~/bin/git ;# with Dscho's patch
-rwxr-xr-x 83 junio src 3389299 2008-01-02 15:18 /home/junio/bin/git
-- >8 --
Uninline prefixcmp()
Now the routine is an open-coded loop that avoids an extra
strlen() in the previous implementation, it got a bit too big to
be inlined. Uninlining it makes code footprint smaller but the
result still retains the avoidance of strlen() cost.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
git-compat-util.h | 11 ++---------
strbuf.c | 9 +++++++++
2 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/git-compat-util.h b/git-compat-util.h
index 7059cbd..b6ef544 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -122,6 +122,8 @@ extern void set_die_routine(void (*routine)(const char *err, va_list params) NOR
extern void set_error_routine(void (*routine)(const char *err, va_list params));
extern void set_warn_routine(void (*routine)(const char *warn, va_list params));
+extern int prefixcmp(const char *str, const char *prefix);
+
#ifdef NO_MMAP
#ifndef PROT_READ
@@ -396,15 +398,6 @@ static inline int sane_case(int x, int high)
return x;
}
-static inline int prefixcmp(const char *str, const char *prefix)
-{
- for (; ; str++, prefix++)
- if (!*prefix)
- return 0;
- else if (*str != *prefix)
- return (unsigned char)*prefix - (unsigned char)*str;
-}
-
static inline int strtoul_ui(char const *s, int base, unsigned int *result)
{
unsigned long ul;
diff --git a/strbuf.c b/strbuf.c
index b9b194b..5efcfc8 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -1,5 +1,14 @@
#include "cache.h"
+int prefixcmp(const char *str, const char *prefix)
+{
+ for (; ; str++, prefix++)
+ if (!*prefix)
+ return 0;
+ else if (*str != *prefix)
+ return (unsigned char)*prefix - (unsigned char)*str;
+}
+
/*
* Used as the default ->buf value, so that people can always assume
* buf is non NULL and ->buf is NUL terminated even for a freshly
next prev parent reply other threads:[~2008-01-03 9:09 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-12-30 13:46 [PATCH] Avoid a useless prefix lookup in strbuf_expand() Marco Costalba
2008-01-02 18:11 ` René Scharfe
[not found] ` <e5bfff550801021027i6d6a399cob96ae3c840661884@mail.gmail.com>
2008-01-03 0:45 ` René Scharfe
2008-01-03 9:08 ` Junio C Hamano [this message]
2008-01-03 10:06 ` Marco Costalba
-- strict thread matches above, loose matches on Subject: below --
2008-01-06 0:10 Marco Costalba
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7vr6gztgwn.fsf@gitster.siamese.dyndns.org \
--to=gitster@pobox.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=git@vger.kernel.org \
--cc=mcostalba@gmail.com \
--cc=rene.scharfe@lsrfire.ath.cx \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).