On Tue, Oct 23 2018, Duy Nguyen wrote:
> On Tue, Oct 23, 2018 at 12:17 PM Ævar Arnfjörð Bjarmason > <ava...@gmail.com> wrote: >> >> >> On Tue, Oct 23 2018, Johannes Schindelin wrote: >> >> > Hi Ævar, >> > >> > On Mon, 22 Oct 2018, Ævar Arnfjörð Bjarmason wrote: >> > >> >> So I think the only reason to keep it [GETTEXT_POISON] compile-time is >> >> performance, but I don't think that matters. It's not like we're >> >> printing gigabytes of _() formatted output. Everything where formatting >> >> matters is plumbing which doesn't use this API. These messages are >> >> always for human consumption. >> > >> > Well, let's make sure that your impression is correct before going too >> > far. I, too, had the impression that gettext cannot possibly be expensive, >> > especifally in Git for Windows' settings, where we do not even ship >> > translations. Yet see the commit message of cc5e1bf99247 (gettext: avoid >> > initialization if the locale dir is not present, 2018-04-21): >> > >> > The runtime of a simple `git.exe version` call on Windows is >> > currently dominated by the gettext setup, adding a whopping ~150ms >> > to the ~210ms total. >> > >> > I would be in favor of your change to make this a runtime option, of >> > course, as long as it does not affect performance greatly (in particular >> > on Windows, where we fight an uphill battle to make Git faster). >> >> How expensive gettext() may or may not be isn't relevant to the >> GETTEXT_POISON compile-time option. > > If a user requests NO_GETTEXT, they should have zero (or near zero) > cost related to gettext. Which is true until now (the inline _() > version is expanded to pretty much no-op with the exception of NULL > string) I'm assuming by "until now" you mean until my RFC patch in https://public-inbox.org/git/875zxtd59e....@evledraar.gmail.com/ Yeah it's more expensive, but I don't see how it matters. We'd be nano-optimizing a thing that's never going to become a bottleneck. I.e. the patch is basically taking the commented-out codepath in this test program: #include <stdio.h> #include <stdlib.h> #include <string.h> int have_flag(void) { return 0; /* static int flag = -1; if (flag == -1) flag = strlen(getenv("GIT_TEST_FOO")); return flag; */ } int main(void) { while (1) { const char *out = have_flag() ? "foo" : "bar"; puts(out); } } When I test that on my laptop with gcc 8.2.0 I get ~230MB/s of output on both versions, i.e. where have_flag() can be trivially constant folded, and where we need to call getenv() for both of: GIT_TEST_FOO= ./main | pv >/dev/null GIT_TEST_FOO=1 ./main | pv >/dev/null We don't have any codepaths where we're calling gettext() to output more than a screenfull or two of output, so optimizing this doesn't make sense. >> The effect of what I'm suggesting here, and which my WIP patch in >> <875zxtd59e....@evledraar.gmail.com> implements is that we'd do a >> one-time getenv() for each process that prints a _() message that we >> aren't doing now, and for each message call a function that would check >> a boolean "are we in poison mode" static global variable. > > Just don't do the getenv() inside _() even if you just do it one time. > getenv() may invalidate whatever value from the previous call. I would > not want to find out my code broke because of an getenv() inside some > innocent _()... How would any code break? We have various getenv("GIT_TEST_*...") already that work the same way. Unless you set that in the environment the function is idempotent, and I don't see how anyone would do that accidentally. > And we should still respect NO_GETTEXT, not doing any extra work when > it's defined. This is not how any of our NO_* defines work. *Sometimes* defining them guarantees you do no extra work, but in other cases we've decided that bending over backwards with #ifdef in various places isn't worth it. E.g. grep_opt in grep.h is a good example of this. Even if you don't compile with PCRE you're pointlessly memzero-ing out members of the struct that'll never be used in your config, because it's easier to maintain the code that way (types always checked at compile-time). (And no, despite my involvement in PCRE I didn't introduce that particular pattern) For the reasons noted above I think NO_GETTEXT is another such case. Just like GIT_TEST_SPLIT_INDEX (added in your d6e3c181bc ("read-cache: force split index mode with GIT_TEST_SPLIT_INDEX", 2014-06-13)) etc.
