Re: [PATCH] Poison gettext with the Ook language

On Tue, Oct 23 2018, Duy Nguyen wrote:

On Tue, Oct 23, 2018 at 12:17 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

quoted

On Tue, Oct 23 2018, Johannes Schindelin wrote:

quoted

Hi Ævar,

On Mon, 22 Oct 2018, Ævar Arnfjörð Bjarmason wrote:

quoted

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.fsf@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.

quoted

The effect of what I'm suggesting here, and which my WIP patch in
[ref] 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.