Re: [PATCH] diff: use strncmp to check for "diff." prefix

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 19, 2022, 18:26 UTC
Message-ID: <221219.86r0wvxmyy.gmgdl@evledraar.gmail.com>
In-Reply-To: <pull.1403.git.git.1671467624143.gitgitgadget@gmail.com>

On Mon, Dec 19 2022, Rose via GitGitGadget wrote:

Show 13 quoted lines

> From: Seija Kijin <doremylover123@gmail.com>
>
> This would be a lot more efficient than
> checking for the full string, as
> so many of the accepted values
> begin with "diff."
>
> This allows the computer to skip most checks
> if var did not start with diff at all.
>
> It would also let us add more "diff." strings
> in the future without having to duplicate
> these 5 character so many times.

This is missing a "why", is this one-off really needed for optimization purposes? If not it doesn't seem worth the churn.

That we duplicate the "diff." part of "diff.renames" (as opposed to "renames" is also a feature in my opinion, it makes things easier to grep for.

It would really just be preferrable to have the compiler do this work for us. A while ago I experimented with that, i.e. if/else if/else chains of strcmp(): https://godbolt.org/z/rrnYWW7nj

As that example shows at least GCC doesn't do anything interesting with it as optimizations are concerned, i.e. you'd really want something like this instead (but for the compiler to do it for you): https://godbolt.org/z/7hsPo59TM

But, as you can be seen at https://godbolt.org/z/zWK46rGK5 it *will* do that if we just use memcmp().

So, doing that really seems preferrable, perhaps with some macro that would provide the "n" to memcmp, so we wouldn't need to hardcode the length.

But all of that would be predicated on whether the optimization is actually worth it.

If we are micro-optimizing config reading we should be moving the relevant code to the configset API, where we have the parsed keys as a hash, rather than trying to micro-optimize the callback API, where we'll call the callback N times, just to brute-force ask it if it's interested in some small subset of the config space.