git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v2] reuse cached commit buffer when parsing signatures

From
Jeff King <peff@peff.net>
Date
Jun 13, 2014, 06:32 UTC
Message-ID
<20140613063211.GA7552@sigill.intra.peff.net>
In-Reply-To
<20140610213509.GA26979@sigill.intra.peff.net>
On Tue, Jun 10, 2014 at 05:35:09PM -0400, Jeff King wrote:
> Here's a re-roll of the commit-slab series.

And here is a re-roll of the --show-signature speedup on top (i.e., the point of the series in the first place), adjusted to use get_commit_buffer.

-- >8 --
Subject: reuse cached commit buffer when parsing signatures

When we call show_signature or show_mergetag, we read the commit object fresh via read_sha1_file and reparse its headers. However, in most cases we already have the object data available, attached to the "struct commit". This is partially laziness in dealing with the memory allocation issues, but partially defensive programming, in that we would always want to verify a clean version of the buffer (not one that might have been munged by other users of the commit).

However, we do not currently ever munge the commit buffer, and not using the already-available buffer carries a fairly big performance penalty when we are looking at a large number of commits. Here are timings on linux.git:

  [baseline, no signatures]
  $ time git log >/dev/null
  real    0m4.902s
  user    0m4.784s
  sys     0m0.120s
  [before]
  $ time git log --show-signature >/dev/null
  real    0m14.735s
  user    0m9.964s
  sys     0m0.944s
  [after]
  $ time git log --show-signature >/dev/null
  real    0m9.981s
  user    0m5.260s
  sys     0m0.936s

Note that our user CPU time drops almost in half, close to the non-signature case, but we do still spend more wall-clock and system time, presumably from dealing with gpg.

An alternative to this is to note that most commits do not have signatures (less than 1% in this repo), yet we pay the re-parsing cost for every commit just to find out if it has a mergetag or signature. If we checked that when parsing the commit initially, we could avoid re-examining most commits later on. Even if we did pursue that direction, however, this would still speed up the cases where we _do_ have signatures. So it's probably worth doing either way.

Signed-off-by: Jeff King <peff@peff.net>
---
 commit.c   | 27 ++++++++++-----------------
 commit.h   |  2 +-
 log-tree.c |  2 +-
 3 files changed, 12 insertions(+), 19 deletions(-)
diff --git a/commit.c b/commit.c
index a036e18..ebd7ad8 100644
--- a/commit.c
+++ b/commit.c
@@ -1138,17 +1138,14 @@ static int do_sign_commit(struct strbuf *buf, const char *keyid)
 	return 0;
 }
 
-int parse_signed_commit(const unsigned char *sha1,
+int parse_signed_commit(const struct commit *commit,
 			struct strbuf *payload, struct strbuf *signature)
 {
+
 	unsigned long size;
-	enum object_type type;
-	char *buffer = read_sha1_file(sha1, &type, &size);
+	const char *buffer = get_commit_buffer(commit, &size);
 	int in_signature, saw_signature = -1;
-	char *line, *tail;
-
-	if (!buffer || type != OBJ_COMMIT)
-		goto cleanup;
+	const char *line, *tail;
 
 	line = buffer;
 	tail = buffer + size;
@@ -1156,7 +1153,7 @@ int parse_signed_commit(const unsigned char *sha1,
 	saw_signature = 0;
 	while (line < tail) {
 		const char *sig = NULL;
-		char *next = memchr(line, '\n', tail - line);
+		const char *next = memchr(line, '\n', tail - line);
 
 		next = next ? next + 1 : tail;
 		if (in_signature && line[0] == ' ')
@@ -1177,8 +1174,7 @@ int parse_signed_commit(const unsigned char *sha1,
 		}
 		line = next;
 	}
- cleanup:
-	free(buffer);
+	unuse_commit_buffer(commit, buffer);
 	return saw_signature;
 }
 
@@ -1269,8 +1265,7 @@ void check_commit_signature(const struct commit* commit, struct signature_check
 
 	sigc->result = 'N';
 
-	if (parse_signed_commit(commit->object.sha1,
-				&payload, &signature) <= 0)
+	if (parse_signed_commit(commit, &payload, &signature) <= 0)
 		goto out;
 	status = verify_signed_buffer(payload.buf, payload.len,
 				      signature.buf, signature.len,
@@ -1315,11 +1310,9 @@ struct commit_extra_header *read_commit_extra_headers(struct commit *commit,
 {
 	struct commit_extra_header *extra = NULL;
 	unsigned long size;
-	enum object_type type;
-	char *buffer = read_sha1_file(commit->object.sha1, &type, &size);
-	if (buffer && type == OBJ_COMMIT)
-		extra = read_commit_extra_header_lines(buffer, size, exclude);
-	free(buffer);
+	const char *buffer = get_commit_buffer(commit, &size);
+	extra = read_commit_extra_header_lines(buffer, size, exclude);
+	unuse_commit_buffer(commit, buffer);
 	return extra;
 }
 
diff --git a/commit.h b/commit.h
index 61559a9..2e1492a 100644
--- a/commit.h
+++ b/commit.h
@@ -325,7 +325,7 @@ struct merge_remote_desc {
  */
 struct commit *get_merge_parent(const char *name);
 
-extern int parse_signed_commit(const unsigned char *sha1,
+extern int parse_signed_commit(const struct commit *commit,
 			       struct strbuf *message, struct strbuf *signature);
 extern void print_commit_list(struct commit_list *list,
 			      const char *format_cur,
diff --git a/log-tree.c b/log-tree.c
index 4447021..10e6844 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -376,7 +376,7 @@ static void show_signature(struct rev_info *opt, struct commit *commit)
 	struct strbuf gpg_output = STRBUF_INIT;
 	int status;
 
-	if (parse_signed_commit(commit->object.sha1, &payload, &signature) <= 0)
+	if (parse_signed_commit(commit, &payload, &signature) <= 0)
 		goto out;
 
 	status = verify_signed_buffer(payload.buf, payload.len,

-- 
2.0.0.566.gfe3e6b2
Previous: Junio C Hamano
Message 60 of 60 in “store length of commit->buffer”
  1. 0/15 store length of commit->bufferJeff King, Jun 9, 2014
  2. 01/15 alloc: include any-object allocations in alloc_reportJeff King, Jun 9, 2014
  3. 02/15 commit: push commit_index update into alloc_commit_nodeJeff King, Jun 9, 2014
  4. Junio C HamanoJun 10, 2014
  5. 03/15 do not create "struct commit" with xcallocJeff King, Jun 9, 2014
  6. Junio C HamanoJun 10, 2014
  7. Jeff KingJun 10, 2014
  8. 04/15 logmsg_reencode: return const bufferJeff King, Jun 9, 2014
  9. Eric SunshineJun 10, 2014
  10. 05/15 sequencer: use logmsg_reencode in get_messageJeff King, Jun 9, 2014
  11. Junio C HamanoJun 10, 2014
  12. Jeff KingJun 10, 2014
  13. 06/15 provide a helper to free commit bufferJeff King, Jun 9, 2014
  14. 07/15 provide a helper to set the commit bufferJeff King, Jun 9, 2014
  15. 08/15 provide helpers to access the commit bufferJeff King, Jun 9, 2014
  16. Eric SunshineJun 10, 2014
  17. 09/15 use get_cached_commit_buffer where appropriateJeff King, Jun 9, 2014
  18. 10/15 use get_commit_buffer to avoid duplicate codeJeff King, Jun 9, 2014
  19. Eric SunshineJun 10, 2014
  20. Jeff KingJun 10, 2014
  21. 11/15 convert logmsg_reencode to get_commit_bufferJeff King, Jun 9, 2014
  22. 12/15 use get_commit_buffer everywhereJeff King, Jun 9, 2014
  23. Junio C HamanoJun 9, 2014
  24. Jeff KingJun 10, 2014
  25. Jeff KingJun 10, 2014
  26. Junio C HamanoJun 10, 2014
  27. Jeff KingJun 10, 2014
  28. 13/15 commit-slab: provide a static initializerJeff King, Jun 9, 2014
  29. 14/15 commit: convert commit->buffer to a slabJeff King, Jun 9, 2014
  30. 15/15 commit: record buffer length in cacheJeff King, Jun 9, 2014
  31. Christian CouderJun 10, 2014
  32. Jeff KingJun 10, 2014
  33. Jeff KingJun 10, 2014
  34. Christian CouderJun 10, 2014
  35. 0/17 store length of commit->bufferJeff King, Jun 10, 2014
  36. 01/17 commit_tree: take a pointer/len pair rather than a const strbufJeff King, Jun 10, 2014
  37. 02/17 replace dangerous uses of strbuf_attachJeff King, Jun 10, 2014
  38. 03/17 alloc: include any-object allocations in alloc_reportJeff King, Jun 10, 2014
  39. 04/17 commit: push commit_index update into alloc_commit_nodeJeff King, Jun 10, 2014
  40. 05/17 do not create "struct commit" with xcallocJeff King, Jun 10, 2014
  41. 06/17 logmsg_reencode: return const bufferJeff King, Jun 10, 2014
  42. 07/17 sequencer: use logmsg_reencode in get_messageJeff King, Jun 10, 2014
  43. 08/17 provide a helper to free commit bufferJeff King, Jun 10, 2014
  44. Junio C HamanoJun 12, 2014
  45. Jeff KingJun 12, 2014
  46. Jeff KingJun 12, 2014
  47. 09/17 provide a helper to set the commit bufferJeff King, Jun 10, 2014
  48. 10/17 provide helpers to access the commit bufferJeff King, Jun 10, 2014
  49. 11/17 use get_cached_commit_buffer where appropriateJeff King, Jun 10, 2014
  50. 12/17 use get_commit_buffer to avoid duplicate codeJeff King, Jun 10, 2014
  51. 13/17 convert logmsg_reencode to get_commit_bufferJeff King, Jun 10, 2014
  52. 14/17 use get_commit_buffer everywhereJeff King, Jun 10, 2014
  53. 15/17 commit-slab: provide a static initializerJeff King, Jun 10, 2014
  54. Junio C HamanoJun 12, 2014
  55. Jeff KingJun 12, 2014
  56. 16/17 commit: convert commit->buffer to a slabJeff King, Jun 10, 2014
  57. 17/17 commit: record buffer length in cacheJeff King, Jun 10, 2014
  58. Jeff KingJun 10, 2014
  59. Junio C HamanoJun 12, 2014
  60. reuse cached commit buffer when parsing signaturesJeff King, Jun 13, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.