trace: improve trace performance

The trace API currently rechecks the environment variable and reopens the
trace file on every API call. This has the ugly side effect that errors
(e.g. file cannot be opened, or the user specified a relative path) are
also reported on every call. Performance can be improved by about factor
three by remembering the environment state and keeping the file open.

Replace the 'const char *key' parameter in the API with a pointer to a
'struct trace_key' that bundles the environment variable name with
additional, trace-internal state. Change the call sites of these APIs to
use a static 'struct trace_key' instead of a string constant.

In trace.c::get_trace_fd(), save and reuse the file descriptor in 'struct
trace_key'.

Add a 'trace_disable()' API, so that packet_trace() can cleanly disable
tracing when it encounters packed data (instead of using unsetenv()).

Signed-off-by: Karsten Blees <blees@dcon.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c3230817db4a7676eb74335b254f30597e66edd9..145105068999634a43d172b31d5a6c8dc7d12572 100644 (file)
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -438,7 +438,7 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
        uint32_t mask = 1 << (cmd->index % 32);
        int i;
 
-       trace_printf_key("GIT_TRACE_SHALLOW",
+       trace_printf_key(&trace_shallow,
                         "shallow: update_shallow_ref %s\n", cmd->ref_name);
        for (i = 0; i < si->shallow->nr; i++)
                if (si->used_shallow[i] &&

diff --git a/commit.h b/commit.h
index a9f177ba488a7002409eb2cdee1f7e9c3673529f..08ef643f20cc6b449e0a1f73a27c26a4dbb03dfc 100644 (file)
--- a/commit.h
+++ b/commit.h
@@ -235,6 +235,7 @@ extern void assign_shallow_commits_to_refs(struct shallow_info *info,
                                           int *ref_status);
 extern int delayed_reachability_test(struct shallow_info *si, int c);
 extern void prune_shallow(int show_only);
+extern struct trace_key trace_shallow;
 
 int is_descendant_of(struct commit *, struct commit_list *);
 int in_merge_bases(struct commit *, struct commit *);

diff --git a/pkt-line.c b/pkt-line.c
index bc63b3b80e5b2853f920ba66fe977c7cc0921c78..8bc89b1e0c0206647a8f5ee8da7727f304d14b2f 100644 (file)
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -3,7 +3,7 @@
 
 char packet_buffer[LARGE_PACKET_MAX];
 static const char *packet_trace_prefix = "git";
-static const char trace_key[] = "GIT_TRACE_PACKET";
+static struct trace_key trace_packet = TRACE_KEY_INIT(PACKET);
 
 void packet_trace_identity(const char *prog)
 {
@@ -15,7 +15,7 @@ static void packet_trace(const char *buf, unsigned int len, int write)
        int i;
        struct strbuf out;
 
-       if (!trace_want(trace_key))
+       if (!trace_want(&trace_packet))
                return;
 
        /* +32 is just a guess for header + quoting */
@@ -27,7 +27,7 @@ static void packet_trace(const char *buf, unsigned int len, int write)
        if ((len >= 4 && starts_with(buf, "PACK")) ||
            (len >= 5 && starts_with(buf+1, "PACK"))) {
                strbuf_addstr(&out, "PACK ...");
-               unsetenv(trace_key);
+               trace_disable(&trace_packet);
        }
        else {
                /* XXX we should really handle printable utf8 */
@@ -43,7 +43,7 @@ static void packet_trace(const char *buf, unsigned int len, int write)
        }
 
        strbuf_addch(&out, '\n');
-       trace_strbuf(trace_key, &out);
+       trace_strbuf(&trace_packet, &out);
        strbuf_release(&out);
 }
 

diff --git a/shallow.c b/shallow.c
index 0b267b64117c5f1d2df662e3d413418d575a94bd..de07709e3358947062984970e85005ded224baf9 100644 (file)
--- a/shallow.c
+++ b/shallow.c
@@ -325,7 +325,7 @@ void prune_shallow(int show_only)
        strbuf_release(&sb);
 }
 
-#define TRACE_KEY "GIT_TRACE_SHALLOW"
+struct trace_key trace_shallow = TRACE_KEY_INIT(SHALLOW);
 
 /*
  * Step 1, split sender shallow commits into "ours" and "theirs"
@@ -334,7 +334,7 @@ void prune_shallow(int show_only)
 void prepare_shallow_info(struct shallow_info *info, struct sha1_array *sa)
 {
        int i;
-       trace_printf_key(TRACE_KEY, "shallow: prepare_shallow_info\n");
+       trace_printf_key(&trace_shallow, "shallow: prepare_shallow_info\n");
        memset(info, 0, sizeof(*info));
        info->shallow = sa;
        if (!sa)
@@ -365,7 +365,7 @@ void remove_nonexistent_theirs_shallow(struct shallow_info *info)
 {
        unsigned char (*sha1)[20] = info->shallow->sha1;
        int i, dst;
-       trace_printf_key(TRACE_KEY, "shallow: remove_nonexistent_theirs_shallow\n");
+       trace_printf_key(&trace_shallow, "shallow: remove_nonexistent_theirs_shallow\n");
        for (i = dst = 0; i < info->nr_theirs; i++) {
                if (i != dst)
                        info->theirs[dst] = info->theirs[i];
@@ -516,7 +516,7 @@ void assign_shallow_commits_to_refs(struct shallow_info *info,
        int *shallow, nr_shallow = 0;
        struct paint_info pi;
 
-       trace_printf_key(TRACE_KEY, "shallow: assign_shallow_commits_to_refs\n");
+       trace_printf_key(&trace_shallow, "shallow: assign_shallow_commits_to_refs\n");
        shallow = xmalloc(sizeof(*shallow) * (info->nr_ours + info->nr_theirs));
        for (i = 0; i < info->nr_ours; i++)
                shallow[nr_shallow++] = info->ours[i];
@@ -622,7 +622,7 @@ static void post_assign_shallow(struct shallow_info *info,
        int bitmap_nr = (info->ref->nr + 31) / 32;
        struct commit_array ca;
 
-       trace_printf_key(TRACE_KEY, "shallow: post_assign_shallow\n");
+       trace_printf_key(&trace_shallow, "shallow: post_assign_shallow\n");
        if (ref_status)
                memset(ref_status, 0, sizeof(*ref_status) * info->ref->nr);
 

diff --git a/trace.c b/trace.c
index 3e315587e084d7666ea4f9c7117dc4b3329d2188..8662b7980133eb900fde3bbb675e7a363f04cf1f 100644 (file)
--- a/trace.c
+++ b/trace.c
@@ -26,43 +26,66 @@
 #include "quote.h"
 
 /* Get a trace file descriptor from "key" env variable. */
-static int get_trace_fd(const char *key, int *need_close)
+static int get_trace_fd(struct trace_key *key)
 {
-       char *trace = getenv(key);
+       static struct trace_key trace_default = { "GIT_TRACE" };
+       const char *trace;
+
+       /* use default "GIT_TRACE" if NULL */
+       if (!key)
+               key = &trace_default;
+
+       /* don't open twice */
+       if (key->initialized)
+               return key->fd;
+
+       trace = getenv(key->key);
 
        if (!trace || !strcmp(trace, "") ||
            !strcmp(trace, "0") || !strcasecmp(trace, "false"))
-               return 0;
-       if (!strcmp(trace, "1") || !strcasecmp(trace, "true"))
-               return STDERR_FILENO;
-       if (strlen(trace) == 1 && isdigit(*trace))
-               return atoi(trace);
-       if (is_absolute_path(trace)) {
+               key->fd = 0;
+       else if (!strcmp(trace, "1") || !strcasecmp(trace, "true"))
+               key->fd = STDERR_FILENO;
+       else if (strlen(trace) == 1 && isdigit(*trace))
+               key->fd = atoi(trace);
+       else if (is_absolute_path(trace)) {
                int fd = open(trace, O_WRONLY | O_APPEND | O_CREAT, 0666);
                if (fd == -1) {
                        fprintf(stderr,
                                "Could not open '%s' for tracing: %s\n"
                                "Defaulting to tracing on stderr...\n",
                                trace, strerror(errno));
-                       return STDERR_FILENO;
+                       key->fd = STDERR_FILENO;
+               } else {
+                       key->fd = fd;
+                       key->need_close = 1;
                }
-               *need_close = 1;
-               return fd;
+       } else {
+               fprintf(stderr, "What does '%s' for %s mean?\n"
+                       "If you want to trace into a file, then please set "
+                       "%s to an absolute pathname (starting with /).\n"
+                       "Defaulting to tracing on stderr...\n",
+                       trace, key->key, key->key);
+               key->fd = STDERR_FILENO;
        }
 
-       fprintf(stderr, "What does '%s' for %s mean?\n", trace, key);
-       fprintf(stderr, "If you want to trace into a file, "
-               "then please set %s to an absolute pathname "
-               "(starting with /).\n", key);
-       fprintf(stderr, "Defaulting to tracing on stderr...\n");
+       key->initialized = 1;
+       return key->fd;
+}
 
-       return STDERR_FILENO;
+void trace_disable(struct trace_key *key)
+{
+       if (key->need_close)
+               close(key->fd);
+       key->fd = 0;
+       key->initialized = 1;
+       key->need_close = 0;
 }
 
 static const char err_msg[] = "Could not trace into fd given by "
        "GIT_TRACE environment variable";
 
-static void trace_vprintf(const char *key, const char *format, va_list ap)
+static void trace_vprintf(struct trace_key *key, const char *format, va_list ap)
 {
        struct strbuf buf = STRBUF_INIT;
 
@@ -75,7 +98,7 @@ static void trace_vprintf(const char *key, const char *format, va_list ap)
        strbuf_release(&buf);
 }
 
-void trace_printf_key(const char *key, const char *format, ...)
+void trace_printf_key(struct trace_key *key, const char *format, ...)
 {
        va_list ap;
        va_start(ap, format);
@@ -87,31 +110,24 @@ void trace_printf(const char *format, ...)
 {
        va_list ap;
        va_start(ap, format);
-       trace_vprintf("GIT_TRACE", format, ap);
+       trace_vprintf(NULL, format, ap);
        va_end(ap);
 }
 
-void trace_strbuf(const char *key, const struct strbuf *buf)
+void trace_strbuf(struct trace_key *key, const struct strbuf *buf)
 {
-       int fd, need_close = 0;
-
-       fd = get_trace_fd(key, &need_close);
+       int fd = get_trace_fd(key);
        if (!fd)
                return;
 
        write_or_whine_pipe(fd, buf->buf, buf->len, err_msg);
-
-       if (need_close)
-               close(fd);
 }
 
 void trace_argv_printf(const char **argv, const char *format, ...)
 {
        struct strbuf buf = STRBUF_INIT;
        va_list ap;
-       int fd, need_close = 0;
-
-       fd = get_trace_fd("GIT_TRACE", &need_close);
+       int fd = get_trace_fd(NULL);
        if (!fd)
                return;
 
@@ -124,9 +140,6 @@ void trace_argv_printf(const char **argv, const char *format, ...)
        strbuf_addch(&buf, '\n');
        write_or_whine_pipe(fd, buf.buf, buf.len, err_msg);
        strbuf_release(&buf);
-
-       if (need_close)
-               close(fd);
 }
 
 static const char *quote_crnl(const char *path)
@@ -155,11 +168,11 @@ static const char *quote_crnl(const char *path)
 /* FIXME: move prefix to startup_info struct and get rid of this arg */
 void trace_repo_setup(const char *prefix)
 {
-       static const char *key = "GIT_TRACE_SETUP";
+       static struct trace_key key = TRACE_KEY_INIT(SETUP);
        const char *git_work_tree;
        char cwd[PATH_MAX];
 
-       if (!trace_want(key))
+       if (!trace_want(&key))
                return;
 
        if (!getcwd(cwd, PATH_MAX))
@@ -171,18 +184,13 @@ void trace_repo_setup(const char *prefix)
        if (!prefix)
                prefix = "(null)";
 
-       trace_printf_key(key, "setup: git_dir: %s\n", quote_crnl(get_git_dir()));
-       trace_printf_key(key, "setup: worktree: %s\n", quote_crnl(git_work_tree));
-       trace_printf_key(key, "setup: cwd: %s\n", quote_crnl(cwd));
-       trace_printf_key(key, "setup: prefix: %s\n", quote_crnl(prefix));
+       trace_printf_key(&key, "setup: git_dir: %s\n", quote_crnl(get_git_dir()));
+       trace_printf_key(&key, "setup: worktree: %s\n", quote_crnl(git_work_tree));
+       trace_printf_key(&key, "setup: cwd: %s\n", quote_crnl(cwd));
+       trace_printf_key(&key, "setup: prefix: %s\n", quote_crnl(prefix));
 }
 
-int trace_want(const char *key)
+int trace_want(struct trace_key *key)
 {
-       const char *trace = getenv(key);
-
-       if (!trace || !strcmp(trace, "") ||
-           !strcmp(trace, "0") || !strcasecmp(trace, "false"))
-               return 0;
-       return 1;
+       return !!get_trace_fd(key);
 }

diff --git a/trace.h b/trace.h
index 8fea50bf1dbf9d4d67f1fba630d3ff037e6d29da..d85ac4c6529ea1895a37847a009e17cbccf12735 100644 (file)
--- a/trace.h
+++ b/trace.h
@@ -4,14 +4,24 @@
 #include "git-compat-util.h"
 #include "strbuf.h"
 
+struct trace_key {
+       const char * const key;
+       int fd;
+       unsigned int initialized : 1;
+       unsigned int  need_close : 1;
+};
+
+#define TRACE_KEY_INIT(name) { "GIT_TRACE_" #name, 0, 0, 0 }
+
 __attribute__((format (printf, 1, 2)))
 extern void trace_printf(const char *format, ...);
 __attribute__((format (printf, 2, 3)))
 extern void trace_argv_printf(const char **argv, const char *format, ...);
 extern void trace_repo_setup(const char *prefix);
-extern int trace_want(const char *key);
+extern int trace_want(struct trace_key *key);
+extern void trace_disable(struct trace_key *key);
 __attribute__((format (printf, 2, 3)))
-extern void trace_printf_key(const char *key, const char *format, ...);
-extern void trace_strbuf(const char *key, const struct strbuf *buf);
+extern void trace_printf_key(struct trace_key *key, const char *format, ...);
+extern void trace_strbuf(struct trace_key *key, const struct strbuf *buf);
 
 #endif /* TRACE_H */