Re: [PATCH RESEND] Avoid a useless prefix lookup in strbuf_expand()

2 messages, 2 authors, 2016-06-15 · open the first message on its own page

Re: [PATCH RESEND] Avoid a useless prefix lookup in strbuf_expand()

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:44:10

Marco Costalba [off-list ref] writes:

quoted hunk

Currently the --prett=format prefix is looked up in a
...

diff --git a/pretty.c b/pretty.c
index b987ff2..64ead65 100644
--- a/pretty.c
+++ b/pretty.c

@@ -282,16 +282,18 @@ static char *logmsg_reencode(const struct commit *commit,
 	return out;
 }
 
-static void format_person_part(struct strbuf *sb, char part,
+/* returns placeholder length or 0 if placeholder is not known */
+static size_t format_person_part(struct strbuf *sb, char part,
                                const char *msg, int len)
 {
-	int start, end, tz = 0;
-	unsigned long date;
+	int start, end, tz = 0, end_of_data;
+	unsigned long date = 0;
 	char *ep;
 
-	/* parse name */
+	/* advance 'end' to point to email start delimiter */
 	for (end = 0; end < len && msg[end] != '<'; end++)
 		; /* do nothing */
+

Another thing I noticed is that --pretty='format:%an %ae %at'
would end up running this function three times.  Perhaps it is
worth memoizing the result in format_commit_context while you
are at it?

Re: [PATCH RESEND] Avoid a useless prefix lookup in strbuf_expand()

From: Marco Costalba <hidden>
Date: 2016-06-15 22:44:10

On Feb 5, 2008 8:00 AM, Junio C Hamano [off-list ref] wrote:

Another thing I noticed is that --pretty='format:%an %ae %at'
would end up running this function three times.  Perhaps it is
worth memoizing the result in format_commit_context while you
are at it?

Yes, I tought about this. But at the end I rejected this idea to keep
the code simple and because the optimization was in any case small
(and also in a special cases only).

I did some profiling with and without my patch and I noticed a nice
improvement so that with the patch applied git-rev-list and git-log
--pretty=format have almost the same execution times.

This is because the biggest time in both cases is spent in zlib
decompress, so also a local further speed up of  format_person_part()
does not gain anything but complicates the code.

Marco

P.S: I will try to adress your concerns on the naming of end_of_data
flag (hints are welcomed, I'm bad at naming) and on the skipping
further elaborations if end_of_data is true to have cleaner code, I
was thinking about using 'goto xxxx' or something like that.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help