Re: [PATCH] Small optimization across postgres (remove strlenduplicate usage) - Mailing list pgsql-hackers
| From | Ranier Vilela |
|---|---|
| Subject | Re: [PATCH] Small optimization across postgres (remove strlenduplicate usage) |
| Date | |
| Msg-id | CAEudQAqUKOyku5JtsGKiA+w_rgw-8AOFJnZjQVDrx=03K+zTJw@mail.gmail.com Whole thread Raw |
| In response to | Re: [PATCH] Small optimization across postgres (remove strlen duplicate usage) (Tom Lane <tgl@sss.pgh.pa.us>) |
| List | pgsql-hackers |
Em dom., 19 de abr. de 2020 às 18:38, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
Tomas Vondra <tomas.vondra@2ndquadrant.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.
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.
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.
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 */
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 */
Ranier Vilela
pgsql-hackers by date: