{"thread":{"id":"37529","subject":"[PATCH] refs: write packed_refs file using stdio","startedAt":"2014-09-10T10:03:52Z","lastAt":"2014-09-10T19:14:54Z","messageCount":4,"participants":["Jeff King","Michael Haggerty"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"249113","messageId":"20140910100352.GA12164@peff.net","threadId":"37529","inReplyTo":null,"subject":"[PATCH] refs: write packed_refs file using stdio","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-10T10:03:52Z","receivedAt":"2014-09-10T10:03:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We write each line of a new packed-refs file individually\nusing a write() syscall (and sometimes 2, if the ref is\npeeled). Since each line is only about 50-100 bytes long,\nthis creates a lot of system call overhead.\n\nWe can instead open a stdio handle around our descriptor and\nuse fprintf to write to it. The extra buffering is not a\nproblem for us, because nobody will read our new packed-refs\nfile until we call commit_lock_file (by which point we have\nflushed everything).\n\nOn a pathological repository with 8.5 million refs, this\ndropped the time to run `git pack-refs` from 20s to 6s.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nObviously that repo is ridiculous (but a sad reality for me).\n\nHowever, I think the benefits extend to smaller files, too. And it's\npretty easy to do (and I actually think the resulting write_packed_entry\nis a lot easier to read, as well as lifting some arbitrary limits).\n\n cache.h        |  2 ++\n refs.c         | 39 ++++++++++++++++-----------------------\n write_or_die.c | 15 +++++++++++++++\n 3 files changed, 33 insertions(+), 23 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 4d5b76c..bc286ce 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1395,6 +1395,8 @@ extern const char *git_mailmap_blob;\n \n /* IO helper functions */\n extern void maybe_flush_or_die(FILE *, const char *);\n+__attribute__((format (printf, 2, 3)))\n+extern void fprintf_or_die(FILE *, const char *fmt, ...);\n extern int copy_fd(int ifd, int ofd);\n extern int copy_file(const char *dst, const char *src, int mode);\n extern int copy_file_with_time(const char *dst, const char *src, int mode);\ndiff --git a/refs.c b/refs.c\nindex 27927f2..f08faed 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2191,25 +2191,12 @@ struct ref_lock *lock_any_ref_for_update(const char *refname,\n  * Write an entry to the packed-refs file for the specified refname.\n  * If peeled is non-NULL, write it as the entry's peeled value.\n  */\n-static void write_packed_entry(int fd, char *refname, unsigned char *sha1,\n+static void write_packed_entry(FILE *fh, char *refname, unsigned char *sha1,\n \t\t\t       unsigned char *peeled)\n {\n-\tchar line[PATH_MAX + 100];\n-\tint len;\n-\n-\tlen = snprintf(line, sizeof(line), \"%s %s\\n\",\n-\t\t       sha1_to_hex(sha1), refname);\n-\t/* this should not happen but just being defensive */\n-\tif (len > sizeof(line))\n-\t\tdie(\"too long a refname '%s'\", refname);\n-\twrite_or_die(fd, line, len);\n-\n-\tif (peeled) {\n-\t\tif (snprintf(line, sizeof(line), \"^%s\\n\",\n-\t\t\t     sha1_to_hex(peeled)) != PEELED_LINE_LENGTH)\n-\t\t\tdie(\"internal error\");\n-\t\twrite_or_die(fd, line, PEELED_LINE_LENGTH);\n-\t}\n+\tfprintf_or_die(fh, \"%s %s\\n\", sha1_to_hex(sha1), refname);\n+\tif (peeled)\n+\t\tfprintf_or_die(fh, \"^%s\\n\", sha1_to_hex(peeled));\n }\n \n /*\n@@ -2217,13 +2204,12 @@ static void write_packed_entry(int fd, char *refname, unsigned char *sha1,\n  */\n static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data)\n {\n-\tint *fd = cb_data;\n \tenum peel_status peel_status = peel_entry(entry, 0);\n \n \tif (peel_status != PEEL_PEELED && peel_status != PEEL_NON_TAG)\n \t\terror(\"internal error: %s is not a valid packed reference!\",\n \t\t      entry->name);\n-\twrite_packed_entry(*fd, entry->name, entry->u.value.sha1,\n+\twrite_packed_entry(cb_data, entry->name, entry->u.value.sha1,\n \t\t\t   peel_status == PEEL_PEELED ?\n \t\t\t   entry->u.value.peeled : NULL);\n \treturn 0;\n@@ -2259,15 +2245,22 @@ int commit_packed_refs(void)\n \t\tget_packed_ref_cache(&ref_cache);\n \tint error = 0;\n \tint save_errno = 0;\n+\tFILE *out;\n \n \tif (!packed_ref_cache->lock)\n \t\tdie(\"internal error: packed-refs not locked\");\n-\twrite_or_die(packed_ref_cache->lock->fd,\n-\t\t     PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER));\n \n+\tout = fdopen(packed_ref_cache->lock->fd, \"w\");\n+\tif (!out)\n+\t\tdie_errno(\"unable to fdopen packed-refs descriptor\");\n+\n+\tfprintf_or_die(out, \"%s\", PACKED_REFS_HEADER);\n \tdo_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),\n-\t\t\t\t 0, write_packed_entry_fn,\n-\t\t\t\t &packed_ref_cache->lock->fd);\n+\t\t\t\t 0, write_packed_entry_fn, out);\n+\tif (fclose(out))\n+\t\tdie_errno(\"write error\");\n+\tpacked_ref_cache->lock->fd = -1;\n+\n \tif (commit_lock_file(packed_ref_cache->lock)) {\n \t\tsave_errno = errno;\n \t\terror = -1;\ndiff --git a/write_or_die.c b/write_or_die.c\nindex b50f99a..e7afe7a 100644\n--- a/write_or_die.c\n+++ b/write_or_die.c\n@@ -49,6 +49,21 @@ void maybe_flush_or_die(FILE *f, const char *desc)\n \t}\n }\n \n+void fprintf_or_die(FILE *f, const char *fmt, ...)\n+{\n+\tva_list ap;\n+\tint ret;\n+\n+\tva_start(ap, fmt);\n+\tret = vfprintf(f, fmt, ap);\n+\tva_end(ap);\n+\n+\tif (ret < 0) {\n+\t\tcheck_pipe(errno);\n+\t\tdie_errno(\"write error\");\n+\t}\n+}\n+\n void fsync_or_die(int fd, const char *msg)\n {\n \tif (fsync(fd) < 0) {\n-- \n2.1.0.373.g91ca799\n"},{"id":"249118","messageId":"54103437.2090305@alum.mit.edu","threadId":"37529","inReplyTo":"20140910100352.GA12164@peff.net","subject":"Re: [PATCH] refs: write packed_refs file using stdio","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-10T11:21:27Z","receivedAt":"2014-09-10T11:21:27Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/10/2014 12:03 PM, Jeff King wrote:\n> We write each line of a new packed-refs file individually\n> using a write() syscall (and sometimes 2, if the ref is\n> peeled). Since each line is only about 50-100 bytes long,\n> this creates a lot of system call overhead.\n> \n> We can instead open a stdio handle around our descriptor and\n> use fprintf to write to it. The extra buffering is not a\n> problem for us, because nobody will read our new packed-refs\n> file until we call commit_lock_file (by which point we have\n> flushed everything).\n> \n> On a pathological repository with 8.5 million refs, this\n> dropped the time to run `git pack-refs` from 20s to 6s.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> Obviously that repo is ridiculous (but a sad reality for me).\n> \n> However, I think the benefits extend to smaller files, too. And it's\n> pretty easy to do (and I actually think the resulting write_packed_entry\n> is a lot easier to read, as well as lifting some arbitrary limits).\n> \n>  cache.h        |  2 ++\n>  refs.c         | 39 ++++++++++++++++-----------------------\n>  write_or_die.c | 15 +++++++++++++++\n>  3 files changed, 33 insertions(+), 23 deletions(-)\n> \n> diff --git a/cache.h b/cache.h\n> index 4d5b76c..bc286ce 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1395,6 +1395,8 @@ extern const char *git_mailmap_blob;\n>  \n>  /* IO helper functions */\n>  extern void maybe_flush_or_die(FILE *, const char *);\n> +__attribute__((format (printf, 2, 3)))\n> +extern void fprintf_or_die(FILE *, const char *fmt, ...);\n>  extern int copy_fd(int ifd, int ofd);\n>  extern int copy_file(const char *dst, const char *src, int mode);\n>  extern int copy_file_with_time(const char *dst, const char *src, int mode);\n> diff --git a/refs.c b/refs.c\n> index 27927f2..f08faed 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2191,25 +2191,12 @@ struct ref_lock *lock_any_ref_for_update(const char *refname,\n>   * Write an entry to the packed-refs file for the specified refname.\n>   * If peeled is non-NULL, write it as the entry's peeled value.\n>   */\n> -static void write_packed_entry(int fd, char *refname, unsigned char *sha1,\n> +static void write_packed_entry(FILE *fh, char *refname, unsigned char *sha1,\n>  \t\t\t       unsigned char *peeled)\n>  {\n> -\tchar line[PATH_MAX + 100];\n> -\tint len;\n> -\n> -\tlen = snprintf(line, sizeof(line), \"%s %s\\n\",\n> -\t\t       sha1_to_hex(sha1), refname);\n> -\t/* this should not happen but just being defensive */\n> -\tif (len > sizeof(line))\n> -\t\tdie(\"too long a refname '%s'\", refname);\n> -\twrite_or_die(fd, line, len);\n> -\n> -\tif (peeled) {\n> -\t\tif (snprintf(line, sizeof(line), \"^%s\\n\",\n> -\t\t\t     sha1_to_hex(peeled)) != PEELED_LINE_LENGTH)\n> -\t\t\tdie(\"internal error\");\n> -\t\twrite_or_die(fd, line, PEELED_LINE_LENGTH);\n> -\t}\n> +\tfprintf_or_die(fh, \"%s %s\\n\", sha1_to_hex(sha1), refname);\n> +\tif (peeled)\n> +\t\tfprintf_or_die(fh, \"^%s\\n\", sha1_to_hex(peeled));\n>  }\n>  \n>  /*\n> @@ -2217,13 +2204,12 @@ static void write_packed_entry(int fd, char *refname, unsigned char *sha1,\n>   */\n>  static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data)\n>  {\n> -\tint *fd = cb_data;\n>  \tenum peel_status peel_status = peel_entry(entry, 0);\n>  \n>  \tif (peel_status != PEEL_PEELED && peel_status != PEEL_NON_TAG)\n>  \t\terror(\"internal error: %s is not a valid packed reference!\",\n>  \t\t      entry->name);\n> -\twrite_packed_entry(*fd, entry->name, entry->u.value.sha1,\n> +\twrite_packed_entry(cb_data, entry->name, entry->u.value.sha1,\n>  \t\t\t   peel_status == PEEL_PEELED ?\n>  \t\t\t   entry->u.value.peeled : NULL);\n>  \treturn 0;\n> @@ -2259,15 +2245,22 @@ int commit_packed_refs(void)\n>  \t\tget_packed_ref_cache(&ref_cache);\n>  \tint error = 0;\n>  \tint save_errno = 0;\n> +\tFILE *out;\n>  \n>  \tif (!packed_ref_cache->lock)\n>  \t\tdie(\"internal error: packed-refs not locked\");\n> -\twrite_or_die(packed_ref_cache->lock->fd,\n> -\t\t     PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER));\n>  \n> +\tout = fdopen(packed_ref_cache->lock->fd, \"w\");\n> +\tif (!out)\n> +\t\tdie_errno(\"unable to fdopen packed-refs descriptor\");\n> +\n> +\tfprintf_or_die(out, \"%s\", PACKED_REFS_HEADER);\n>  \tdo_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),\n> -\t\t\t\t 0, write_packed_entry_fn,\n> -\t\t\t\t &packed_ref_cache->lock->fd);\n> +\t\t\t\t 0, write_packed_entry_fn, out);\n> +\tif (fclose(out))\n> +\t\tdie_errno(\"write error\");\n> +\tpacked_ref_cache->lock->fd = -1;\n\nIt might be a minuscule bit safer to set `lock->fd = -1` *before*\ncalling `fclose()`.\n\nTBH, it makes me uncomfortable having code outside of `lockfile.c`\nhaving this level of intimacy with lockfile objects. I think it would be\nbetter to have a\n\n    FILE *fopen_lock_file(struct *lock_file, const char *mode);\n\nthat records the `FILE *` inside the `lockfile` instance, and to teach\n`commit_lock_file()` and its friends to call `fclose()` if the `FILE *`\nwas created. I think that such a feature would encourage other lockfile\nusers to use the more convenient and readable stdio API.\n\nBut there is precedent for what you are doing so I will add\n`fopen_lock_file()` to my mythical todo list and not bother you further\nwith the idea.\n\n> +\n>  \tif (commit_lock_file(packed_ref_cache->lock)) {\n>  \t\tsave_errno = errno;\n>  \t\terror = -1;\n> diff --git a/write_or_die.c b/write_or_die.c\n> index b50f99a..e7afe7a 100644\n> --- a/write_or_die.c\n> +++ b/write_or_die.c\n> @@ -49,6 +49,21 @@ void maybe_flush_or_die(FILE *f, const char *desc)\n>  \t}\n>  }\n>  \n> +void fprintf_or_die(FILE *f, const char *fmt, ...)\n> +{\n> +\tva_list ap;\n> +\tint ret;\n> +\n> +\tva_start(ap, fmt);\n> +\tret = vfprintf(f, fmt, ap);\n> +\tva_end(ap);\n> +\n> +\tif (ret < 0) {\n> +\t\tcheck_pipe(errno);\n> +\t\tdie_errno(\"write error\");\n> +\t}\n> +}\n> +\n>  void fsync_or_die(int fd, const char *msg)\n>  {\n>  \tif (fsync(fd) < 0) {\n> \n\nReviewed-by: Michael Haggerty <mhagger@alum.mit.edu>\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"249119","messageId":"20140910113948.GA16224@peff.net","threadId":"37529","inReplyTo":"54103437.2090305@alum.mit.edu","subject":"Re: [PATCH] refs: write packed_refs file using stdio","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-10T11:39:48Z","receivedAt":"2014-09-10T11:39:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 10, 2014 at 01:21:27PM +0200, Michael Haggerty wrote:\n\n> > +\tif (fclose(out))\n> > +\t\tdie_errno(\"write error\");\n> > +\tpacked_ref_cache->lock->fd = -1;\n> \n> It might be a minuscule bit safer to set `lock->fd = -1` *before*\n> calling `fclose()`.\n\nYeah, I considered that. The worst case is a signal coming in between\nthe two calls (or a somebody calling die() between the two :) ). In that\ncase the lockfile code will close() the fd again, which should be a noop\n(since nobody will have opened it in the interim...right?).\n\nThat \"since\" assumption is the dangerous part. But on the other hand, if\nwe unset the fd first, then the lockfile code may fail to close it if it\nis called before the fclose() (definitely by a signal in this case). I\nthink there are platforms where that would cause us to fail to remove\nthe file, which is annoying.\n\nSo I dunno. We cannot be atomic here. I could go either way.\n\n> TBH, it makes me uncomfortable having code outside of `lockfile.c`\n> having this level of intimacy with lockfile objects.\n\nI kind of agree.\n\n> I think it would be better to have a\n> \n>     FILE *fopen_lock_file(struct *lock_file, const char *mode);\n> \n> that records the `FILE *` inside the `lockfile` instance, and to teach\n> `commit_lock_file()` and its friends to call `fclose()` if the `FILE *`\n> was created. I think that such a feature would encourage other lockfile\n> users to use the more convenient and readable stdio API.\n\nI was tempted by that. We could also do:\n\n  if (fflush(out))\n\tdie_errno(\"write error\");\n  if (commit_lock_file(...))\n     ...\n\nwhich sidesteps the issue. We do then have to _later_ call fclose(out)\nto free the handle memory, which is going to want to close the fd again.\nPutting us back in the \"let's hope nobody opened it in the meantime\"\ncase from above.\n\n-Peff\n"},{"id":"249145","messageId":"20140910191453.GB23655@peff.net","threadId":"37529","inReplyTo":"CAL=YDW=uWP2kWB31MEvJvVP7yUdwoh95PvfEYT6LT1x2UXpAvg@mail.gmail.com","subject":"Re: [PATCH] refs: write packed_refs file using stdio","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-10T19:14:54Z","receivedAt":"2014-09-10T19:14:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 10, 2014 at 07:32:17AM -0700, Ronnie Sahlberg wrote:\n\n> Even better could be to build an iovec for all the data and use writev() ?\n> (now without gmail adding formatting)\n\nI'm not sure that would make anything easier. We're literally\nsprintf-ing (and calling sha1_to_hex) into a buffer so we can write()\nit. That seems like a solid case for using fprintf instead.\n\nWe can't just writev() a bunch of buffers we have already. We'd\nliterally have to make an array of buffers to format into. It's much\nsimpler (and probably faster) to just format directly into stdio's\nbuffers.\n\n-Peff\n"}]}