refs: write packed_refs file using stdio

We write each line of a new packed-refs file individually
using a write() syscall (and sometimes 2, if the ref is
peeled). Since each line is only about 50-100 bytes long,
this creates a lot of system call overhead.

We can instead open a stdio handle around our descriptor and
use fprintf to write to it. The extra buffering is not a
problem for us, because nobody will read our new packed-refs
file until we call commit_lock_file (by which point we have
flushed everything).

On a pathological repository with 8.5 million refs, this
dropped the time to run `git pack-refs` from 20s to 6s.

Signed-off-by: Jeff King <peff@peff.net>
Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/cache.h b/cache.h
index 4d5b76c76ab2275f51c504cac62cda7e563e1d1d..bc286ce22db4c921caeadb8c33e81102ee7d30a2 100644 (file)
--- a/cache.h
+++ b/cache.h
@@ -1395,6 +1395,8 @@ extern const char *git_mailmap_blob;
 
 /* IO helper functions */
 extern void maybe_flush_or_die(FILE *, const char *);
 
 /* IO helper functions */
 extern void maybe_flush_or_die(FILE *, const char *);

+__attribute__((format (printf, 2, 3)))
+extern void fprintf_or_die(FILE *, const char *fmt, ...);

 extern int copy_fd(int ifd, int ofd);
 extern int copy_file(const char *dst, const char *src, int mode);
 extern int copy_file_with_time(const char *dst, const char *src, int mode);
 extern int copy_fd(int ifd, int ofd);
 extern int copy_file(const char *dst, const char *src, int mode);
 extern int copy_file_with_time(const char *dst, const char *src, int mode);

diff --git a/refs.c b/refs.c
index 27927f2319130cc0575817542dfd47c37cc5149b..f08faeda23657dca278e9f032c14ab03dcb537ac 100644 (file)
--- a/refs.c
+++ b/refs.c
@@ -2191,25 +2191,12 @@ struct ref_lock *lock_any_ref_for_update(const char *refname,
  * Write an entry to the packed-refs file for the specified refname.
  * If peeled is non-NULL, write it as the entry's peeled value.
  */
  * Write an entry to the packed-refs file for the specified refname.
  * If peeled is non-NULL, write it as the entry's peeled value.
  */

-static void write_packed_entry(int fd, char *refname, unsigned char *sha1,
+static void write_packed_entry(FILE *fh, char *refname, unsigned char *sha1,

                               unsigned char *peeled)
 {
                               unsigned char *peeled)
 {

-       char line[PATH_MAX + 100];
-       int len;
-
-       len = snprintf(line, sizeof(line), "%s %s\n",
-                      sha1_to_hex(sha1), refname);
-       /* this should not happen but just being defensive */
-       if (len > sizeof(line))
-               die("too long a refname '%s'", refname);
-       write_or_die(fd, line, len);
-
-       if (peeled) {
-               if (snprintf(line, sizeof(line), "^%s\n",
-                            sha1_to_hex(peeled)) != PEELED_LINE_LENGTH)
-                       die("internal error");
-               write_or_die(fd, line, PEELED_LINE_LENGTH);
-       }
+       fprintf_or_die(fh, "%s %s\n", sha1_to_hex(sha1), refname);
+       if (peeled)
+               fprintf_or_die(fh, "^%s\n", sha1_to_hex(peeled));

 }
 
 /*
 }
 
 /*


@@ -2217,13 +2204,12 @@ static void write_packed_entry(int fd, char *refname, unsigned char *sha1,
  */
 static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data)
 {
  */
 static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data)
 {

-       int *fd = cb_data;

        enum peel_status peel_status = peel_entry(entry, 0);
 
        if (peel_status != PEEL_PEELED && peel_status != PEEL_NON_TAG)
                error("internal error: %s is not a valid packed reference!",
                      entry->name);
        enum peel_status peel_status = peel_entry(entry, 0);
 
        if (peel_status != PEEL_PEELED && peel_status != PEEL_NON_TAG)
                error("internal error: %s is not a valid packed reference!",
                      entry->name);

-       write_packed_entry(*fd, entry->name, entry->u.value.sha1,
+       write_packed_entry(cb_data, entry->name, entry->u.value.sha1,

                           peel_status == PEEL_PEELED ?
                           entry->u.value.peeled : NULL);
        return 0;
                           peel_status == PEEL_PEELED ?
                           entry->u.value.peeled : NULL);
        return 0;


@@ -2259,15 +2245,22 @@ int commit_packed_refs(void)
                get_packed_ref_cache(&ref_cache);
        int error = 0;
        int save_errno = 0;
                get_packed_ref_cache(&ref_cache);
        int error = 0;
        int save_errno = 0;

+       FILE *out;

 
        if (!packed_ref_cache->lock)
                die("internal error: packed-refs not locked");
 
        if (!packed_ref_cache->lock)
                die("internal error: packed-refs not locked");

-       write_or_die(packed_ref_cache->lock->fd,
-                    PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER));

 
 

+       out = fdopen(packed_ref_cache->lock->fd, "w");
+       if (!out)
+               die_errno("unable to fdopen packed-refs descriptor");
+
+       fprintf_or_die(out, "%s", PACKED_REFS_HEADER);

        do_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),
        do_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),

-                                0, write_packed_entry_fn,
-                                &packed_ref_cache->lock->fd);
+                                0, write_packed_entry_fn, out);
+       if (fclose(out))
+               die_errno("write error");
+       packed_ref_cache->lock->fd = -1;
+

        if (commit_lock_file(packed_ref_cache->lock)) {
                save_errno = errno;
                error = -1;
        if (commit_lock_file(packed_ref_cache->lock)) {
                save_errno = errno;
                error = -1;

diff --git a/write_or_die.c b/write_or_die.c
index b50f99a9361926d2116c85e3e90a132fb9dab742..e7afe7a295e586d58313e82e87b22de347aef7a1 100644 (file)
--- a/write_or_die.c
+++ b/write_or_die.c
@@ -49,6 +49,21 @@ void maybe_flush_or_die(FILE *f, const char *desc)
        }
 }
 
        }
 }
 

+void fprintf_or_die(FILE *f, const char *fmt, ...)
+{
+       va_list ap;
+       int ret;
+
+       va_start(ap, fmt);
+       ret = vfprintf(f, fmt, ap);
+       va_end(ap);
+
+       if (ret < 0) {
+               check_pipe(errno);
+               die_errno("write error");
+       }
+}
+

 void fsync_or_die(int fd, const char *msg)
 {
        if (fsync(fd) < 0) {
 void fsync_or_die(int fd, const char *msg)
 {
        if (fsync(fd) < 0) {