{"thread":{"id":"37061","subject":"[PATCH 2/5] Don't define away __attribute__ on gcc","startedAt":"2014-07-04T23:43:47Z","lastAt":"2014-07-07T22:15:07Z","messageCount":11,"participants":["Andi Kleen","Bert Wesarg","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"245417","messageId":"1404517432-25185-1-git-send-email-andi@firstfloor.org","threadId":"37061","inReplyTo":null,"subject":"Fix the bitrotted profile feedback build","fromName":"Andi Kleen","fromEmail":"andi@firstfloor.org","sentAt":"2014-07-04T23:43:47Z","receivedAt":"2014-07-04T23:43:47Z","isPatch":false,"sender":{"key":"andi@firstfloor.org","avatar":null},"body":"The profile feedback build support had bitrotted. This patchkit fixes\nit and improves it slightly by adding a new profile-fast target.\n\n-Andi\n"},{"id":"245416","messageId":"1404517432-25185-2-git-send-email-andi@firstfloor.org","threadId":"37061","inReplyTo":"1404517432-25185-1-git-send-email-andi@firstfloor.org","subject":"[PATCH 1/5] Use BASIC_FLAGS for profile feedback","fromName":"Andi Kleen","fromEmail":"andi@firstfloor.org","sentAt":"2014-07-04T23:43:48Z","receivedAt":"2014-07-04T23:43:48Z","isPatch":true,"sender":{"key":"andi@firstfloor.org","avatar":null},"body":"From: Andi Kleen \n\nUse BASIC_CFLAGS instead of CFLAGS to set up the profile feedback\noption in the Makefile.\n\nThis allows still overriding CFLAGS on the make command line\nwithout disabling profile feedback.\n\nSigned-off-by: Andi Kleen \n---\n Makefile | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 07ea105..a9770ac 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1552,13 +1552,13 @@ endif\n PROFILE_DIR := $(CURDIR)\n \n ifeq (\"$(PROFILE)\",\"GEN\")\n-\tCFLAGS += -fprofile-generate=$(PROFILE_DIR) -DNO_NORETURN=1\n+\tBASIC_CFLAGS += -fprofile-generate=$(PROFILE_DIR) -DNO_NORETURN=1\n \tEXTLIBS += -lgcov\n \texport CCACHE_DISABLE = t\n \tV = 1\n else\n ifneq (\"$(PROFILE)\",\"\")\n-\tCFLAGS += -fprofile-use=$(PROFILE_DIR) -fprofile-correction -DNO_NORETURN=1\n+\tBASIC_CFLAGS += -fprofile-use=$(PROFILE_DIR) -fprofile-correction -DNO_NORETURN=1\n \texport CCACHE_DISABLE = t\n \tV = 1\n endif\n-- \n2.0.1\n"},{"id":"245415","messageId":"1404517432-25185-3-git-send-email-andi@firstfloor.org","threadId":"37061","inReplyTo":"1404517432-25185-1-git-send-email-andi@firstfloor.org","subject":"[PATCH 2/5] Don't define away __attribute__ on gcc","fromName":"Andi Kleen","fromEmail":"andi@firstfloor.org","sentAt":"2014-07-04T23:43:49Z","receivedAt":"2014-07-04T23:43:49Z","isPatch":true,"sender":{"key":"andi@firstfloor.org","avatar":null},"body":"From: Andi Kleen \n\nProfile feedback sets -DNO_NORETURN, which causes the compat\nheader file to go into a default #else block. That #else\nblock defines away __attribute__(). Doing so causes all\nkinds of problems with the Linux and gcc system headers:\nin particular it makes the xmmintrin.h headers error out,\nbreaking the build.\n\nDon't define away __attribute__ when __GNUC__ is set.\n\nSigned-off-by: Andi Kleen \n---\n git-compat-util.h | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 96f5554..01e8695 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -291,10 +291,12 @@ extern char *gitbasename(char *);\n #else\n #define NORETURN\n #define NORETURN_PTR\n+#ifndef __GNUC__\n #ifndef __attribute__\n #define __attribute__(x)\n #endif\n #endif\n+#endif\n \n /* The sentinel attribute is valid from gcc version 4.0 */\n #if defined(__GNUC__) && (__GNUC__ >= 4)\n-- \n2.0.1\n"},{"id":"245419","messageId":"1404517432-25185-4-git-send-email-andi@firstfloor.org","threadId":"37061","inReplyTo":"1404517432-25185-1-git-send-email-andi@firstfloor.org","subject":"[PATCH 3/5] Run the perf test suite for profile feedback too","fromName":"Andi Kleen","fromEmail":"andi@firstfloor.org","sentAt":"2014-07-04T23:43:50Z","receivedAt":"2014-07-04T23:43:50Z","isPatch":true,"sender":{"key":"andi@firstfloor.org","avatar":null},"body":"From: Andi Kleen \n\nOpen: If the perf test suite is representative enough it may\nbe reasonable to only run that and skip the much longer full\ntest suite. Thoughts?\n\nSigned-off-by: Andi Kleen \n---\n Makefile | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/Makefile b/Makefile\nindex a9770ac..ba64be9 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1647,6 +1647,7 @@ ifeq ($(filter all,$(MAKECMDGOALS)),all)\n all:: profile-clean\n \t$(MAKE) PROFILE=GEN all\n \t$(MAKE) PROFILE=GEN -j1 test\n+\t$(MAKE) PROFILE=GEN -j1 perf\n endif\n endif\n \n-- \n2.0.1\n"},{"id":"245420","messageId":"1404517432-25185-5-git-send-email-andi@firstfloor.org","threadId":"37061","inReplyTo":"1404517432-25185-1-git-send-email-andi@firstfloor.org","subject":"[PATCH 4/5] Fix profile feedback with -jN and add profile-fast","fromName":"Andi Kleen","fromEmail":"andi@firstfloor.org","sentAt":"2014-07-04T23:43:51Z","receivedAt":"2014-07-04T23:43:51Z","isPatch":true,"sender":{"key":"andi@firstfloor.org","avatar":null},"body":"From: Andi Kleen \n\nProfile feedback always failed for me with -jN. The problem\nwas that there was no implicit ordering between the profile generate\nstage and the profile use stage. So some objects in the later stage\nwould be linked with profile generate objects, and fail due\nto the missing -lgcov.\n\nThis adds a new profile target that implicitely enforces the\ncorrect ordering by using submakes. Plus a profile-install target\nto also install. This is also nicer to type that PROFILE=...\n\nPlus I always run the performance test suite now for the full\nprofile run.\n\nIn addition I also added a profile-fast / profile-fast-install\ntarget the only runs the performance test suite instead of the\nwhole test suite. This significantly speeds up the profile build,\nwhich was totally dominated by test suite run time. However\nit may have less coverage of course.\n\nSigned-off-by: Andi Kleen \n---\n INSTALL | 14 ++++++++++++--\n Makefile | 21 +++++++++++++++++----\n 2 files changed, 29 insertions(+), 6 deletions(-)\n\ndiff --git a/INSTALL b/INSTALL\nindex ba01e74..6ec7a24 100644\n--- a/INSTALL\n+++ b/INSTALL\n@@ -28,7 +28,7 @@ set up install paths (via config.mak.autogen), so you can write instead\n If you're willing to trade off (much) longer build time for a later\n faster git you can also do a profile feedback build with\n \n-\t$ make prefix=/usr PROFILE=BUILD all\n+\t$ make prefix=/usr profile\n \t# make prefix=/usr PROFILE=BUILD install\n \n This will run the complete test suite as training workload and then\n@@ -36,10 +36,20 @@ rebuild git with the generated profile feedback. This results in a git\n which is a few percent faster on CPU intensive workloads. This\n may be a good tradeoff for distribution packagers.\n \n+Alternatively you can run profile feedback only with the git benchmark\n+suite. This runs significantly faster than the full test suite, but\n+has less coverage:\n+\n+\t$ make prefix=/usr profile-fast\n+\t# make prefix=/usr PROFILE=BUILD install\n+\n Or if you just want to install a profile-optimized version of git into\n your home directory, you could run:\n \n-\t$ make PROFILE=BUILD install\n+\t$ make profile-install\n+\n+or\n+\t$ make profile-fast-install\n \n As a caveat: a profile-optimized build takes a *lot* longer since the\n git tree must be built twice, and in order for the profiling\ndiff --git a/Makefile b/Makefile\nindex ba64be9..a760402 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1643,13 +1643,20 @@ SHELL = $(SHELL_PATH)\n all:: shell_compatibility_test\n \n ifeq \"$(PROFILE)\" \"BUILD\"\n-ifeq ($(filter all,$(MAKECMDGOALS)),all)\n-all:: profile-clean\n+all:: profile\n+endif\n+\n+profile:: profile-clean\n \t$(MAKE) PROFILE=GEN all\n \t$(MAKE) PROFILE=GEN -j1 test\n \t$(MAKE) PROFILE=GEN -j1 perf\n-endif\n-endif\n+\t$(MAKE) PROFILE=USE all\n+\n+profile-fast: profile-clean\n+\t$(MAKE) PROFILE=GEN all\n+\t$(MAKE) PROFILE=GEN -j1 perf\n+\t$(MAKE) PROFILE=USE all\n+\n \n all:: $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS\n ifneq (,$X)\n@@ -2336,6 +2343,12 @@ mergetools_instdir_SQ = $(subst ','\\'',$(mergetools_instdir))\n \n install_bindir_programs := $(patsubst %,%$X,$(BINDIR_PROGRAMS_NEED_X)) $(BINDIR_PROGRAMS_NO_X)\n \n+profile-install: profile\n+\t$(MAKE) install\n+\n+profile-fast-install: profile-fast\n+\t$(MAKE) install\n+\n install: all\n \t$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(bindir_SQ)'\n \t$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'\n-- \n2.0.1\n"},{"id":"245418","messageId":"1404517432-25185-6-git-send-email-andi@firstfloor.org","threadId":"37061","inReplyTo":"1404517432-25185-1-git-send-email-andi@firstfloor.org","subject":"[PATCH 5/5] Add a little script to compare two make perf runs","fromName":"Andi Kleen","fromEmail":"andi@firstfloor.org","sentAt":"2014-07-04T23:43:52Z","receivedAt":"2014-07-04T23:43:52Z","isPatch":true,"sender":{"key":"andi@firstfloor.org","avatar":null},"body":"From: Andi Kleen \n\nSigned-off-by: Andi Kleen \n---\n diff-res | 26 ++++++++++++++++++++++++++\n 1 file changed, 26 insertions(+)\n create mode 100755 diff-res\n\ndiff --git a/diff-res b/diff-res\nnew file mode 100755\nindex 0000000..90d57be\n--- /dev/null\n+++ b/diff-res\n@@ -0,0 +1,26 @@\n+#!/usr/bin/python\n+# compare two make perf output file\n+# this should be the results only without any header\n+import argparse\n+import math, operator\n+from collections import OrderedDict\n+\n+ap = argparse.ArgumentParser()\n+ap.add_argument('file1', type=argparse.FileType('r'))\n+ap.add_argument('file2', type=argparse.FileType('r'))\n+args = ap.parse_args()\n+\n+cmp = (OrderedDict(), OrderedDict())\n+for f, k in zip((args.file1, args.file2), cmp):\n+ for j in f:\n+ num = j[59:63]\n+ name = j[:59]\n+ k[name] = float(num)\n+\n+for j in cmp[0].keys():\n+ print j, cmp[1][j] - cmp[0][j]\n+\n+def geomean(l):\n+ return math.pow(reduce(operator.mul, filter(lambda x: x != 0.0, l)), 1.0 / len(l))\n+\n+print \"geomean %.2f -> %.2f\" % (geomean(cmp[0].values()), geomean(cmp[1].values()))\n-- \n2.0.1\n"},{"id":"245444","messageId":"CAKPyHN3rz+TUkcpAS3151XZo+zK2Un=LOrQ_A=TVo4QQ_EUsDg@mail.gmail.com","threadId":"37061","inReplyTo":"1404517432-25185-6-git-send-email-andi@firstfloor.org","subject":"Re: [PATCH 5/5] Add a little script to compare two make perf runs","fromName":"Bert Wesarg","fromEmail":"bert.wesarg@googlemail.com","sentAt":"2014-07-06T16:12:12Z","receivedAt":"2014-07-06T16:12:12Z","isPatch":true,"sender":{"key":"bert.wesarg@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/111934?v=4"},"body":"Hi,\n\nOn Sat, Jul 5, 2014 at 1:43 AM, Andi Kleen wrote:\n> From: Andi Kleen \n>\n> Signed-off-by: Andi Kleen \n> ---\n> diff-res | 26 ++++++++++++++++++++++++++\n> 1 file changed, 26 insertions(+)\n> create mode 100755 diff-res\n>\n> diff --git a/diff-res b/diff-res\n> new file mode 100755\n> index 0000000..90d57be\n> --- /dev/null\n> +++ b/diff-res\n> @@ -0,0 +1,26 @@\n> +#!/usr/bin/python\n> +# compare two make perf output file\n> +# this should be the results only without any header\n> +import argparse\n> +import math, operator\n> +from collections import OrderedDict\n> +\n> +ap = argparse.ArgumentParser()\n> +ap.add_argument('file1', type=argparse.FileType('r'))\n> +ap.add_argument('file2', type=argparse.FileType('r'))\n> +args = ap.parse_args()\n> +\n> +cmp = (OrderedDict(), OrderedDict())\n> +for f, k in zip((args.file1, args.file2), cmp):\n> + for j in f:\n> + num = j[59:63]\n> + name = j[:59]\n> + k[name] = float(num)\n> +\n> +for j in cmp[0].keys():\n> + print j, cmp[1][j] - cmp[0][j]\n> +\n> +def geomean(l):\n> + return math.pow(reduce(operator.mul, filter(lambda x: x != 0.0, l)), 1.0 / len(l))\n> +\n> +print \"geomean %.2f -> %.2f\" % (geomean(cmp[0].values()), geomean(cmp[1].values()))\n\na justification why the geometric mean is used here would increase my\nconfident significantly.\n\nIt calculates wrong values anyway iff there are zeros in the sampling set.\n\nThanks.\n\nBert\n"},{"id":"245445","messageId":"20140706161527.GU19781@tassilo.jf.intel.com","threadId":"37061","inReplyTo":"CAKPyHN3rz+TUkcpAS3151XZo+zK2Un=LOrQ_A=TVo4QQ_EUsDg@mail.gmail.com","subject":"Re: [PATCH 5/5] Add a little script to compare two make perf runs","fromName":"Andi Kleen","fromEmail":"ak@linux.intel.com","sentAt":"2014-07-06T16:15:27Z","receivedAt":"2014-07-06T16:15:27Z","isPatch":true,"sender":{"key":"ak@linux.intel.com","avatar":null},"body":"> a justification why the geometric mean is used here would increase my\n> confident significantly.\n\nIt's just a standard way to summarize sets of benchmarks. For example SPEC \nuses the same approach.\n\nAnyways the script is not essential to the rest of the profile feedback\nfeature. Just ignore it if it's controversal.\n\n-Andi\n"},{"id":"245446","messageId":"CAKPyHN37FtG9=1zoMM3FxDzn1eSjbH7WGX8FOfkPLk8HJ5Scxg@mail.gmail.com","threadId":"37061","inReplyTo":"20140706161527.GU19781@tassilo.jf.intel.com","subject":"Re: [PATCH 5/5] Add a little script to compare two make perf runs","fromName":"Bert Wesarg","fromEmail":"bert.wesarg@googlemail.com","sentAt":"2014-07-06T16:46:34Z","receivedAt":"2014-07-06T16:46:34Z","isPatch":true,"sender":{"key":"bert.wesarg@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/111934?v=4"},"body":"On Sun, Jul 6, 2014 at 6:15 PM, Andi Kleen wrote:\n>> a justification why the geometric mean is used here would increase my\n>> confident significantly.\n>\n> It's just a standard way to summarize sets of benchmarks. For example SPEC\n> uses the same approach.\n\nNo, SPEC would have calculated the geometric mean of the ratios\ncmp[1][j] / cmp[0][j]. And this should also only be used under the\nassumption that there is a multiplicative correlation.\n\nBert\n"},{"id":"245492","messageId":"xmqq1ttwdgjy.fsf@gitster.dls.corp.google.com","threadId":"37061","inReplyTo":"1404517432-25185-4-git-send-email-andi@firstfloor.org","subject":"Re: [PATCH 3/5] Run the perf test suite for profile feedback too","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-07T21:06:57Z","receivedAt":"2014-07-07T21:06:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andi Kleen writes:\n\n> From: Andi Kleen \n>\n> Open: If the perf test suite is representative enough it may\n> be reasonable to only run that and skip the much longer full\n> test suite. Thoughts?\n\nI do not think it right now is representative, nor it was meant to\nbecome so. The operations are those that people cared about and\ntuned, and hopefully it would cover stuff actual end users care\nabout in the real life, though.\n\n>\n> Signed-off-by: Andi Kleen \n> ---\n> Makefile | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/Makefile b/Makefile\n> index a9770ac..ba64be9 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1647,6 +1647,7 @@ ifeq ($(filter all,$(MAKECMDGOALS)),all)\n> all:: profile-clean\n> \t$(MAKE) PROFILE=GEN all\n> \t$(MAKE) PROFILE=GEN -j1 test\n> +\t$(MAKE) PROFILE=GEN -j1 perf\n> endif\n> endif\n"},{"id":"245497","messageId":"20140707221507.GV19781@tassilo.jf.intel.com","threadId":"37061","inReplyTo":"xmqq1ttwdgjy.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/5] Run the perf test suite for profile feedback too","fromName":"Andi Kleen","fromEmail":"ak@linux.intel.com","sentAt":"2014-07-07T22:15:07Z","receivedAt":"2014-07-07T22:15:07Z","isPatch":true,"sender":{"key":"ak@linux.intel.com","avatar":null},"body":"On Mon, Jul 07, 2014 at 02:06:57PM -0700, Junio C Hamano wrote:\n> Andi Kleen writes:\n> \n> > From: Andi Kleen \n> >\n> > Open: If the perf test suite is representative enough it may\n> > be reasonable to only run that and skip the much longer full\n> > test suite. Thoughts?\n> \n> I do not think it right now is representative, nor it was meant to\n> become so. The operations are those that people cared about and\n> tuned, and hopefully it would cover stuff actual end users care\n> about in the real life, though.\n\nI ended up answering the question by creating two separate \nmakefile targets in the next patch.\n\nprofile to run the full test suite and profile-fast to run\nonly the performance test. profile-fast as the name implies\nis a lot faster to build, and fast enough that it's not \nannoying.\n\nI'll remove the \"Open\"\n\n-Andi\n"}]}