{"thread":{"id":"45668","subject":"[PATCH v4 0/2] string-list: use ALLOC_GROW macro when reallocing","startedAt":"2017-04-11T20:08:18Z","lastAt":"2017-04-11T21:35:31Z","messageCount":6,"participants":["git@jeffhostetler.com","Jeff King","Jonathan Nieder"],"isPatch":true,"patchVersion":4,"patchTotal":2},"messages":[{"id":"316634","messageId":"20170411200802.31638-1-git@jeffhostetler.com","threadId":"45668","inReplyTo":null,"subject":"[PATCH v4 0/2] string-list: use ALLOC_GROW macro when reallocing","fromName":"","fromEmail":"git@jeffhostetler.com","sentAt":"2017-04-11T20:08:00Z","receivedAt":"2017-04-11T20:08:18Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nVersion 4 greatly simplifies the p0005 perf test. It now uses an existing\nrepo -- either real-world or artificial from t/perf/repos/many-files.sh.\n\nJeff Hostetler (2):\n string-list: use ALLOC_GROW macro when reallocing string_list\n p0005-status: time status on very large repo\n\n string-list.c | 5 +----\n t/perf/p0005-status.sh | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 52 insertions(+), 4 deletions(-)\n create mode 100755 t/perf/p0005-status.sh\n\n-- \n2.9.3\n\n"},{"id":"316635","messageId":"20170411200802.31638-3-git@jeffhostetler.com","threadId":"45668","inReplyTo":"20170411200802.31638-1-git@jeffhostetler.com","subject":"[PATCH v4 2/2] p0005-status: time status on very large repo","fromName":"","fromEmail":"git@jeffhostetler.com","sentAt":"2017-04-11T20:08:02Z","receivedAt":"2017-04-11T20:08:20Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nSigned-off-by: Jeff Hostetler \n---\n t/perf/p0005-status.sh | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 51 insertions(+)\n create mode 100755 t/perf/p0005-status.sh\n\ndiff --git a/t/perf/p0005-status.sh b/t/perf/p0005-status.sh\nnew file mode 100755\nindex 0000000..0469aee\n--- /dev/null\n+++ b/t/perf/p0005-status.sh\n@@ -0,0 +1,51 @@\n+#!/bin/sh\n+##\n+## This test measures the performance of various read-tree\n+## and status operations. It is primarily interested in\n+## the algorithmic costs of index operations and recursive\n+## tree traversal -- and NOT disk I/O on thousands of files.\n+\n+test_description=\"Tests performance of read-tree\"\n+\n+. ./perf-lib.sh\n+\n+test_perf_default_repo\n+\n+## If the test repo was generated by ./repos/many-files.sh\n+## then we know something about the data shape and branches,\n+## so we can isolate testing to the ballast-related commits\n+## and setup sparse-checkout so we don't have to populate\n+## the ballast files and directories.\n+##\n+## Otherwise, we make some general assumptions about the\n+## repo and consider the entire history of the current\n+## branch to be the ballast.\n+\n+git branch | grep p0006-ballast >/dev/null 2>&1\n+synthetic=$?\n+if test \"$synthetic\" = 0\n+then\n+ echo Assuming synthetic repo from many-files.sh\n+ git branch br_base master\n+ git branch br_ballast p0006-ballast\n+ echo '/*' >.git/info/sparse-checkout\n+ echo '!ballast/*' >>.git/info/sparse-checkout\n+ git config --local core.sparsecheckout 1\n+else\n+ echo Assuming non-synthetic repo...\n+ git branch br_base $(git rev-list HEAD | tail -n 1)\n+ git branch br_ballast HEAD\n+fi\n+\n+test_expect_success 'setup' '\n+\tgit checkout -q br_ballast\n+'\n+\n+nr_files=$(git ls-files | wc -l)\n+\n+test_perf \"read-tree status br_ballast ($nr_files)\" '\n+\tgit read-tree HEAD &&\n+\tgit status\n+'\n+\n+test_done\n-- \n2.9.3\n\n"},{"id":"316636","messageId":"20170411200802.31638-2-git@jeffhostetler.com","threadId":"45668","inReplyTo":"20170411200802.31638-1-git@jeffhostetler.com","subject":"[PATCH v4 1/2] string-list: use ALLOC_GROW macro when reallocing string_list","fromName":"","fromEmail":"git@jeffhostetler.com","sentAt":"2017-04-11T20:08:01Z","receivedAt":"2017-04-11T20:08:22Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nUse ALLOC_GROW() macro when reallocing a string_list array\nrather than simply increasing it by 32. This is a performance\noptimization.\n\nDuring status on a very large repo and there are many changes,\na significant percentage of the total run time is spent\nreallocing the wt_status.changes array.\n\nThis change decreases the time in wt_status_collect_changes_worktree()\nfrom 125 seconds to 45 seconds on my very large repository.\n\nThis produced a modest gain on my 1M file artificial repo, but\nbroke even on linux.git.\n\nTest HEAD^^ HEAD\n---------------------------------------------------------------------------------------\n0005.2: read-tree status br_ballast (1000001) 8.29(5.62+2.62) 8.22(5.57+2.63) -0.8%\n\nSigned-off-by: Jeff Hostetler \n---\n string-list.c | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/string-list.c b/string-list.c\nindex 45016ad..003ca18 100644\n--- a/string-list.c\n+++ b/string-list.c\n@@ -41,10 +41,7 @@ static int add_entry(int insert_at, struct string_list *list, const char *string\n \tif (exact_match)\n \t\treturn -1 - index;\n \n-\tif (list->nr + 1 >= list->alloc) {\n-\t\tlist->alloc += 32;\n-\t\tREALLOC_ARRAY(list->items, list->alloc);\n-\t}\n+\tALLOC_GROW(list->items, list->nr+1, list->alloc);\n \tif (index < list->nr)\n \t\tmemmove(list->items + index + 1, list->items + index,\n \t\t\t\t(list->nr - index)\n-- \n2.9.3\n\n"},{"id":"316637","messageId":"20170411201141.dfe4l2rn2oqxqvap@sigill.intra.peff.net","threadId":"45668","inReplyTo":"20170411200802.31638-1-git@jeffhostetler.com","subject":"Re: [PATCH v4 0/2] string-list: use ALLOC_GROW macro when reallocing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-11T20:11:41Z","receivedAt":"2017-04-11T20:11:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 11, 2017 at 08:08:00PM +0000, git@jeffhostetler.com wrote:\n\n> From: Jeff Hostetler \n> \n> Version 4 greatly simplifies the p0005 perf test. It now uses an existing\n> repo -- either real-world or artificial from t/perf/repos/many-files.sh.\n> \n> Jeff Hostetler (2):\n> string-list: use ALLOC_GROW macro when reallocing string_list\n> p0005-status: time status on very large repo\n\nLooks fine, though we may want to re-order when applying to introduce\np0005 before we show its output in the commit message of the other.\ncommit.\n\n-Peff\n"},{"id":"316648","messageId":"20170411212955.GP8741@aiede.mtv.corp.google.com","threadId":"45668","inReplyTo":"20170411200802.31638-2-git@jeffhostetler.com","subject":"Re: [PATCH v4 1/2] string-list: use ALLOC_GROW macro when reallocing string_list","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-04-11T21:29:55Z","receivedAt":"2017-04-11T21:30:06Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"git@jeffhostetler.com wrote:\n\n> During status on a very large repo and there are many changes,\n> a significant percentage of the total run time is spent\n> reallocing the wt_status.changes array.\n>\n> This change decreases the time in wt_status_collect_changes_worktree()\n> from 125 seconds to 45 seconds on my very large repository.\n>\n> This produced a modest gain on my 1M file artificial repo, but\n> broke even on linux.git.\n>\n> Test HEAD^^ HEAD\n> ---------------------------------------------------------------------------------------\n> 0005.2: read-tree status br_ballast (1000001) 8.29(5.62+2.62) 8.22(5.57+2.63) -0.8%\n>\n> Signed-off-by: Jeff Hostetler \n> ---\n> string-list.c | 5 +----\n> 1 file changed, 1 insertion(+), 4 deletions(-)\n\nNice. After rebasing (or just squashing together) with the p0005\npatch as peff suggested,\n\nReviewed-by: Jonathan Nieder \n"},{"id":"316651","messageId":"20170411213523.GQ8741@aiede.mtv.corp.google.com","threadId":"45668","inReplyTo":"20170411200802.31638-3-git@jeffhostetler.com","subject":"Re: [PATCH v4 2/2] p0005-status: time status on very large repo","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-04-11T21:35:23Z","receivedAt":"2017-04-11T21:35:31Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff Hostetler wrote:\n\n> Signed-off-by: Jeff Hostetler \n\nUsually the commit message is a place to put some of the motivation\nbehind the patch or why I should want to apply it. In this example,\neverything is answered by the previous patch, which suggests that it\nwould be easier to understand if they were squashed into a single\npatch.\n\n[...]\n> +++ b/t/perf/p0005-status.sh\n> @@ -0,0 +1,51 @@\n> +#!/bin/sh\n> +##\n> +## This test measures the performance of various read-tree\n> +## and status operations. It is primarily interested in\n> +## the algorithmic costs of index operations and recursive\n> +## tree traversal -- and NOT disk I/O on thousands of files.\n\nNit: git usually uses a single # to start comments in shell scripts\n(and likewise below).\n\n[...]\n> +## If the test repo was generated by ./repos/many-files.sh\n> +## then we know something about the data shape and branches,\n> +## so we can isolate testing to the ballast-related commits\n> +## and setup sparse-checkout so we don't have to populate\n> +## the ballast files and directories.\n> +##\n> +## Otherwise, we make some general assumptions about the\n> +## repo and consider the entire history of the current\n> +## branch to be the ballast.\n> +\n> +git branch | grep p0006-ballast >/dev/null 2>&1\n> +synthetic=$?\n> +if test \"$synthetic\" = 0\n\nnit: no need to run a command and read $? when using the\ncommand directly from if would do:\n\n\tif git branch | grep p0006-ballast\n\nThis can be simplified further by using a lower-level command\nfor the test:\n\n\tif git rev-parse --verify refs/heads/p0006-ballast^{commit}\n\n> +then\n> + echo Assuming synthetic repo from many-files.sh\n> + git branch br_base master\n> + git branch br_ballast p0006-ballast\n> + echo '/*' >.git/info/sparse-checkout\n> + echo '!ballast/*' >>.git/info/sparse-checkout\n> + git config --local core.sparsecheckout 1\n> +else\n> + echo Assuming non-synthetic repo...\n> + git branch br_base $(git rev-list HEAD | tail -n 1)\n> + git branch br_ballast HEAD\n> +fi\n\nThis kind of setup instructions usually go in a test_expect_success\nblock so that the perf harness can detect whether they failed.\n\n> +\n> +test_expect_success 'setup' '\n> +\tgit checkout -q br_ballast\n\nnit: no need to use \"-q\" --- most output from test_expect_success\nis suppressed unless the script is run with \"-v\", at which point\nit becomes useful for debugging.\n\n> +'\n> +\n> +nr_files=$(git ls-files | wc -l)\n\nThis also can go in the test_expect_success block.\n\n> +\n> +test_perf \"read-tree status br_ballast ($nr_files)\" '\n> +\tgit read-tree HEAD &&\n> +\tgit status\n> +'\n\nLooks nice and simple. Thanks for writing it.\n\nHope that helps,\nJonathan\n"}]}