{"thread":{"id":"47631","subject":"[PATCH v3 2/2] sha1_file: improve sha1_file_name() perfs","startedAt":"2018-01-18T10:09:20Z","lastAt":"2018-01-18T10:09:27Z","messageCount":2,"participants":["Christian Couder"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"336764","messageId":"20180118100854.22467-2-chriscool@tuxfamily.org","threadId":"47631","inReplyTo":"20180118100854.22467-1-chriscool@tuxfamily.org","subject":"[PATCH v3 2/2] sha1_file: improve sha1_file_name() perfs","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-01-18T10:08:54Z","receivedAt":"2018-01-18T10:09:20Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"As sha1_file_name() could be performance sensitive, let's\nmake it faster by using strbuf_addstr() and strbuf_addc()\ninstead of strbuf_addf().\n\nHelped-by: Derrick Stolee <stolee@gmail.com>\nHelped-by: Jeff Hostetler <git@jeffhostetler.com>\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n sha1_file.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\nSo compared to v2 I just removed the call to strbuf_grow()\nand the related computation. \n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex f66c21b2da..4f7f17a24c 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -323,8 +323,8 @@ static void fill_sha1_path(struct strbuf *buf, const unsigned char *sha1)\n \n void sha1_file_name(struct strbuf *buf, const unsigned char *sha1)\n {\n-\tstrbuf_addf(buf, \"%s/\", get_object_directory());\n-\n+\tstrbuf_addstr(buf, get_object_directory());\n+\tstrbuf_addch(buf, '/');\n \tfill_sha1_path(buf, sha1);\n }\n \n-- \n2.16.0.rc2.3.g7330290b09\n\n"},{"id":"336765","messageId":"20180118100854.22467-1-chriscool@tuxfamily.org","threadId":"47631","inReplyTo":null,"subject":"[PATCH v3 1/2] sha1_file: remove static strbuf from sha1_file_name()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-01-18T10:08:53Z","receivedAt":"2018-01-18T10:09:27Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Using a static buffer in sha1_file_name() is error prone\nand the performance improvements it gives are not needed\nin many of the callers.\n\nSo let's get rid of this static buffer and, if necessary\nor helpful, let's use one in the caller.\n\nSuggested-by: Jeff Hostetler <git@jeffhostetler.com>\nHelped-by: Kevin Daudt <me@ikke.info>\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n cache.h       |  8 +++-----\n http-walker.c |  6 ++++--\n http.c        | 16 ++++++++++------\n sha1_file.c   | 38 +++++++++++++++++++++++++-------------\n 4 files changed, 42 insertions(+), 26 deletions(-)\n\nThis patch is the same as v2.\n\ndiff --git a/cache.h b/cache.h\nindex d8b975a571..6db565408e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -957,12 +957,10 @@ extern void check_repository_format(void);\n #define TYPE_CHANGED    0x0040\n \n /*\n- * Return the name of the file in the local object database that would\n- * be used to store a loose object with the specified sha1.  The\n- * return value is a pointer to a statically allocated buffer that is\n- * overwritten each time the function is called.\n+ * Put in `buf` the name of the file in the local object database that\n+ * would be used to store a loose object with the specified sha1.\n  */\n-extern const char *sha1_file_name(const unsigned char *sha1);\n+extern void sha1_file_name(struct strbuf *buf, const unsigned char *sha1);\n \n /*\n  * Return an abbreviated sha1 unique within this repository's object database.\ndiff --git a/http-walker.c b/http-walker.c\nindex 1ae8363de2..07c2b1af82 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -544,8 +544,10 @@ static int fetch_object(struct walker *walker, unsigned char *sha1)\n \t} else if (hashcmp(obj_req->sha1, req->real_sha1)) {\n \t\tret = error(\"File %s has bad hash\", hex);\n \t} else if (req->rename < 0) {\n-\t\tret = error(\"unable to write sha1 filename %s\",\n-\t\t\t    sha1_file_name(req->sha1));\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\tsha1_file_name(&buf, req->sha1);\n+\t\tret = error(\"unable to write sha1 filename %s\", buf.buf);\n+\t\tstrbuf_release(&buf);\n \t}\n \n \trelease_http_object_request(req);\ndiff --git a/http.c b/http.c\nindex 5977712712..5979305bc9 100644\n--- a/http.c\n+++ b/http.c\n@@ -2168,7 +2168,7 @@ struct http_object_request *new_http_object_request(const char *base_url,\n \tunsigned char *sha1)\n {\n \tchar *hex = sha1_to_hex(sha1);\n-\tconst char *filename;\n+\tstruct strbuf filename = STRBUF_INIT;\n \tchar prevfile[PATH_MAX];\n \tint prevlocal;\n \tchar prev_buf[PREV_BUF_SIZE];\n@@ -2180,14 +2180,15 @@ struct http_object_request *new_http_object_request(const char *base_url,\n \thashcpy(freq->sha1, sha1);\n \tfreq->localfile = -1;\n \n-\tfilename = sha1_file_name(sha1);\n+\tsha1_file_name(&filename, sha1);\n \tsnprintf(freq->tmpfile, sizeof(freq->tmpfile),\n-\t\t \"%s.temp\", filename);\n+\t\t \"%s.temp\", filename.buf);\n \n-\tsnprintf(prevfile, sizeof(prevfile), \"%s.prev\", filename);\n+\tsnprintf(prevfile, sizeof(prevfile), \"%s.prev\", filename.buf);\n \tunlink_or_warn(prevfile);\n \trename(freq->tmpfile, prevfile);\n \tunlink_or_warn(freq->tmpfile);\n+\tstrbuf_release(&filename);\n \n \tif (freq->localfile != -1)\n \t\terror(\"fd leakage in start: %d\", freq->localfile);\n@@ -2302,6 +2303,7 @@ void process_http_object_request(struct http_object_request *freq)\n int finish_http_object_request(struct http_object_request *freq)\n {\n \tstruct stat st;\n+\tstruct strbuf filename = STRBUF_INIT;\n \n \tclose(freq->localfile);\n \tfreq->localfile = -1;\n@@ -2327,8 +2329,10 @@ int finish_http_object_request(struct http_object_request *freq)\n \t\tunlink_or_warn(freq->tmpfile);\n \t\treturn -1;\n \t}\n-\tfreq->rename =\n-\t\tfinalize_object_file(freq->tmpfile, sha1_file_name(freq->sha1));\n+\n+\tsha1_file_name(&filename, freq->sha1);\n+\tfreq->rename = finalize_object_file(freq->tmpfile, filename.buf);\n+\tstrbuf_release(&filename);\n \n \treturn freq->rename;\n }\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 3da70ac650..f66c21b2da 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -321,15 +321,11 @@ static void fill_sha1_path(struct strbuf *buf, const unsigned char *sha1)\n \t}\n }\n \n-const char *sha1_file_name(const unsigned char *sha1)\n+void sha1_file_name(struct strbuf *buf, const unsigned char *sha1)\n {\n-\tstatic struct strbuf buf = STRBUF_INIT;\n-\n-\tstrbuf_reset(&buf);\n-\tstrbuf_addf(&buf, \"%s/\", get_object_directory());\n+\tstrbuf_addf(buf, \"%s/\", get_object_directory());\n \n-\tfill_sha1_path(&buf, sha1);\n-\treturn buf.buf;\n+\tfill_sha1_path(buf, sha1);\n }\n \n struct strbuf *alt_scratch_buf(struct alternate_object_database *alt)\n@@ -710,7 +706,12 @@ int check_and_freshen_file(const char *fn, int freshen)\n \n static int check_and_freshen_local(const unsigned char *sha1, int freshen)\n {\n-\treturn check_and_freshen_file(sha1_file_name(sha1), freshen);\n+\tstatic struct strbuf buf = STRBUF_INIT;\n+\n+\tstrbuf_reset(&buf);\n+\tsha1_file_name(&buf, sha1);\n+\n+\treturn check_and_freshen_file(buf.buf, freshen);\n }\n \n static int check_and_freshen_nonlocal(const unsigned char *sha1, int freshen)\n@@ -866,8 +867,12 @@ static int stat_sha1_file(const unsigned char *sha1, struct stat *st,\n \t\t\t  const char **path)\n {\n \tstruct alternate_object_database *alt;\n+\tstatic struct strbuf buf = STRBUF_INIT;\n+\n+\tstrbuf_reset(&buf);\n+\tsha1_file_name(&buf, sha1);\n+\t*path = buf.buf;\n \n-\t*path = sha1_file_name(sha1);\n \tif (!lstat(*path, st))\n \t\treturn 0;\n \n@@ -891,8 +896,12 @@ static int open_sha1_file(const unsigned char *sha1, const char **path)\n \tint fd;\n \tstruct alternate_object_database *alt;\n \tint most_interesting_errno;\n+\tstatic struct strbuf buf = STRBUF_INIT;\n+\n+\tstrbuf_reset(&buf);\n+\tsha1_file_name(&buf, sha1);\n+\t*path = buf.buf;\n \n-\t*path = sha1_file_name(sha1);\n \tfd = git_open(*path);\n \tif (fd >= 0)\n \t\treturn fd;\n@@ -1557,9 +1566,12 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n \tgit_SHA_CTX c;\n \tunsigned char parano_sha1[20];\n \tstatic struct strbuf tmp_file = STRBUF_INIT;\n-\tconst char *filename = sha1_file_name(sha1);\n+\tstatic struct strbuf filename = STRBUF_INIT;\n+\n+\tstrbuf_reset(&filename);\n+\tsha1_file_name(&filename, sha1);\n \n-\tfd = create_tmpfile(&tmp_file, filename);\n+\tfd = create_tmpfile(&tmp_file, filename.buf);\n \tif (fd < 0) {\n \t\tif (errno == EACCES)\n \t\t\treturn error(\"insufficient permission for adding an object to repository database %s\", get_object_directory());\n@@ -1612,7 +1624,7 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n \t\t\twarning_errno(\"failed utime() on %s\", tmp_file.buf);\n \t}\n \n-\treturn finalize_object_file(tmp_file.buf, filename);\n+\treturn finalize_object_file(tmp_file.buf, filename.buf);\n }\n \n static int freshen_loose_object(const unsigned char *sha1)\n-- \n2.16.0.rc2.3.g7330290b09\n\n"}]}