Re: [PATCH v1 0/4] Teach 'run' perf script to read config files
On Wed, Jul 26, 2017 at 05:58:09PM +0200, Christian Couder wrote: > Actually after taking another look at that, it looks like the following > happens: > > 1) the run script sources the original GIT-BUILD-OPTIONS file from > ../.. relative to its location > 2) a git version is built in "build/$rev" using GIT_PERF_MAKE_OPTS > which generates a new GIT-BUILD-OPTIONS file in "build/$rev/" > 3) when the actual perf scripts are run they source the original > GIT-BUILD-OPTIONS file (through perf-lib.sh which sources test-lib.sh) Right, the perf scripts are run in the context of the "outer" repository, and get their options from that one. I think that's intentional, and does the right thing for GIT_PERF_* options. It's possibly confusing if the tests really do want to know about the build options for a particular test-build (like asking if it was built with NO_PERL, for example). I think in practice it works out OK, because we tend to do test-builds that are similar to what's in the outer repo (because we copy config.mak, and don't tend to add a lot of command-line options). But if you put exotic options into your GIT_PERF_MAKE_OPTS, they won't be reflected in the config that the test scripts see. > I wonder how useful 1) is, as the variables sourced from original > GIT-BUILD-OPTIONS are not used inside the "run" script and not > available to its child processes as they are not exported. > Is it just so that if people add GIT_PERF_* variables to their > config.mak before building they can then have those variables used by > the run script? Exactly. I put GIT_PERF_MAKE_OPTS in my config.mak, and they end up respected for each run without me having to specify them manually. > I also wonder if it would be better at step 3) to source the > GIT-BUILD-OPTIONS file generated at step 2) instead of the original > one, because they can be different as the options in > $GIT_PERF_MAKE_OPTS will be baked into the new GIT-BUILD-OPTIONS file. > (Of course if $GIT_PERF_MAKE_OPTS was added to config.mak before > building, then they will be in the original one too. But > $GIT_PERF_MAKE_OPTS should work without that.) I think that would make some cases work (build options for the tested build), but I fear that it would break others (perf variables that probably should be coming from the "outer" layer). Remember that test builds may not be current versions and may not be forwarding those variables via GIT-BUILD-OPTIONS. I think it's important that the bundle of t/perf scripts act as a single unit that is driven primarily by the currently checked-out version (and it's up to those scripts to handle inconsistencies in old versions; see the $MODERN_GIT stuff I added a few months ago. Right now I don't think it has been a big problem, because the build config tends to be the same. But if we introduce more "properties" that the user can tweak for a certain test run, this distinction is probably going to cause more bugs. I'd almost say that the perf scripts should be a project outside of git.git entirely, to eliminate confusion. -Peff
Re: [PATCH v1 0/4] Teach 'run' perf script to read config files
On Fri, Jul 14, 2017 at 8:27 AM, Christian Couderwrote: > On Thu, Jul 13, 2017 at 10:55 PM, Jeff King wrote: >> On Thu, Jul 13, 2017 at 08:57:01PM +0200, Christian Couder wrote: >> >>> >> We want to make it possible to store the parameters to the 'run' >>> >> script in a config file. This will make it easier to store, reuse, >>> >> share and compare parameters. >>> > >>> > Because perf-lib is built on test-lib, it already reads >>> > GIT-BUILD-OPTIONS. >>> >>> Actually the 'run' script also sources GIT-BUILD-OPTIONS, so maybe >>> this is not necessary. >> >> Ah, right. The one that comes via perf-lib gets the variables into the >> test scripts themselves. But anything "run" would need itself would come >> from the source it does itself. And that's where GIT_PERF_MAKE_OPTS has >> an effect. >> >>> Also are the variables in GIT-BUILD-OPTIONS exported already? >> >> No, I don't think so. But because both "run" and the scripts themselves >> source them, they're available more or less everywhere, except for >> sub-processes inside the scripts. > > Ok, I see. Actually after taking another look at that, it looks like the following happens: 1) the run script sources the original GIT-BUILD-OPTIONS file from ../.. relative to its location 2) a git version is built in "build/$rev" using GIT_PERF_MAKE_OPTS which generates a new GIT-BUILD-OPTIONS file in "build/$rev/" 3) when the actual perf scripts are run they source the original GIT-BUILD-OPTIONS file (through perf-lib.sh which sources test-lib.sh) I wonder how useful 1) is, as the variables sourced from original GIT-BUILD-OPTIONS are not used inside the "run" script and not available to its child processes as they are not exported. Is it just so that if people add GIT_PERF_* variables to their config.mak before building they can then have those variables used by the run script? I also wonder if it would be better at step 3) to source the GIT-BUILD-OPTIONS file generated at step 2) instead of the original one, because they can be different as the options in $GIT_PERF_MAKE_OPTS will be baked into the new GIT-BUILD-OPTIONS file. (Of course if $GIT_PERF_MAKE_OPTS was added to config.mak before building, then they will be in the original one too. But $GIT_PERF_MAKE_OPTS should work without that.)
Re: [PATCH v1 0/4] Teach 'run' perf script to read config files
On Fri, Jul 14, 2017 at 08:27:53AM +0200, Christian Couder wrote: > The whole thing seems really complex to me though. And this makes me > think that people might want to specify different GIT-BUILD-OPTIONS > and config.mak files to be used when running perf tests, so that the > results from perf tests can easily be reproduced later even if they > have changed their build options in their development Git repo in the > meantime. I agree with the complexity. The general idea is that your currently built HEAD is a snapshot in time of options. But that doesn't have to be so, and laying out the options in a config file does seem like an improvement. There is another implicit dependency, though: the set of (and exact content of) the tests depends on your HEAD, too. So if I do: git checkout v2.5.0 cd t/perf ./run v2.0.0 v2.1.0 I might get different results if I replace "v2.5.0" in the first command with some other version, because the content of the tests will be different. I'm not sure how to account for that in storing results. Most of the time the version of the tests you ran is not going to be interesting. But it can be a source of confusing discrepancies if a test subtly changed between two runs. It probably happens infrequently enough that it's not worth worrying about. > So perhaps the config file should make it possible to specify a > directory where all the build files (GIT-BUILD-OPTIONS, config.mak, > config.mak.autogen and config.status) that should be used should be > taken. And then it could also let people change some variables to > override what is in those files which is needed to run perf tests with > different parameters. That sounds reasonable. I think you could ditch GIT-BUILD-OPTIONS entirely. It's only needed to pull in GIT_PERF variables that would be better served by being in the config in the first place. -Peff
Re: [PATCH v1 0/4] Teach 'run' perf script to read config files
On Thu, Jul 13, 2017 at 10:55 PM, Jeff Kingwrote: > On Thu, Jul 13, 2017 at 08:57:01PM +0200, Christian Couder wrote: > >> >> We want to make it possible to store the parameters to the 'run' >> >> script in a config file. This will make it easier to store, reuse, >> >> share and compare parameters. >> > >> > Because perf-lib is built on test-lib, it already reads >> > GIT-BUILD-OPTIONS. >> >> Actually the 'run' script also sources GIT-BUILD-OPTIONS, so maybe >> this is not necessary. > > Ah, right. The one that comes via perf-lib gets the variables into the > test scripts themselves. But anything "run" would need itself would come > from the source it does itself. And that's where GIT_PERF_MAKE_OPTS has > an effect. > >> Also are the variables in GIT-BUILD-OPTIONS exported already? > > No, I don't think so. But because both "run" and the scripts themselves > source them, they're available more or less everywhere, except for > sub-processes inside the scripts. Ok, I see. >> > And the Makefile copies several perf-related values >> > into it, including GIT_PERF_MAKE_OPTS and GIT_PERF_REPEAT_COUNT. So you >> > can already do: >> > >> > echo 'GIT_PERF_REPEAT_COUNT = 10' >>config.mak >> > echo 'GIT_PERF_MAKE_OPTS = CFLAGS="-O2" DEVELOPER=1' >>config.mak >> > make >> >> The "make" here might not even be needed as in the 'run' script >> "config.mak" is copied into the "build/$rev" directory where "make" is >> run to build the $rev version. > > You need it to bake the config into GIT-BUILD-OPTIONS, which is the only > thing that gets read by "run" and the perf scripts. If you are > just setting MAKE_OPTS to things that your config.mak already sets, then > yes, you can skip that one. Thanks for the explanations. The whole thing seems really complex to me though. And this makes me think that people might want to specify different GIT-BUILD-OPTIONS and config.mak files to be used when running perf tests, so that the results from perf tests can easily be reproduced later even if they have changed their build options in their development Git repo in the meantime. So perhaps the config file should make it possible to specify a directory where all the build files (GIT-BUILD-OPTIONS, config.mak, config.mak.autogen and config.status) that should be used should be taken. And then it could also let people change some variables to override what is in those files which is needed to run perf tests with different parameters.
Re: [PATCH v1 0/4] Teach 'run' perf script to read config files
On Thu, Jul 13, 2017 at 08:57:01PM +0200, Christian Couder wrote: > >> We want to make it possible to store the parameters to the 'run' > >> script in a config file. This will make it easier to store, reuse, > >> share and compare parameters. > > > > Because perf-lib is built on test-lib, it already reads > > GIT-BUILD-OPTIONS. > > Actually the 'run' script also sources GIT-BUILD-OPTIONS, so maybe > this is not necessary. Ah, right. The one that comes via perf-lib gets the variables into the test scripts themselves. But anything "run" would need itself would come from the source it does itself. And that's where GIT_PERF_MAKE_OPTS has an effect. > Also are the variables in GIT-BUILD-OPTIONS exported already? No, I don't think so. But because both "run" and the scripts themselves source them, they're available more or less everywhere, except for sub-processes inside the scripts. > > And the Makefile copies several perf-related values > > into it, including GIT_PERF_MAKE_OPTS and GIT_PERF_REPEAT_COUNT. So you > > can already do: > > > > echo 'GIT_PERF_REPEAT_COUNT = 10' >>config.mak > > echo 'GIT_PERF_MAKE_OPTS = CFLAGS="-O2" DEVELOPER=1' >>config.mak > > make > > The "make" here might not even be needed as in the 'run' script > "config.mak" is copied into the "build/$rev" directory where "make" is > run to build the $rev version. You need it to bake the config into GIT-BUILD-OPTIONS, which is the only thing that gets read by "run" and the perf scripts. If you are just setting MAKE_OPTS to things that your config.mak already sets, then yes, you can skip that one. -Peff
Re: [PATCH v1 0/4] Teach 'run' perf script to read config files
On Thu, Jul 13, 2017 at 8:40 PM, Jeff Kingwrote: > On Thu, Jul 13, 2017 at 11:29:10AM -0700, Junio C Hamano wrote: > >> > So then I think your config file primarily becomes about defining the >> > properties of each run. I'm not sure if it would look like what you're >> > starting on here or not. >> >> Yeah, I suspect that the final shape that defines the matrix might >> have to become quite a bit different. > > I think it would help if the perf code was split better into three > distinct bits: > > 1. A data-store capable of storing the run tuples along with their > outcomes for each test. > > 2. A "run" front-end that runs various profiles (based on config, > command-line options, etc) and writes the results to the data > store. > > 3. A flexible viewer which can slice and dice the contents of the data > store according to different parameters. > > We're almost there now. The "run" script actually does store results, > and you can view them via "aggregate.pl" without actually re-running the > tests. But the data store only indexes on one property: the tree that > was tested (and all of the other properties are ignored totally; you can > get some quite confusing results if you do a "./run" using say git.git > as your test repo, and then a followup with "linux.git"). Yeah I agree, but if possible I'd like to avoid working on the three different parts at the same time. I haven't thought much about how to improve the data store yet. I may have to look at that soon though. > I have to imagine that somebody else has written such a system already > that we could reuse. I don't know of one off-hand, but this is also not > an area where I've spent a lot of time. Actually about the viewer AEvar suggested having something like speed.python.org and speed.pypy.org which seem to be made using https://github.com/tobami/codespeed So unless something else is suggested, I plan to make it possible to import the results of the perf tests into codespeed, but I haven't looked at that much yet. > We're sort of drifting off topic from Christian's patches here. But if > we did have a third-party system, I suspect the interesting work would > be setting up profiles for the "run" tool to kick off. And we might be > stuck in such a case using whatever format the tool prefers. So having a > sense of what the final solution looks like might help us know whether > it makes sense to introduce a custom config format here. I don't think we should completely switch to a third-party system for everything. Though it would simplify my work if we decide to do that. I think people might want different viewers, so we should just make sure that we can easily massage the results from the run script, so that it will be easy to provide them as input to many different viewers. So we are pretty free to decide how we specify which tests should be performed on which revision, and I think a config file is the best way.
Re: [PATCH v1 0/4] Teach 'run' perf script to read config files
Jeff Kingwrites: > ... But if > we did have a third-party system, I suspect the interesting work would > be setting up profiles for the "run" tool to kick off. And we might be > stuck in such a case using whatever format the tool prefers. So having a > sense of what the final solution looks like might help us know whether > it makes sense to introduce a custom config format here. Agreed. Thanks.
Re: [PATCH v1 0/4] Teach 'run' perf script to read config files
On Thu, Jul 13, 2017 at 6:58 PM, Jeff Kingwrote: > On Thu, Jul 13, 2017 at 08:50:46AM +0200, Christian Couder wrote: > >> Goal >> >> >> Using many long environment variables to give parameters to the 'run' >> script is error prone and tiring. >> >> We want to make it possible to store the parameters to the 'run' >> script in a config file. This will make it easier to store, reuse, >> share and compare parameters. > > Because perf-lib is built on test-lib, it already reads > GIT-BUILD-OPTIONS. Actually the 'run' script also sources GIT-BUILD-OPTIONS, so maybe this is not necessary. Also are the variables in GIT-BUILD-OPTIONS exported already? > And the Makefile copies several perf-related values > into it, including GIT_PERF_MAKE_OPTS and GIT_PERF_REPEAT_COUNT. So you > can already do: > > echo 'GIT_PERF_REPEAT_COUNT = 10' >>config.mak > echo 'GIT_PERF_MAKE_OPTS = CFLAGS="-O2" DEVELOPER=1' >>config.mak > make The "make" here might not even be needed as in the 'run' script "config.mak" is copied into the "build/$rev" directory where "make" is run to build the $rev version. > cd t/perf > ./run > > I suspect there are still a lot of things that could be made easier with > a config file, so I'm not against the concept. Your example here: > >> [perf "with libpcre"] >> makeOpts = DEVELOPER=1 CFLAGS='-g -O0' USE_LIBPCRE=YesPlease >> [perf "without libpcre"] >> makeOpts = DEVELOPER=1 CFLAGS='-g -O0' > > is a lot more compelling. But right now the perf suite is not useful at > all for comparing two builds of the same tree. For that, I think it > would be more useful if we could define a tuple of parameters for a run. > One of which could be the tree we're testing. Build opts are another. > Tested repository is another. And then we'd fill in a table of results > and let you slice up the table by any column (e.g., compare times for > runs against a single tree but with differing build options). Yeah, improving the output part is another thing that I have discussed with AEvar and that I have planned to work on.
Re: [PATCH v1 0/4] Teach 'run' perf script to read config files
On Thu, Jul 13, 2017 at 11:29:10AM -0700, Junio C Hamano wrote: > > So then I think your config file primarily becomes about defining the > > properties of each run. I'm not sure if it would look like what you're > > starting on here or not. > > Yeah, I suspect that the final shape that defines the matrix might > have to become quite a bit different. I think it would help if the perf code was split better into three distinct bits: 1. A data-store capable of storing the run tuples along with their outcomes for each test. 2. A "run" front-end that runs various profiles (based on config, command-line options, etc) and writes the results to the data store. 3. A flexible viewer which can slice and dice the contents of the data store according to different parameters. We're almost there now. The "run" script actually does store results, and you can view them via "aggregate.pl" without actually re-running the tests. But the data store only indexes on one property: the tree that was tested (and all of the other properties are ignored totally; you can get some quite confusing results if you do a "./run" using say git.git as your test repo, and then a followup with "linux.git"). I have to imagine that somebody else has written such a system already that we could reuse. I don't know of one off-hand, but this is also not an area where I've spent a lot of time. We're sort of drifting off topic from Christian's patches here. But if we did have a third-party system, I suspect the interesting work would be setting up profiles for the "run" tool to kick off. And we might be stuck in such a case using whatever format the tool prefers. So having a sense of what the final solution looks like might help us know whether it makes sense to introduce a custom config format here. -Peff
Re: [PATCH v1 0/4] Teach 'run' perf script to read config files
Jeff Kingwrites: > Because perf-lib is built on test-lib, it already reads > GIT-BUILD-OPTIONS. And the Makefile copies several perf-related values > into it, including GIT_PERF_MAKE_OPTS and GIT_PERF_REPEAT_COUNT. So you > can already do: > ... > But right now the perf suite is not useful at > all for comparing two builds of the same tree. > For that, I think it > would be more useful if we could define a tuple of parameters for a run. > One of which could be the tree we're testing. Build opts are another. > Tested repository is another. And then we'd fill in a table of results > and let you slice up the table by any column (e.g., compare times for > runs against a single tree but with differing build options). Yeah, I think we saw this discussed in not-so-distant past, for which we want a good solution, and it might be the case that such a solution can be made easier to use with a separate configuration file (which this topic may or may not be used as a building block). > So then I think your config file primarily becomes about defining the > properties of each run. I'm not sure if it would look like what you're > starting on here or not. Yeah, I suspect that the final shape that defines the matrix might have to become quite a bit different.
Re: [PATCH v1 0/4] Teach 'run' perf script to read config files
On Thu, Jul 13, 2017 at 08:50:46AM +0200, Christian Couder wrote: > Goal > > > Using many long environment variables to give parameters to the 'run' > script is error prone and tiring. > > We want to make it possible to store the parameters to the 'run' > script in a config file. This will make it easier to store, reuse, > share and compare parameters. Because perf-lib is built on test-lib, it already reads GIT-BUILD-OPTIONS. And the Makefile copies several perf-related values into it, including GIT_PERF_MAKE_OPTS and GIT_PERF_REPEAT_COUNT. So you can already do: echo 'GIT_PERF_REPEAT_COUNT = 10' >>config.mak echo 'GIT_PERF_MAKE_OPTS = CFLAGS="-O2" DEVELOPER=1' >>config.mak make cd t/perf ./run I suspect there are still a lot of things that could be made easier with a config file, so I'm not against the concept. Your example here: > [perf "with libpcre"] > makeOpts = DEVELOPER=1 CFLAGS='-g -O0' USE_LIBPCRE=YesPlease > [perf "without libpcre"] > makeOpts = DEVELOPER=1 CFLAGS='-g -O0' is a lot more compelling. But right now the perf suite is not useful at all for comparing two builds of the same tree. For that, I think it would be more useful if we could define a tuple of parameters for a run. One of which could be the tree we're testing. Build opts are another. Tested repository is another. And then we'd fill in a table of results and let you slice up the table by any column (e.g., compare times for runs against a single tree but with differing build options). So then I think your config file primarily becomes about defining the properties of each run. I'm not sure if it would look like what you're starting on here or not. -Peff