Re: [PATCH] Small optimization across postgres (remove strlen duplicate usage)

Em dom., 19 de abr. de 2020 às 18:38, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> escreveu:

> Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com> writes:
> > On Sun, Apr 19, 2020 at 11:24:38AM -0300, Ranier Vilela wrote:
> >> strlen it is one of the low fruits that can be harvested.
>
> > Maybe there are places where this would help, but I don't see a reason
> > to just throw away all strlen calls and replace them with something
> > clearly less convenient and possibly more error-prone (I'd expect quite
> > a few off-by-one mistakes with this).
>
> I've heard it claimed that modern compilers will optimize
> strlen('literal') to a constant at compile time. I'm not sure how
> much I believe that, but to the extent that it's true, replacing such
> calls would provide exactly no performance benefit.
>
Tom, I wouldn't believe it very much, they still have a lot of stupid
compilers out there (msvc for example).
Furthermore, optimizations are a complicated business, often the adjacent
code does not allow for such optimizations.
When a programmer does, there is no doubt.

>
> I'm quite -1 on changing these to sizeof(), in any case, because
> (a) that opens up room for confusion about whether the trailing nul is
> included, and (b) it makes it very easy, when changing or copy/pasting
> code, to apply sizeof to something that's not a string literal, with
> disastrous results.
>
It may be true, but I have seen a lot of Postgres code, where sizeof is
used extensively, even with real chances of what you said happened. So that
risk already exists.

>
> The cases where Ranier proposes to replace strlen(foo) == 0
> with a test on foo[0] do seem like wins, though. Asking for
> the full string length to be computed is more computation than
> necessary, and it's less clear that the compiler could be
> expected to save you from that. Anyway there's a coding style
> proposition that we should be doing this consistently, and
> certainly lots of places do do this without using strlen().
>
Yes, this is the idea.

>
> I can't get excited about the proposed changes to optimize away
> multiple calls of strlen() either, unless there's performance
> measurements to support them individually. This again seems like
> something a compiler might do for you.
>
Again, the compiler will not always save us.
I have seen many fanstatic solutions in Postgres, but I have also seen a
lot of written code, forgive me, without caprice, without much care.
The idea is, little by little, to prevent carefree code, either written or
left in Postgres.

You can see an example in that patch.
After a few calls the programmer validates the entry and leaves if it is
bad. When it should be done before anything.

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 5bdc02fce2..a00cca2605 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -10699,10 +10699,15 @@ GUCArrayDelete(ArrayType *array, const char *name)
struct config_generic *record;
ArrayType *newarray;
int i;
+ int len;
int index;

Assert(name);

+ /* if array is currently null, then surely nothing to delete */
+ if (!array)
+ return NULL;
+
/* test if the option is valid and we're allowed to set it */
(void) validate_option_array_item(name, NULL, false);

@@ -10711,12 +10716,9 @@ GUCArrayDelete(ArrayType *array, const char *name)
if (record)
name = record->name;

- /* if array is currently null, then surely nothing to delete */
- if (!array)
- return NULL;
-
newarray = NULL;
index = 1;
+ len = strlen(name);

for (i = 1; i <= ARR_DIMS(array)[0]; i++)
{
@@ -10735,8 +10737,8 @@ GUCArrayDelete(ArrayType *array, const char *name)
val = TextDatumGetCString(d);

/* ignore entry if it's what we want to delete */
- if (strncmp(val, name, strlen(name)) == 0
- && val[strlen(name)] == '=')
+ if (strncmp(val, name, len) == 0
+ && val[len] == '=')
continue;

/* else add it to the output array */

regards,
Ranier Vilela