{"thread":{"id":"46373","subject":"[PATCH v1 0/4] Teach 'run' perf script to read config files","startedAt":"2017-07-13T06:51:09Z","lastAt":"2017-07-26T16:54:34Z","messageCount":16,"participants":["Christian Couder","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"324337","messageId":"20170713065050.19215-1-chriscool@tuxfamily.org","threadId":"46373","inReplyTo":null,"subject":"[PATCH v1 0/4] Teach 'run' perf script to read config files","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-07-13T06:50:46Z","receivedAt":"2017-07-13T06:51:09Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Goal\n~~~~\n\nUsing many long environment variables to give parameters to the 'run'\nscript is error prone and tiring.\n\nWe want to make it possible to store the parameters to the 'run'\nscript in a config file. This will make it easier to store, reuse,\nshare and compare parameters.\n\nIn the future it could also make it easy to run series of tests. \n\nDesign\n~~~~~~\n\nWe use the same config format as the \".git/config\" file as Git users\nand developers are familiar with this nice format and have great tools\nto change, query and manage it.\n\nWe use values from the config file to set the environment variables\nthat are used by the scripts if they are not already set.\n\nHighlevel view of the patches in the series\n~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n\nPatch 1/4 teaches the '--config ' option to the 'run'\nscript, but is just put into the GIT_PERF_CONFIG_FILE\nvariable which is not used yet.\n\nPatch 2/4 add the get_var_from_env_or_config() function to read config\noptions from the and set values to some variables from\nthese config options or from default values.\n\nPatch 3/4 and 4/4 use the get_var_from_env_or_config() function to\nmake it possible to set parameters used by the 'run' script. \n\nFuture work\n~~~~~~~~~~~\n\nWe want to make it possible to run series of tests by passing only a\nconfig file to the 'run' script.\n\nFor example a config file like the following could be used to run perf\ntests with Git compiled both with and without libpcre:\n\n[perf]\n dirsOrRevs = v2.12.0 v2.13.0\n repeatCount = 10\n[perf \"with libpcre\"]\n makeOpts = DEVELOPER=1 CFLAGS='-g -O0' USE_LIBPCRE=YesPlease\n[perf \"without libpcre\"]\n makeOpts = DEVELOPER=1 CFLAGS='-g -O0'\n\nThis will make it easy to see what changes between the different runs.\n\nBut this can be added later, and the current series can already be\nuseful as is.\n\nLinks\n~~~~~\n\nThis patch series is also available here:\n\n https://github.com/chriscool/git/commits/perf-conf\n\n\nChristian Couder (4):\n perf/run: add '--config' option to the 'run' script\n perf/run: add get_var_from_env_or_config()\n perf/run: add GIT_PERF_DIRS_OR_REVS\n perf/run: add calls to get_var_from_env_or_config()\n\n t/perf/perf-lib.sh | 3 ---\n t/perf/run | 34 +++++++++++++++++++++++++++++++++-\n 2 files changed, 33 insertions(+), 4 deletions(-)\n\n-- \n2.13.2.647.g8b2efe2a0f\n\n"},{"id":"324338","messageId":"20170713065050.19215-3-chriscool@tuxfamily.org","threadId":"46373","inReplyTo":"20170713065050.19215-1-chriscool@tuxfamily.org","subject":"[PATCH v1 2/4] perf/run: add get_var_from_env_or_config()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-07-13T06:50:48Z","receivedAt":"2017-07-13T06:51:11Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Add get_var_from_env_or_config() to easily set variables\nfrom a config file if they are defined there and not already set.\n\nThis can also set them to a default value if one is provided.\n\nAs an example, use this function to set GIT_PERF_REPEAT_COUNT\nfrom the perf.repeatCount config option or from the default\nvalue.\n\nSigned-off-by: Christian Couder \n---\n t/perf/perf-lib.sh | 3 ---\n t/perf/run | 21 +++++++++++++++++++++\n 2 files changed, 21 insertions(+), 3 deletions(-)\n\ndiff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\nindex b50211b259..2f88fc12a9 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -59,9 +59,6 @@ perf_results_dir=$TEST_OUTPUT_DIRECTORY/test-results\n mkdir -p \"$perf_results_dir\"\n rm -f \"$perf_results_dir\"/$(basename \"$0\" .sh).subtests\n \n-if test -z \"$GIT_PERF_REPEAT_COUNT\"; then\n-\tGIT_PERF_REPEAT_COUNT=3\n-fi\n die_if_build_dir_not_repo () {\n \tif ! ( cd \"$TEST_DIRECTORY/..\" &&\n \t\t git rev-parse --build-dir >/dev/null 2>&1 ); then\ndiff --git a/t/perf/run b/t/perf/run\nindex 1e7c2a59e4..41580ac6df 100755\n--- a/t/perf/run\n+++ b/t/perf/run\n@@ -34,6 +34,7 @@ unpack_git_rev () {\n \t(cd \"$(git rev-parse --show-cdup)\" && git archive --format=tar $rev) |\n \t(cd build/$rev && tar x)\n }\n+\n build_git_rev () {\n \trev=$1\n \tfor config in config.mak config.mak.autogen config.status\n@@ -92,6 +93,26 @@ run_dirs () {\n \tdone\n }\n \n+get_var_from_env_or_config () {\n+\tenv_var=\"$1\"\n+\tconf_var=\"$2\"\n+\t# $3 can be set to a default value\n+\n+\t# Do nothing if the env variable is already set\n+\teval \"test -z \\\"\\${$env_var+x}\\\"\" || return\n+\n+\t# Check if the variable is in the config file\n+\ttest -n \"$GIT_PERF_CONFIG_FILE\" &&\n+\tconf_value=$(git config -f \"$GIT_PERF_CONFIG_FILE\" \"$conf_var\") &&\n+\teval \"$env_var=\\\"$conf_value\\\"\" || {\n+\t\ttest -n \"${3+x}\" &&\n+\t\teval \"$env_var=\\\"$3\\\"\"\n+\t}\n+}\n+\n+get_var_from_env_or_config \"GIT_PERF_REPEAT_COUNT\" \"perf.repeatCount\" 3\n+export GIT_PERF_REPEAT_COUNT\n+\n GIT_PERF_AGGREGATING_LATER=t\n export GIT_PERF_AGGREGATING_LATER\n \n-- \n2.13.2.647.g8b2efe2a0f\n\n"},{"id":"324339","messageId":"20170713065050.19215-2-chriscool@tuxfamily.org","threadId":"46373","inReplyTo":"20170713065050.19215-1-chriscool@tuxfamily.org","subject":"[PATCH v1 1/4] perf/run: add '--config' option to the 'run' script","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-07-13T06:50:47Z","receivedAt":"2017-07-13T06:51:13Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"It is error prone and tiring to use many long environment\nvariables to give parameters to the 'run' script.\n\nLet's make it easy to store some parameters in a config\nfile and to pass them to the run script.\n\nThe GIT_PERF_CONFIG_FILE variable will be set to the\nargument of the '--config' option. This variable is not\nused yet. It will be used in a following commit.\n\nSigned-off-by: Christian Couder \n---\n t/perf/run | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/t/perf/run b/t/perf/run\nindex beb4acc0e4..1e7c2a59e4 100755\n--- a/t/perf/run\n+++ b/t/perf/run\n@@ -2,9 +2,14 @@\n \n case \"$1\" in\n \t--help)\n-\t\techo \"usage: $0 [other_git_tree...] [--] [test_scripts]\"\n+\t\techo \"usage: $0 [--config file] [other_git_tree...] [--] [test_scripts]\"\n \t\texit 0\n \t\t;;\n+\t--config)\n+\t\tshift\n+\t\tGIT_PERF_CONFIG_FILE=$(cd \"$(dirname \"$1\")\"; pwd)/$(basename \"$1\")\n+\t\texport GIT_PERF_CONFIG_FILE\n+\t\tshift ;;\n esac\n \n die () {\n-- \n2.13.2.647.g8b2efe2a0f\n\n"},{"id":"324340","messageId":"20170713065050.19215-4-chriscool@tuxfamily.org","threadId":"46373","inReplyTo":"20170713065050.19215-1-chriscool@tuxfamily.org","subject":"[PATCH v1 3/4] perf/run: add GIT_PERF_DIRS_OR_REVS","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-07-13T06:50:49Z","receivedAt":"2017-07-13T06:51:14Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This environment variable can be set to some revisions or\ndirectories whose Git versions should be tested, in addition\nto the revisions or directories passed as arguments to the\n'run' script.\n\nThis enables a \"perf.dirsOrRevs\" configuration variable to\nbe used to set revisions or directories whose Git versions\nshould be tested.\n\nSigned-off-by: Christian Couder \n---\n t/perf/run | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/t/perf/run b/t/perf/run\nindex 41580ac6df..ad442fe64a 100755\n--- a/t/perf/run\n+++ b/t/perf/run\n@@ -113,6 +113,9 @@ get_var_from_env_or_config () {\n get_var_from_env_or_config \"GIT_PERF_REPEAT_COUNT\" \"perf.repeatCount\" 3\n export GIT_PERF_REPEAT_COUNT\n \n+get_var_from_env_or_config \"GIT_PERF_DIRS_OR_REVS\" \"perf.dirsOrRevs\"\n+set -- $GIT_PERF_DIRS_OR_REVS \"$@\"\n+\n GIT_PERF_AGGREGATING_LATER=t\n export GIT_PERF_AGGREGATING_LATER\n \n-- \n2.13.2.647.g8b2efe2a0f\n\n"},{"id":"324341","messageId":"20170713065050.19215-5-chriscool@tuxfamily.org","threadId":"46373","inReplyTo":"20170713065050.19215-1-chriscool@tuxfamily.org","subject":"[PATCH v1 4/4] perf/run: add calls to get_var_from_env_or_config()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-07-13T06:50:50Z","receivedAt":"2017-07-13T06:51:17Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"These calls make it possible to have the make command or the\nmake options in a config file, instead of in environment\nvariables.\n\nSigned-off-by: Christian Couder \n---\n t/perf/run | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/t/perf/run b/t/perf/run\nindex ad442fe64a..6bd15e7017 100755\n--- a/t/perf/run\n+++ b/t/perf/run\n@@ -116,6 +116,9 @@ export GIT_PERF_REPEAT_COUNT\n get_var_from_env_or_config \"GIT_PERF_DIRS_OR_REVS\" \"perf.dirsOrRevs\"\n set -- $GIT_PERF_DIRS_OR_REVS \"$@\"\n \n+get_var_from_env_or_config \"GIT_PERF_MAKE_COMMAND\" \"perf.makeCommand\"\n+get_var_from_env_or_config \"GIT_PERF_MAKE_OPTS\" \"perf.makeOpts\"\n+\n GIT_PERF_AGGREGATING_LATER=t\n export GIT_PERF_AGGREGATING_LATER\n \n-- \n2.13.2.647.g8b2efe2a0f\n\n"},{"id":"324375","messageId":"20170713165840.e5cdw7pa2m6haaen@sigill.intra.peff.net","threadId":"46373","inReplyTo":"20170713065050.19215-1-chriscool@tuxfamily.org","subject":"Re: [PATCH v1 0/4] Teach 'run' perf script to read config files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-13T16:58:41Z","receivedAt":"2017-07-13T16:58:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 13, 2017 at 08:50:46AM +0200, Christian Couder wrote:\n\n> Goal\n> ~~~~\n> \n> Using many long environment variables to give parameters to the 'run'\n> script is error prone and tiring.\n> \n> We want to make it possible to store the parameters to the 'run'\n> script in a config file. This will make it easier to store, reuse,\n> share and compare parameters.\n\nBecause perf-lib is built on test-lib, it already reads\nGIT-BUILD-OPTIONS. And the Makefile copies several perf-related values\ninto it, including GIT_PERF_MAKE_OPTS and GIT_PERF_REPEAT_COUNT. So you\ncan already do:\n\n echo 'GIT_PERF_REPEAT_COUNT = 10' >>config.mak\n echo 'GIT_PERF_MAKE_OPTS = CFLAGS=\"-O2\" DEVELOPER=1' >>config.mak\n make\n cd t/perf\n ./run \n\nI suspect there are still a lot of things that could be made easier with\na config file, so I'm not against the concept. Your example here:\n\n> [perf \"with libpcre\"]\n> makeOpts = DEVELOPER=1 CFLAGS='-g -O0' USE_LIBPCRE=YesPlease\n> [perf \"without libpcre\"]\n> makeOpts = DEVELOPER=1 CFLAGS='-g -O0'\n\nis a lot more compelling. But right now the perf suite is not useful at\nall for comparing two builds of the same tree. For that, I think it\nwould be more useful if we could define a tuple of parameters for a run.\nOne of which could be the tree we're testing. Build opts are another.\nTested repository is another. And then we'd fill in a table of results\nand let you slice up the table by any column (e.g., compare times for\nruns against a single tree but with differing build options).\n\nSo then I think your config file primarily becomes about defining the\nproperties of each run. I'm not sure if it would look like what you're\nstarting on here or not.\n\n-Peff\n"},{"id":"324405","messageId":"xmqqeftkxkax.fsf@gitster.mtv.corp.google.com","threadId":"46373","inReplyTo":"20170713165840.e5cdw7pa2m6haaen@sigill.intra.peff.net","subject":"Re: [PATCH v1 0/4] Teach 'run' perf script to read config files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-13T18:29:10Z","receivedAt":"2017-07-13T18:29:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Because perf-lib is built on test-lib, it already reads\n> GIT-BUILD-OPTIONS. And the Makefile copies several perf-related values\n> into it, including GIT_PERF_MAKE_OPTS and GIT_PERF_REPEAT_COUNT. So you\n> can already do:\n> ...\n> But right now the perf suite is not useful at\n> all for comparing two builds of the same tree.\n> For that, I think it\n> would be more useful if we could define a tuple of parameters for a run.\n> One of which could be the tree we're testing. Build opts are another.\n> Tested repository is another. And then we'd fill in a table of results\n> and let you slice up the table by any column (e.g., compare times for\n> runs against a single tree but with differing build options).\n\nYeah, I think we saw this discussed in not-so-distant past, for\nwhich we want a good solution, and it might be the case that such a\nsolution can be made easier to use with a separate configuration\nfile (which this topic may or may not be used as a building block).\n\n> So then I think your config file primarily becomes about defining the\n> properties of each run. I'm not sure if it would look like what you're\n> starting on here or not.\n\nYeah, I suspect that the final shape that defines the matrix might\nhave to become quite a bit different.\n"},{"id":"324408","messageId":"20170713184051.fcwg76o6ovnsjjbm@sigill.intra.peff.net","threadId":"46373","inReplyTo":"xmqqeftkxkax.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1 0/4] Teach 'run' perf script to read config files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-13T18:40:52Z","receivedAt":"2017-07-13T18:41:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 13, 2017 at 11:29:10AM -0700, Junio C Hamano wrote:\n\n> > So then I think your config file primarily becomes about defining the\n> > properties of each run. I'm not sure if it would look like what you're\n> > starting on here or not.\n> \n> Yeah, I suspect that the final shape that defines the matrix might\n> have to become quite a bit different.\n\nI think it would help if the perf code was split better into three\ndistinct bits:\n\n 1. A data-store capable of storing the run tuples along with their\n outcomes for each test.\n\n 2. A \"run\" front-end that runs various profiles (based on config,\n command-line options, etc) and writes the results to the data\n store.\n\n 3. A flexible viewer which can slice and dice the contents of the data\n store according to different parameters.\n\nWe're almost there now. The \"run\" script actually does store results,\nand you can view them via \"aggregate.pl\" without actually re-running the\ntests. But the data store only indexes on one property: the tree that\nwas tested (and all of the other properties are ignored totally; you can\nget some quite confusing results if you do a \"./run\" using say git.git\nas your test repo, and then a followup with \"linux.git\").\n\nI have to imagine that somebody else has written such a system already\nthat we could reuse. I don't know of one off-hand, but this is also not\nan area where I've spent a lot of time.\n\nWe're sort of drifting off topic from Christian's patches here. But if\nwe did have a third-party system, I suspect the interesting work would\nbe setting up profiles for the \"run\" tool to kick off. And we might be\nstuck in such a case using whatever format the tool prefers. So having a\nsense of what the final solution looks like might help us know whether\nit makes sense to introduce a custom config format here.\n\n-Peff\n"},{"id":"324412","messageId":"CAP8UFD3VnpMuMpcfRcTwL4nRpOF5URj6zsQqiEWYwo=1pi5Phw@mail.gmail.com","threadId":"46373","inReplyTo":"20170713165840.e5cdw7pa2m6haaen@sigill.intra.peff.net","subject":"Re: [PATCH v1 0/4] Teach 'run' perf script to read config files","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-07-13T18:57:01Z","receivedAt":"2017-07-13T18:57:07Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Jul 13, 2017 at 6:58 PM, Jeff King wrote:\n> On Thu, Jul 13, 2017 at 08:50:46AM +0200, Christian Couder wrote:\n>\n>> Goal\n>> ~~~~\n>>\n>> Using many long environment variables to give parameters to the 'run'\n>> script is error prone and tiring.\n>>\n>> We want to make it possible to store the parameters to the 'run'\n>> script in a config file. This will make it easier to store, reuse,\n>> share and compare parameters.\n>\n> Because perf-lib is built on test-lib, it already reads\n> GIT-BUILD-OPTIONS.\n\nActually the 'run' script also sources GIT-BUILD-OPTIONS, so maybe\nthis is not necessary.\nAlso are the variables in GIT-BUILD-OPTIONS exported already?\n\n> And the Makefile copies several perf-related values\n> into it, including GIT_PERF_MAKE_OPTS and GIT_PERF_REPEAT_COUNT. So you\n> can already do:\n>\n> echo 'GIT_PERF_REPEAT_COUNT = 10' >>config.mak\n> echo 'GIT_PERF_MAKE_OPTS = CFLAGS=\"-O2\" DEVELOPER=1' >>config.mak\n> make\n\nThe \"make\" here might not even be needed as in the 'run' script\n\"config.mak\" is copied into the \"build/$rev\" directory where \"make\" is\nrun to build the $rev version.\n\n> cd t/perf\n> ./run \n>\n> I suspect there are still a lot of things that could be made easier with\n> a config file, so I'm not against the concept. Your example here:\n>\n>> [perf \"with libpcre\"]\n>> makeOpts = DEVELOPER=1 CFLAGS='-g -O0' USE_LIBPCRE=YesPlease\n>> [perf \"without libpcre\"]\n>> makeOpts = DEVELOPER=1 CFLAGS='-g -O0'\n>\n> is a lot more compelling. But right now the perf suite is not useful at\n> all for comparing two builds of the same tree. For that, I think it\n> would be more useful if we could define a tuple of parameters for a run.\n> One of which could be the tree we're testing. Build opts are another.\n> Tested repository is another. And then we'd fill in a table of results\n> and let you slice up the table by any column (e.g., compare times for\n> runs against a single tree but with differing build options).\n\nYeah, improving the output part is another thing that I have discussed\nwith AEvar and that I have planned to work on.\n"},{"id":"324418","messageId":"xmqqmv88w3bt.fsf@gitster.mtv.corp.google.com","threadId":"46373","inReplyTo":"20170713184051.fcwg76o6ovnsjjbm@sigill.intra.peff.net","subject":"Re: [PATCH v1 0/4] Teach 'run' perf script to read config files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-13T19:21:10Z","receivedAt":"2017-07-13T19:21:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ... But if\n> we did have a third-party system, I suspect the interesting work would\n> be setting up profiles for the \"run\" tool to kick off. And we might be\n> stuck in such a case using whatever format the tool prefers. So having a\n> sense of what the final solution looks like might help us know whether\n> it makes sense to introduce a custom config format here.\n\nAgreed.\n\nThanks.\n"},{"id":"324428","messageId":"CAP8UFD1ZfdV2+9Z6pjUvtxWitxdwquPjycWr+H-XR3Uw9pruBg@mail.gmail.com","threadId":"46373","inReplyTo":"20170713184051.fcwg76o6ovnsjjbm@sigill.intra.peff.net","subject":"Re: [PATCH v1 0/4] Teach 'run' perf script to read config files","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-07-13T19:45:22Z","receivedAt":"2017-07-13T19:45:29Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Jul 13, 2017 at 8:40 PM, Jeff King wrote:\n> On Thu, Jul 13, 2017 at 11:29:10AM -0700, Junio C Hamano wrote:\n>\n>> > So then I think your config file primarily becomes about defining the\n>> > properties of each run. I'm not sure if it would look like what you're\n>> > starting on here or not.\n>>\n>> Yeah, I suspect that the final shape that defines the matrix might\n>> have to become quite a bit different.\n>\n> I think it would help if the perf code was split better into three\n> distinct bits:\n>\n> 1. A data-store capable of storing the run tuples along with their\n> outcomes for each test.\n>\n> 2. A \"run\" front-end that runs various profiles (based on config,\n> command-line options, etc) and writes the results to the data\n> store.\n>\n> 3. A flexible viewer which can slice and dice the contents of the data\n> store according to different parameters.\n>\n> We're almost there now. The \"run\" script actually does store results,\n> and you can view them via \"aggregate.pl\" without actually re-running the\n> tests. But the data store only indexes on one property: the tree that\n> was tested (and all of the other properties are ignored totally; you can\n> get some quite confusing results if you do a \"./run\" using say git.git\n> as your test repo, and then a followup with \"linux.git\").\n\nYeah I agree, but if possible I'd like to avoid working on the three\ndifferent parts at the same time.\n\nI haven't thought much about how to improve the data store yet.\nI may have to look at that soon though.\n\n> I have to imagine that somebody else has written such a system already\n> that we could reuse. I don't know of one off-hand, but this is also not\n> an area where I've spent a lot of time.\n\nActually about the viewer AEvar suggested having something like\nspeed.python.org and speed.pypy.org which seem to be made using\nhttps://github.com/tobami/codespeed\n\nSo unless something else is suggested, I plan to make it possible to\nimport the results of the perf tests into codespeed, but I haven't\nlooked at that much yet.\n\n> We're sort of drifting off topic from Christian's patches here. But if\n> we did have a third-party system, I suspect the interesting work would\n> be setting up profiles for the \"run\" tool to kick off. And we might be\n> stuck in such a case using whatever format the tool prefers. So having a\n> sense of what the final solution looks like might help us know whether\n> it makes sense to introduce a custom config format here.\n\nI don't think we should completely switch to a third-party system for\neverything.\nThough it would simplify my work if we decide to do that.\n\nI think people might want different viewers, so we should just make\nsure that we can easily massage the results from the run script, so\nthat it will be easy to provide them as input to many different\nviewers.\n\nSo we are pretty free to decide how we specify which tests should be\nperformed on which revision, and I think a config file is the best\nway.\n"},{"id":"324447","messageId":"20170713205535.otzi3gjd63ubb2dm@sigill.intra.peff.net","threadId":"46373","inReplyTo":"CAP8UFD3VnpMuMpcfRcTwL4nRpOF5URj6zsQqiEWYwo=1pi5Phw@mail.gmail.com","subject":"Re: [PATCH v1 0/4] Teach 'run' perf script to read config files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-13T20:55:36Z","receivedAt":"2017-07-13T20:55:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 13, 2017 at 08:57:01PM +0200, Christian Couder wrote:\n\n> >> We want to make it possible to store the parameters to the 'run'\n> >> script in a config file. This will make it easier to store, reuse,\n> >> share and compare parameters.\n> >\n> > Because perf-lib is built on test-lib, it already reads\n> > GIT-BUILD-OPTIONS.\n> \n> Actually the 'run' script also sources GIT-BUILD-OPTIONS, so maybe\n> this is not necessary.\n\nAh, right. The one that comes via perf-lib gets the variables into the\ntest scripts themselves. But anything \"run\" would need itself would come\nfrom the source it does itself. And that's where GIT_PERF_MAKE_OPTS has\nan effect.\n\n> Also are the variables in GIT-BUILD-OPTIONS exported already?\n\nNo, I don't think so. But because both \"run\" and the scripts themselves\nsource them, they're available more or less everywhere, except for\nsub-processes inside the scripts.\n\n> > And the Makefile copies several perf-related values\n> > into it, including GIT_PERF_MAKE_OPTS and GIT_PERF_REPEAT_COUNT. So you\n> > can already do:\n> >\n> > echo 'GIT_PERF_REPEAT_COUNT = 10' >>config.mak\n> > echo 'GIT_PERF_MAKE_OPTS = CFLAGS=\"-O2\" DEVELOPER=1' >>config.mak\n> > make\n> \n> The \"make\" here might not even be needed as in the 'run' script\n> \"config.mak\" is copied into the \"build/$rev\" directory where \"make\" is\n> run to build the $rev version.\n\nYou need it to bake the config into GIT-BUILD-OPTIONS, which is the only\nthing that gets read by \"run\" and the perf scripts. If you are\njust setting MAKE_OPTS to things that your config.mak already sets, then\nyes, you can skip that one.\n\n-Peff\n"},{"id":"324490","messageId":"CAP8UFD1FXPtzHF1J5ZfcsmMiHrfXQOd=qEghJQp3eFYsVd-nxQ@mail.gmail.com","threadId":"46373","inReplyTo":"20170713205535.otzi3gjd63ubb2dm@sigill.intra.peff.net","subject":"Re: [PATCH v1 0/4] Teach 'run' perf script to read config files","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-07-14T06:27:53Z","receivedAt":"2017-07-14T06:28:00Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Jul 13, 2017 at 10:55 PM, Jeff King wrote:\n> On Thu, Jul 13, 2017 at 08:57:01PM +0200, Christian Couder wrote:\n>\n>> >> We want to make it possible to store the parameters to the 'run'\n>> >> script in a config file. This will make it easier to store, reuse,\n>> >> share and compare parameters.\n>> >\n>> > Because perf-lib is built on test-lib, it already reads\n>> > GIT-BUILD-OPTIONS.\n>>\n>> Actually the 'run' script also sources GIT-BUILD-OPTIONS, so maybe\n>> this is not necessary.\n>\n> Ah, right. The one that comes via perf-lib gets the variables into the\n> test scripts themselves. But anything \"run\" would need itself would come\n> from the source it does itself. And that's where GIT_PERF_MAKE_OPTS has\n> an effect.\n>\n>> Also are the variables in GIT-BUILD-OPTIONS exported already?\n>\n> No, I don't think so. But because both \"run\" and the scripts themselves\n> source them, they're available more or less everywhere, except for\n> sub-processes inside the scripts.\n\nOk, I see.\n\n>> > And the Makefile copies several perf-related values\n>> > into it, including GIT_PERF_MAKE_OPTS and GIT_PERF_REPEAT_COUNT. So you\n>> > can already do:\n>> >\n>> > echo 'GIT_PERF_REPEAT_COUNT = 10' >>config.mak\n>> > echo 'GIT_PERF_MAKE_OPTS = CFLAGS=\"-O2\" DEVELOPER=1' >>config.mak\n>> > make\n>>\n>> The \"make\" here might not even be needed as in the 'run' script\n>> \"config.mak\" is copied into the \"build/$rev\" directory where \"make\" is\n>> run to build the $rev version.\n>\n> You need it to bake the config into GIT-BUILD-OPTIONS, which is the only\n> thing that gets read by \"run\" and the perf scripts. If you are\n> just setting MAKE_OPTS to things that your config.mak already sets, then\n> yes, you can skip that one.\n\nThanks for the explanations.\n\nThe whole thing seems really complex to me though. And this makes me\nthink that people might want to specify different GIT-BUILD-OPTIONS\nand config.mak files to be used when running perf tests, so that the\nresults from perf tests can easily be reproduced later even if they\nhave changed their build options in their development Git repo in the\nmeantime.\n\nSo perhaps the config file should make it possible to specify a\ndirectory where all the build files (GIT-BUILD-OPTIONS, config.mak,\nconfig.mak.autogen and config.status) that should be used should be\ntaken. And then it could also let people change some variables to\noverride what is in those files which is needed to run perf tests with\ndifferent parameters.\n"},{"id":"324491","messageId":"20170714080544.3nzndfx3p2yaxmac@sigill.intra.peff.net","threadId":"46373","inReplyTo":"CAP8UFD1FXPtzHF1J5ZfcsmMiHrfXQOd=qEghJQp3eFYsVd-nxQ@mail.gmail.com","subject":"Re: [PATCH v1 0/4] Teach 'run' perf script to read config files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-14T08:05:44Z","receivedAt":"2017-07-14T08:06:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 14, 2017 at 08:27:53AM +0200, Christian Couder wrote:\n\n> The whole thing seems really complex to me though. And this makes me\n> think that people might want to specify different GIT-BUILD-OPTIONS\n> and config.mak files to be used when running perf tests, so that the\n> results from perf tests can easily be reproduced later even if they\n> have changed their build options in their development Git repo in the\n> meantime.\n\nI agree with the complexity. The general idea is that your currently\nbuilt HEAD is a snapshot in time of options. But that doesn't have to be\nso, and laying out the options in a config file does seem like an\nimprovement.\n\nThere is another implicit dependency, though: the set of (and exact\ncontent of) the tests depends on your HEAD, too. So if I do:\n\n git checkout v2.5.0\n cd t/perf\n ./run v2.0.0 v2.1.0\n\nI might get different results if I replace \"v2.5.0\" in the first command\nwith some other version, because the content of the tests will be\ndifferent. I'm not sure how to account for that in storing results. Most\nof the time the version of the tests you ran is not going to be\ninteresting. But it can be a source of confusing discrepancies if a test\nsubtly changed between two runs. It probably happens infrequently enough\nthat it's not worth worrying about.\n\n> So perhaps the config file should make it possible to specify a\n> directory where all the build files (GIT-BUILD-OPTIONS, config.mak,\n> config.mak.autogen and config.status) that should be used should be\n> taken. And then it could also let people change some variables to\n> override what is in those files which is needed to run perf tests with\n> different parameters.\n\nThat sounds reasonable. I think you could ditch GIT-BUILD-OPTIONS\nentirely. It's only needed to pull in GIT_PERF variables that would be\nbetter served by being in the config in the first place.\n\n-Peff\n"},{"id":"325128","messageId":"CAP8UFD2NNBN=6GHbQPjz19hQUb+k_43YZBKimaA=3M6m4RH7Tw@mail.gmail.com","threadId":"46373","inReplyTo":"CAP8UFD1FXPtzHF1J5ZfcsmMiHrfXQOd=qEghJQp3eFYsVd-nxQ@mail.gmail.com","subject":"Re: [PATCH v1 0/4] Teach 'run' perf script to read config files","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-07-26T15:58:09Z","receivedAt":"2017-07-26T16:00:35Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Jul 14, 2017 at 8:27 AM, Christian Couder\n wrote:\n> On Thu, Jul 13, 2017 at 10:55 PM, Jeff King wrote:\n>> On Thu, Jul 13, 2017 at 08:57:01PM +0200, Christian Couder wrote:\n>>\n>>> >> We want to make it possible to store the parameters to the 'run'\n>>> >> script in a config file. This will make it easier to store, reuse,\n>>> >> share and compare parameters.\n>>> >\n>>> > Because perf-lib is built on test-lib, it already reads\n>>> > GIT-BUILD-OPTIONS.\n>>>\n>>> Actually the 'run' script also sources GIT-BUILD-OPTIONS, so maybe\n>>> this is not necessary.\n>>\n>> Ah, right. The one that comes via perf-lib gets the variables into the\n>> test scripts themselves. But anything \"run\" would need itself would come\n>> from the source it does itself. And that's where GIT_PERF_MAKE_OPTS has\n>> an effect.\n>>\n>>> Also are the variables in GIT-BUILD-OPTIONS exported already?\n>>\n>> No, I don't think so. But because both \"run\" and the scripts themselves\n>> source them, they're available more or less everywhere, except for\n>> sub-processes inside the scripts.\n>\n> Ok, I see.\n\nActually after taking another look at that, it looks like the following happens:\n\n1) the run script sources the original GIT-BUILD-OPTIONS file from\n../.. relative to its location\n2) a git version is built in \"build/$rev\" using GIT_PERF_MAKE_OPTS\nwhich generates a new GIT-BUILD-OPTIONS file in \"build/$rev/\"\n3) when the actual perf scripts are run they source the original\nGIT-BUILD-OPTIONS file (through perf-lib.sh which sources test-lib.sh)\n\nI wonder how useful 1) is, as the variables sourced from original\nGIT-BUILD-OPTIONS are not used inside the \"run\" script and not\navailable to its child processes as they are not exported.\nIs it just so that if people add GIT_PERF_* variables to their\nconfig.mak before building they can then have those variables used by\nthe run script?\n\nI also wonder if it would be better at step 3) to source the\nGIT-BUILD-OPTIONS file generated at step 2) instead of the original\none, because they can be different as the options in\n$GIT_PERF_MAKE_OPTS will be baked into the new GIT-BUILD-OPTIONS file.\n(Of course if $GIT_PERF_MAKE_OPTS was added to config.mak before\nbuilding, then they will be in the original one too. But\n$GIT_PERF_MAKE_OPTS should work without that.)\n"},{"id":"325132","messageId":"20170726165427.xh6ykoxjdibfqasp@sigill.intra.peff.net","threadId":"46373","inReplyTo":"CAP8UFD2NNBN=6GHbQPjz19hQUb+k_43YZBKimaA=3M6m4RH7Tw@mail.gmail.com","subject":"Re: [PATCH v1 0/4] Teach 'run' perf script to read config files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-26T16:54:28Z","receivedAt":"2017-07-26T16:54:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 26, 2017 at 05:58:09PM +0200, Christian Couder wrote:\n\n> Actually after taking another look at that, it looks like the following happens:\n> \n> 1) the run script sources the original GIT-BUILD-OPTIONS file from\n> ../.. relative to its location\n> 2) a git version is built in \"build/$rev\" using GIT_PERF_MAKE_OPTS\n> which generates a new GIT-BUILD-OPTIONS file in \"build/$rev/\"\n> 3) when the actual perf scripts are run they source the original\n> GIT-BUILD-OPTIONS file (through perf-lib.sh which sources test-lib.sh)\n\nRight, the perf scripts are run in the context of the \"outer\"\nrepository, and get their options from that one. I think that's\nintentional, and does the right thing for GIT_PERF_* options. It's\npossibly confusing if the tests really do want to know about the build\noptions for a particular test-build (like asking if it was built with\nNO_PERL, for example).\n\nI think in practice it works out OK, because we tend to do test-builds\nthat are similar to what's in the outer repo (because we copy\nconfig.mak, and don't tend to add a lot of command-line options). But if\nyou put exotic options into your GIT_PERF_MAKE_OPTS, they won't be\nreflected in the config that the test scripts see.\n\n> I wonder how useful 1) is, as the variables sourced from original\n> GIT-BUILD-OPTIONS are not used inside the \"run\" script and not\n> available to its child processes as they are not exported.\n> Is it just so that if people add GIT_PERF_* variables to their\n> config.mak before building they can then have those variables used by\n> the run script?\n\nExactly. I put GIT_PERF_MAKE_OPTS in my config.mak, and they end up\nrespected for each run without me having to specify them manually.\n\n> I also wonder if it would be better at step 3) to source the\n> GIT-BUILD-OPTIONS file generated at step 2) instead of the original\n> one, because they can be different as the options in\n> $GIT_PERF_MAKE_OPTS will be baked into the new GIT-BUILD-OPTIONS file.\n> (Of course if $GIT_PERF_MAKE_OPTS was added to config.mak before\n> building, then they will be in the original one too. But\n> $GIT_PERF_MAKE_OPTS should work without that.)\n\nI think that would make some cases work (build options for the tested\nbuild), but I fear that it would break others (perf variables that\nprobably should be coming from the \"outer\" layer). Remember that test\nbuilds may not be current versions and may not be forwarding those\nvariables via GIT-BUILD-OPTIONS. I think it's important that the bundle\nof t/perf scripts act as a single unit that is driven primarily by the\ncurrently checked-out version (and it's up to those scripts to handle\ninconsistencies in old versions; see the $MODERN_GIT stuff I added a few\nmonths ago.\n\nRight now I don't think it has been a big problem, because the build\nconfig tends to be the same. But if we introduce more \"properties\" that\nthe user can tweak for a certain test run, this distinction is probably\ngoing to cause more bugs. I'd almost say that the perf scripts should be\na project outside of git.git entirely, to eliminate confusion.\n\n-Peff\n"}]}