{"thread":{"id":"35851","subject":"[PATCH] blame.c: prepare_lines should not call xrealloc for every line","startedAt":"2014-02-12T14:27:24Z","lastAt":"2014-02-12T19:36:44Z","messageCount":2,"participants":["David Kastrup","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"234660","messageId":"1392215244-26785-1-git-send-email-dak@gnu.org","threadId":"35851","inReplyTo":null,"subject":"[PATCH] blame.c: prepare_lines should not call xrealloc for every line","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-02-12T14:27:24Z","receivedAt":"2014-02-12T14:27:24Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Making a single preparation run for counting the lines will avoid memory\nfragmentation.  Also, fix the allocated memory size which was wrong\nwhen sizeof(int *) != sizeof(int), and would have been too small\nfor sizeof(int *) < sizeof(int), admittedly unlikely.\n\nSigned-off-by: David Kastrup <dak@gnu.org>\n---\n\nSince there was no feedback after the last defense/explanation of the\ncoding choices, the code rewritten by this patch was much more awful,\nand the kind of style requests (fixed already in the last iteration)\nare not actually heeded by the core developers themselves, I have no\nidea whether this patch will be dropped just like the last one.\n\nAs opposed to the last try, this incorporates a suggestion from Jeff\nto change sizeof(type) to sizeof(expression) which is not helping much\nsince the types of lineno and sb->lineno still need to be changed in\nsync.\n\nIt also fiddles cosmetically with the code layout of the loops.\n\nbuiltin/blame.c | 46 +++++++++++++++++++++++++++++++---------------\n 1 file changed, 31 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex e44a6bb..1aefedf 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1772,25 +1772,41 @@ static int prepare_lines(struct scoreboard *sb)\n {\n \tconst char *buf = sb->final_buf;\n \tunsigned long len = sb->final_buf_size;\n-\tint num = 0, incomplete = 0, bol = 1;\n+\tconst char *end = buf + len;\n+\tconst char *p;\n+\tint *lineno;\n+\tint num = 0, incomplete = 0;\n \n-\tif (len && buf[len-1] != '\\n')\n-\t\tincomplete++; /* incomplete line at the end */\n-\twhile (len--) {\n-\t\tif (bol) {\n-\t\t\tsb->lineno = xrealloc(sb->lineno,\n-\t\t\t\t\t      sizeof(int *) * (num + 1));\n-\t\t\tsb->lineno[num] = buf - sb->final_buf;\n-\t\t\tbol = 0;\n-\t\t}\n-\t\tif (*buf++ == '\\n') {\n+\tfor (p = buf;;) {\n+\t\tp = memchr(p, '\\n', end - p);\n+\t\tif (p) {\n+\t\t\tp++;\n \t\t\tnum++;\n-\t\t\tbol = 1;\n+\t\t\tcontinue;\n \t\t}\n+\t\tbreak;\n \t}\n-\tsb->lineno = xrealloc(sb->lineno,\n-\t\t\t      sizeof(int *) * (num + incomplete + 1));\n-\tsb->lineno[num + incomplete] = buf - sb->final_buf;\n+\n+\tif (len && end[-1] != '\\n')\n+\t\tincomplete++; /* incomplete line at the end */\n+\n+\tsb->lineno = xmalloc(sizeof(*sb->lineno) * (num + incomplete + 1));\n+\tlineno = sb->lineno;\n+\n+\t*lineno++ = 0;\n+\tfor (p = buf;;) {\n+\t\tp = memchr(p, '\\n', end - p);\n+\t\tif (p) {\n+\t\t\tp++;\n+\t\t\t*lineno++ = p - buf;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tbreak;\n+\t}\n+\n+\tif (incomplete)\n+\t\t*lineno++ = len;\n+\n \tsb->num_lines = num + incomplete;\n \treturn sb->num_lines;\n }\n-- \n1.8.3.2\n"},{"id":"234685","messageId":"xmqqvbwkm8c3.fsf@gitster.dls.corp.google.com","threadId":"35851","inReplyTo":"1392215244-26785-1-git-send-email-dak@gnu.org","subject":"Re: [PATCH] blame.c: prepare_lines should not call xrealloc for every line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-12T19:36:44Z","receivedAt":"2014-02-12T19:36:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Kastrup <dak@gnu.org> writes:\n\n> Making a single preparation run for counting the lines will avoid memory\n> fragmentation.  Also, fix the allocated memory size which was wrong\n> when sizeof(int *) != sizeof(int), and would have been too small\n> for sizeof(int *) < sizeof(int), admittedly unlikely.\n>\n> Signed-off-by: David Kastrup <dak@gnu.org>\n> ---\n\nI think I took sizeof(int*)->sizeof(int) patch to the 'next' branch\nalready, which might have to conflict with this clean-up, but it\nshould be trivial to resolve.\n\nThanks for resending.  I was busy elsewhere (i.e. \"no feedback\" does\nnot mean \"silent rejection\" nor \"silent agreement\" at least from\nme), and such a resend does help prevent patches fall thru cracks.\n\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index e44a6bb..1aefedf 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -1772,25 +1772,41 @@ static int prepare_lines(struct scoreboard *sb)\n>  {\n>  \tconst char *buf = sb->final_buf;\n>  \tunsigned long len = sb->final_buf_size;\n> +\tconst char *end = buf + len;\n> +\tconst char *p;\n> +\tint *lineno;\n> +\tint num = 0, incomplete = 0;\n>  \n> +\tfor (p = buf;;) {\n> +\t\tp = memchr(p, '\\n', end - p);\n> +\t\tif (p) {\n> +\t\t\tp++;\n>  \t\t\tnum++;\n> +\t\t\tcontinue;\n>  \t\t}\n> +\t\tbreak;\n>  \t}\n> +\n> +\tif (len && end[-1] != '\\n')\n> +\t\tincomplete++; /* incomplete line at the end */\n> +\n> +\tsb->lineno = xmalloc(sizeof(*sb->lineno) * (num + incomplete + 1));\n> +\tlineno = sb->lineno;\n> +\n> +\t*lineno++ = 0;\n> +\tfor (p = buf;;) {\n> +\t\tp = memchr(p, '\\n', end - p);\n> +\t\tif (p) {\n> +\t\t\tp++;\n> +\t\t\t*lineno++ = p - buf;\n> +\t\t\tcontinue;\n> +\t\t}\n> +\t\tbreak;\n> +\t}\n> +\n> +\tif (incomplete)\n> +\t\t*lineno++ = len;\n> +\n>  \tsb->num_lines = num + incomplete;\n>  \treturn sb->num_lines;\n>  }\n"}]}