{"thread":{"id":"58265","subject":"[PATCH] tests: cache glibc version check","startedAt":"2022-08-04T13:38:32Z","lastAt":"2022-08-04T20:42:05Z","messageCount":6,"participants":["Phillip Wood via GitGitGadget","Phillip Wood","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"460618","messageId":"pull.1311.git.1659620305757.gitgitgadget@gmail.com","threadId":"58265","inReplyTo":null,"subject":"[PATCH] tests: cache glibc version check","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-04T13:38:25Z","receivedAt":"2022-08-04T13:38:32Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\n131b94a10a (\"test-lib.sh: Use GLIBC_TUNABLES instead of MALLOC_CHECK_\non glibc >= 2.34\", 2022-03-04) introduced a check for the version of\nglibc that is in use. This check is performed as part of\nsetup_malloc_check() which is called at least once for each test. As\nthe test involves forking `getconf` and `expr` cache the result and\nuse that within setup_malloc_check() to avoid forking these extra\nprocesses for each test.\n\nSigned-off-by: Phillip Wood \n---\n tests: cache glibc version check\n \n A recent discussion on the list[1] reminded me that this patch was\n waiting to be sent.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1311%2Fphillipwood%2Fwip%2Ftest-cache-glibc-tunables-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1311/phillipwood/wip/test-cache-glibc-tunables-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1311\n\n t/test-lib.sh | 11 ++++++++---\n 1 file changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 7726d1da88a..ad81c78fce7 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -557,14 +557,19 @@ then\n \t\t: nothing\n \t}\n else\n+\t_USE_GLIBC_TUNABLES=\n+\tif _GLIBC_VERSION=$(getconf GNU_LIBC_VERSION 2>/dev/null) &&\n+\t _GLIBC_VERSION=${_GLIBC_VERSION#\"glibc \"} &&\n+\t expr 2.34 \\<= \"$_GLIBC_VERSION\" >/dev/null\n+\tthen\n+\t\t_USE_GLIBC_TUNABLES=YesPlease\n+\tfi\n \tsetup_malloc_check () {\n \t\tlocal g\n \t\tlocal t\n \t\tMALLOC_CHECK_=3\tMALLOC_PERTURB_=165\n \t\texport MALLOC_CHECK_ MALLOC_PERTURB_\n-\t\tif _GLIBC_VERSION=$(getconf GNU_LIBC_VERSION 2>/dev/null) &&\n-\t\t _GLIBC_VERSION=${_GLIBC_VERSION#\"glibc \"} &&\n-\t\t expr 2.34 \\<= \"$_GLIBC_VERSION\" >/dev/null\n+\t\tif test -n \"$_USE_GLIBC_TUNABLES\"\n \t\tthen\n \t\t\tg=\n \t\t\tLD_PRELOAD=\"libc_malloc_debug.so.0\"\n\nbase-commit: 23b219f8e3f2adfb0441e135f0a880e6124f766c\n-- \ngitgitgadget\n"},{"id":"460619","messageId":"fd0dcbcd-87dd-57e8-f7ec-418d2b726309@gmail.com","threadId":"58265","inReplyTo":"pull.1311.git.1659620305757.gitgitgadget@gmail.com","subject":"Re: [PATCH] tests: cache glibc version check","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-08-04T14:26:44Z","receivedAt":"2022-08-04T14:27:03Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 04/08/2022 14:38, Phillip Wood via GitGitGadget wrote:\n> From: Phillip Wood \n> \n> 131b94a10a (\"test-lib.sh: Use GLIBC_TUNABLES instead of MALLOC_CHECK_\n> on glibc >= 2.34\", 2022-03-04) introduced a check for the version of\n> glibc that is in use. This check is performed as part of\n> setup_malloc_check() which is called at least once for each test. As\n> the test involves forking `getconf` and `expr` cache the result and\n> use that within setup_malloc_check() to avoid forking these extra\n> processes for each test.\n> \n> Signed-off-by: Phillip Wood \n> ---\n> tests: cache glibc version check\n> \n> A recent discussion on the list[1] reminded me that this patch was\n> waiting to be sent\n[1] https://lore.kernel.org/git/YuL7EotrIpnOn5BT@coredump.intra.peff.net/\n\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1311%2Fphillipwood%2Fwip%2Ftest-cache-glibc-tunables-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1311/phillipwood/wip/test-cache-glibc-tunables-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1311\n> \n> t/test-lib.sh | 11 ++++++++---\n> 1 file changed, 8 insertions(+), 3 deletions(-)\n> \n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 7726d1da88a..ad81c78fce7 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -557,14 +557,19 @@ then\n> \t\t: nothing\n> \t}\n> else\n> +\t_USE_GLIBC_TUNABLES=\n> +\tif _GLIBC_VERSION=$(getconf GNU_LIBC_VERSION 2>/dev/null) &&\n> +\t _GLIBC_VERSION=${_GLIBC_VERSION#\"glibc \"} &&\n> +\t expr 2.34 \\<= \"$_GLIBC_VERSION\" >/dev/null\n> +\tthen\n> +\t\t_USE_GLIBC_TUNABLES=YesPlease\n> +\tfi\n> \tsetup_malloc_check () {\n> \t\tlocal g\n> \t\tlocal t\n> \t\tMALLOC_CHECK_=3\tMALLOC_PERTURB_=165\n> \t\texport MALLOC_CHECK_ MALLOC_PERTURB_\n> -\t\tif _GLIBC_VERSION=$(getconf GNU_LIBC_VERSION 2>/dev/null) &&\n> -\t\t _GLIBC_VERSION=${_GLIBC_VERSION#\"glibc \"} &&\n> -\t\t expr 2.34 \\<= \"$_GLIBC_VERSION\" >/dev/null\n> +\t\tif test -n \"$_USE_GLIBC_TUNABLES\"\n> \t\tthen\n> \t\t\tg=\n> \t\t\tLD_PRELOAD=\"libc_malloc_debug.so.0\"\n> \n> base-commit: 23b219f8e3f2adfb0441e135f0a880e6124f766c\n"},{"id":"460652","messageId":"xmqq4jyr6fuz.fsf@gitster.g","threadId":"58265","inReplyTo":"pull.1311.git.1659620305757.gitgitgadget@gmail.com","subject":"Re: [PATCH] tests: cache glibc version check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-04T18:08:52Z","receivedAt":"2022-08-04T18:09:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" writes:\n\n> From: Phillip Wood \n>\n> 131b94a10a (\"test-lib.sh: Use GLIBC_TUNABLES instead of MALLOC_CHECK_\n> on glibc >= 2.34\", 2022-03-04) introduced a check for the version of\n> glibc that is in use. This check is performed as part of\n> setup_malloc_check() which is called at least once for each test. As\n> the test involves forking `getconf` and `expr` cache the result and\n> use that within setup_malloc_check() to avoid forking these extra\n> processes for each test.\n>\n> Signed-off-by: Phillip Wood \n> ---\n> tests: cache glibc version check\n> \n> A recent discussion on the list[1] reminded me that this patch was\n> waiting to be sent.\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1311%2Fphillipwood%2Fwip%2Ftest-cache-glibc-tunables-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1311/phillipwood/wip/test-cache-glibc-tunables-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1311\n>\n> t/test-lib.sh | 11 ++++++++---\n> 1 file changed, 8 insertions(+), 3 deletions(-)\n>\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 7726d1da88a..ad81c78fce7 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -557,14 +557,19 @@ then\n> \t\t: nothing\n> \t}\n> else\n> +\t_USE_GLIBC_TUNABLES=\n> +\tif _GLIBC_VERSION=$(getconf GNU_LIBC_VERSION 2>/dev/null) &&\n> +\t _GLIBC_VERSION=${_GLIBC_VERSION#\"glibc \"} &&\n> +\t expr 2.34 \\<= \"$_GLIBC_VERSION\" >/dev/null\n> +\tthen\n> +\t\t_USE_GLIBC_TUNABLES=YesPlease\n> +\tfi\n> \tsetup_malloc_check () {\n> \t\tlocal g\n> \t\tlocal t\n> \t\tMALLOC_CHECK_=3\tMALLOC_PERTURB_=165\n> \t\texport MALLOC_CHECK_ MALLOC_PERTURB_\n> -\t\tif _GLIBC_VERSION=$(getconf GNU_LIBC_VERSION 2>/dev/null) &&\n> -\t\t _GLIBC_VERSION=${_GLIBC_VERSION#\"glibc \"} &&\n> -\t\t expr 2.34 \\<= \"$_GLIBC_VERSION\" >/dev/null\n> +\t\tif test -n \"$_USE_GLIBC_TUNABLES\"\n> \t\tthen\n> \t\t\tg=\n> \t\t\tLD_PRELOAD=\"libc_malloc_debug.so.0\"\n\nBetween USE_LIBC_MALLOC_DEBUG, which is the name Peff originally\ngave this intermediate variable, and the one you use here, I am\nundecided. If the only thing the GLIBC_TUNABLES mechanism can do\nwere to tweak the malloc checking, then both names are good, but\nthat is not the case. We are only seeing if we are going to use the\nmalloc check feature given by glibc here, so the original name feels\nmore to the point, and use of GLIBC_TUNABLE mechanism to trigger\nthat malloc check feature is a mere implementation detail.\n\nBut that is minor. Let's queue the patch to help me not to forget\nabout it, and we'll amend it if necessary, as we'd probably need a\nhelped-by or signed-off-by from Peff anyway before this hits 'next'.\n\nThanks.\n"},{"id":"460655","messageId":"53cffc06-ebb1-ac97-ceb4-4c242a82c68a@gmail.com","threadId":"58265","inReplyTo":"xmqq4jyr6fuz.fsf@gitster.g","subject":"Re: [PATCH] tests: cache glibc version check","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-08-04T19:16:12Z","receivedAt":"2022-08-04T19:16:20Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 04/08/2022 19:08, Junio C Hamano wrote:\n> \"Phillip Wood via GitGitGadget\" writes:\n >\n> Between USE_LIBC_MALLOC_DEBUG, which is the name Peff originally\n> gave this intermediate variable, and the one you use here, I am\n> undecided. If the only thing the GLIBC_TUNABLES mechanism can do\n> were to tweak the malloc checking, then both names are good, but\n> that is not the case. We are only seeing if we are going to use the\n> malloc check feature given by glibc here, so the original name feels\n> more to the point, and use of GLIBC_TUNABLE mechanism to trigger\n> that malloc check feature is a mere implementation detail.\n> \n> But that is minor. Let's queue the patch to help me not to forget\n> about it, and we'll amend it if necessary, as we'd probably need a\n> helped-by or signed-off-by from Peff anyway before this hits 'next'.\n\nOh, sorry I'd missed that message where Peff posted essentially the same \npatch. I wrote this at the same time as 067109a5e7 (\"tests: make \nSANITIZE=address imply TEST_NO_MALLOC_CHECK\", 2022-04-09) but did not \npost in then as we were in a rc period and then forgot about it. Having \njust read Peff's message this does not make much difference to the test \ntimings and if I'd seen that before I wouldn't have posted this.\n\nAs for the variable name I don't mind particularly either way, I chose \nthis name as the variable is checking whether we should use the glibc \ntunables mechanism or not.\n\nBest Wishes\n\nPhillip\n\n> Thanks.\n"},{"id":"460660","messageId":"xmqqh72r4v6g.fsf@gitster.g","threadId":"58265","inReplyTo":"53cffc06-ebb1-ac97-ceb4-4c242a82c68a@gmail.com","subject":"Re: [PATCH] tests: cache glibc version check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-04T20:20:55Z","receivedAt":"2022-08-04T20:21:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> it. Having just read Peff's message this does not make much difference\n> to the test timings and if I'd seen that before I wouldn't have posted\n> this.\n\nAh, I forgot about that conclusion, i.e. while this does look the\nright thing to do, it didn't make much difference in practice.\n\nIt still is tempting to take it, though :-), if only to stop people\nsending in the same patch in the future without benchmarking, as\nlong as it does not make it worse.\n\nThanks.\n"},{"id":"460663","messageId":"xmqqczdf4u7d.fsf@gitster.g","threadId":"58265","inReplyTo":"53cffc06-ebb1-ac97-ceb4-4c242a82c68a@gmail.com","subject":"Re: [PATCH] tests: cache glibc version check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-04T20:41:58Z","receivedAt":"2022-08-04T20:42:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> As for the variable name I don't mind particularly either way, I chose\n> this name as the variable is checking whether we should use the glibc\n> tunables mechanism or not.\n\nYup, but that becomes awkward when we decide to use the tunabules\nmechanism for something other than malloc debugging, and that is\nwhere my \"is this the right name?\" comes from.\n\nThanks.\n"}]}