Optimizing Node Files Support
Hi,
I believe that has room for improving generation node files.
The patch attached reduced the size of generated files by 27 kbytes.
From 891 kbytes to 864 kbytes.
About the patch:
1. Avoid useless attribution when from->field is NULL, once that
the new node is palloc0.
2. Avoid useless declaration variable Size, when it is unnecessary.
3. Optimize comparison functions like memcmp and strcmp, using
a short-cut comparison of the first element.
4. Switch several copy attributions like COPY_SCALAR_FIELD or
COPY_LOCATION_FIELD
by one memcpy call.
5. Avoid useless attribution, ignoring the result of pg_strtok when it is
unnecessary.
regards,
Ranier Vilela
Attachments:
optimize_gen_nodes_support.patchapplication/octet-stream; name=optimize_gen_nodes_support.patchDownload
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 84f5e2e6ad..4e06b1d54f 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -40,20 +40,22 @@
/* Copy a field that is a pointer to a C string, or perhaps NULL */
#define COPY_STRING_FIELD(fldname) \
- (newnode->fldname = from->fldname ? pstrdup(from->fldname) : (char *) NULL)
+ do { \
+ if (from->fldname) \
+ newnode->fldname = pstrdup(from->fldname); \
+ } while (0)
/* Copy a field that is an inline array */
#define COPY_ARRAY_FIELD(fldname) \
memcpy(newnode->fldname, from->fldname, sizeof(newnode->fldname))
/* Copy a field that is a pointer to a simple palloc'd object of size sz */
-#define COPY_POINTER_FIELD(fldname, sz) \
+#define COPY_POINTER_FIELD(fldname, num, sz) \
do { \
- Size _size = (sz); \
- if (_size > 0) \
+ if (num > 0) \
{ \
- newnode->fldname = palloc(_size); \
- memcpy(newnode->fldname, from->fldname, _size); \
+ newnode->fldname = palloc(num * sz); \
+ memcpy(newnode->fldname, from->fldname, num * sz); \
} \
} while (0)
@@ -74,10 +76,7 @@ _copyConst(const Const *from)
{
Const *newnode = makeNode(Const);
- COPY_SCALAR_FIELD(consttype);
- COPY_SCALAR_FIELD(consttypmod);
- COPY_SCALAR_FIELD(constcollid);
- COPY_SCALAR_FIELD(constlen);
+ memcpy(newnode, from, sizeof(*from));
if (from->constbyval || from->constisnull)
{
@@ -97,10 +96,6 @@ _copyConst(const Const *from)
from->constlen);
}
- COPY_SCALAR_FIELD(constisnull);
- COPY_SCALAR_FIELD(constbyval);
- COPY_LOCATION_FIELD(location);
-
return newnode;
}
@@ -176,8 +171,6 @@ _copyBitmapset(const Bitmapset *from)
void *
copyObjectImpl(const void *from)
{
- void *retval;
-
if (from == NULL)
return NULL;
@@ -189,7 +182,7 @@ copyObjectImpl(const void *from)
#include "copyfuncs.switch.c"
case T_List:
- retval = list_copy_deep(from);
+ return list_copy_deep(from);
break;
/*
@@ -199,14 +192,13 @@ copyObjectImpl(const void *from)
case T_IntList:
case T_OidList:
case T_XidList:
- retval = list_copy(from);
+ return list_copy(from);
break;
default:
elog(ERROR, "unrecognized node type: %d", (int) nodeTag(from));
- retval = 0; /* keep compiler quiet */
break;
}
- return retval;
+ return NULL;
}
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index b2f07da62e..12d6643132 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -60,19 +60,19 @@
/* Macro for comparing string fields that might be NULL */
#define equalstr(a, b) \
- (((a) != NULL && (b) != NULL) ? (strcmp(a, b) == 0) : (a) == (b))
+ (((a) != NULL && (b) != NULL) ? (a[0] == b[0] && strcmp(a, b) == 0) : (a) == (b))
/* Compare a field that is an inline array */
#define COMPARE_ARRAY_FIELD(fldname) \
do { \
- if (memcmp(a->fldname, b->fldname, sizeof(a->fldname)) != 0) \
+ if (a->fldname[0] != b->fldname[0] || memcmp(a->fldname, b->fldname, sizeof(a->fldname)) != 0) \
return false; \
} while (0)
/* Compare a field that is a pointer to a simple palloc'd object of size sz */
#define COMPARE_POINTER_FIELD(fldname, sz) \
do { \
- if (memcmp(a->fldname, b->fldname, (sz)) != 0) \
+ if (memcmp(a->fldname, b->fldname, sizeof(a->fldname)) != 0) \
return false; \
} while (0)
@@ -224,9 +224,7 @@ _equalList(const List *a, const List *b)
bool
equal(const void *a, const void *b)
{
- bool retval;
-
- if (a == b)
+ if (unlikely(a == b))
return true;
/*
@@ -252,15 +250,14 @@ equal(const void *a, const void *b)
case T_IntList:
case T_OidList:
case T_XidList:
- retval = _equalList(a, b);
+ return _equalList(a, b);
break;
default:
elog(ERROR, "unrecognized node type: %d",
(int) nodeTag(a));
- retval = false; /* keep compiler quiet */
break;
}
- return retval;
+ return false;
}
diff --git a/src/backend/nodes/gen_node_support.pl b/src/backend/nodes/gen_node_support.pl
index b6f086e262..c82af63a01 100644
--- a/src/backend/nodes/gen_node_support.pl
+++ b/src/backend/nodes/gen_node_support.pl
@@ -656,12 +656,12 @@ foreach my $n (@node_types)
next if $struct_no_copy && $struct_no_equal;
print $cfs "\t\tcase T_${n}:\n"
- . "\t\t\tretval = _copy${n}(from);\n"
+ . "\t\t\treturn _copy${n}(from);\n"
. "\t\t\tbreak;\n"
unless $struct_no_copy;
print $efs "\t\tcase T_${n}:\n"
- . "\t\t\tretval = _equal${n}(a, b);\n"
+ . "\t\t\treturn _equal${n}(a, b);\n"
. "\t\t\tbreak;\n"
unless $struct_no_equal;
@@ -680,6 +680,7 @@ static bool
_equal${n}(const $n *a, const $n *b)
{
" unless $struct_no_equal;
+ my $memcpy_ignore = 0;
# print instructions for each field
foreach my $f (@{ $node_type_info{$n}->{fields} })
@@ -708,8 +709,14 @@ _equal${n}(const $n *a, const $n *b)
}
}
+ if (!$memcpy_ignore && $node_type_info{$n}->{fields} > 2)
+ {
+ print $cff "\tmemcpy(newnode, from, sizeof(*from));\n" unless $copy_ignore;
+ $memcpy_ignore = 1;
+ }
+
# override type-specific copy method if copy_as is specified
- if (defined $copy_as_field)
+ if (defined $copy_as_field && !$memcpy_ignore)
{
print $cff "\tnewnode->$f = $copy_as_field;\n"
unless $copy_ignore;
@@ -730,12 +737,12 @@ _equal${n}(const $n *a, const $n *b)
}
elsif ($t eq 'int' && $f =~ 'location$')
{
- print $cff "\tCOPY_LOCATION_FIELD($f);\n" unless $copy_ignore;
+ print $cff "\tCOPY_LOCATION_FIELD($f);\n" unless $copy_ignore || $memcpy_ignore;
print $eff "\tCOMPARE_LOCATION_FIELD($f);\n" unless $equal_ignore;
}
elsif (elem $t, @scalar_types or elem $t, @enum_types)
{
- print $cff "\tCOPY_SCALAR_FIELD($f);\n" unless $copy_ignore;
+ print $cff "\tCOPY_SCALAR_FIELD($f);\n" unless $copy_ignore || $memcpy_ignore;
if (elem 'equal_ignore_if_zero', @a)
{
print $eff
@@ -760,7 +767,7 @@ _equal${n}(const $n *a, const $n *b)
'List*')
{
print $cff
- "\tCOPY_POINTER_FIELD($f, list_length(from->$array_size_field) * sizeof($tt));\n"
+ "\tCOPY_POINTER_FIELD($f, list_length(from->$array_size_field), sizeof($tt));\n"
unless $copy_ignore;
print $eff
"\tCOMPARE_POINTER_FIELD($f, list_length(a->$array_size_field) * sizeof($tt));\n"
@@ -769,7 +776,7 @@ _equal${n}(const $n *a, const $n *b)
else
{
print $cff
- "\tCOPY_POINTER_FIELD($f, from->$array_size_field * sizeof($tt));\n"
+ "\tCOPY_POINTER_FIELD($f, from->$array_size_field, sizeof($tt));\n"
unless $copy_ignore;
print $eff
"\tCOMPARE_POINTER_FIELD($f, a->$array_size_field * sizeof($tt));\n"
@@ -779,7 +786,7 @@ _equal${n}(const $n *a, const $n *b)
elsif ($t eq 'function pointer')
{
# we can copy and compare as a scalar
- print $cff "\tCOPY_SCALAR_FIELD($f);\n" unless $copy_ignore;
+ print $cff "\tCOPY_SCALAR_FIELD($f);\n" unless $copy_ignore || $memcpy_ignore;
print $eff "\tCOMPARE_SCALAR_FIELD($f);\n" unless $equal_ignore;
}
# node type
@@ -792,7 +799,7 @@ _equal${n}(const $n *a, const $n *b)
# array (inline)
elsif ($t =~ /^\w+\[\w+\]$/)
{
- print $cff "\tCOPY_ARRAY_FIELD($f);\n" unless $copy_ignore;
+ print $cff "\tCOPY_ARRAY_FIELD($f);\n" unless $copy_ignore || $memcpy_ignore;
print $eff "\tCOMPARE_ARRAY_FIELD($f);\n" unless $equal_ignore;
}
elsif ($t eq 'struct CustomPathMethods*'
@@ -801,7 +808,7 @@ _equal${n}(const $n *a, const $n *b)
# Fields of these types are required to be a pointer to a
# static table of callback functions. So we don't copy
# the table itself, just reference the original one.
- print $cff "\tCOPY_SCALAR_FIELD($f);\n" unless $copy_ignore;
+ print $cff "\tCOPY_SCALAR_FIELD($f);\n" unless $copy_ignore || $memcpy_ignore;
print $eff "\tCOMPARE_SCALAR_FIELD($f);\n" unless $equal_ignore;
}
else
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index 23776367c5..5caa7392ae 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -58,74 +58,74 @@
/* Read an integer field (anything written as ":fldname %d") */
#define READ_INT_FIELD(fldname) \
- token = pg_strtok(&length); /* skip :fldname */ \
+ (void) pg_strtok(&length); /* skip :fldname */ \
token = pg_strtok(&length); /* get field value */ \
local_node->fldname = atoi(token)
/* Read an unsigned integer field (anything written as ":fldname %u") */
#define READ_UINT_FIELD(fldname) \
- token = pg_strtok(&length); /* skip :fldname */ \
+ (void) pg_strtok(&length); /* skip :fldname */ \
token = pg_strtok(&length); /* get field value */ \
local_node->fldname = atoui(token)
/* Read an unsigned integer field (anything written using UINT64_FORMAT) */
#define READ_UINT64_FIELD(fldname) \
- token = pg_strtok(&length); /* skip :fldname */ \
+ (void) pg_strtok(&length); /* skip :fldname */ \
token = pg_strtok(&length); /* get field value */ \
local_node->fldname = strtou64(token, NULL, 10)
/* Read a long integer field (anything written as ":fldname %ld") */
#define READ_LONG_FIELD(fldname) \
- token = pg_strtok(&length); /* skip :fldname */ \
+ (void) pg_strtok(&length); /* skip :fldname */ \
token = pg_strtok(&length); /* get field value */ \
local_node->fldname = atol(token)
/* Read an OID field (don't hard-wire assumption that OID is same as uint) */
#define READ_OID_FIELD(fldname) \
- token = pg_strtok(&length); /* skip :fldname */ \
+ (void) pg_strtok(&length); /* skip :fldname */ \
token = pg_strtok(&length); /* get field value */ \
local_node->fldname = atooid(token)
/* Read a char field (ie, one ascii character) */
#define READ_CHAR_FIELD(fldname) \
- token = pg_strtok(&length); /* skip :fldname */ \
+ (void) pg_strtok(&length); /* skip :fldname */ \
token = pg_strtok(&length); /* get field value */ \
/* avoid overhead of calling debackslash() for one char */ \
local_node->fldname = (length == 0) ? '\0' : (token[0] == '\\' ? token[1] : token[0])
/* Read an enumerated-type field that was written as an integer code */
#define READ_ENUM_FIELD(fldname, enumtype) \
- token = pg_strtok(&length); /* skip :fldname */ \
+ (void) pg_strtok(&length); /* skip :fldname */ \
token = pg_strtok(&length); /* get field value */ \
local_node->fldname = (enumtype) atoi(token)
/* Read a float field */
#define READ_FLOAT_FIELD(fldname) \
- token = pg_strtok(&length); /* skip :fldname */ \
+ (void) pg_strtok(&length); /* skip :fldname */ \
token = pg_strtok(&length); /* get field value */ \
local_node->fldname = atof(token)
/* Read a boolean field */
#define READ_BOOL_FIELD(fldname) \
- token = pg_strtok(&length); /* skip :fldname */ \
+ (void) pg_strtok(&length); /* skip :fldname */ \
token = pg_strtok(&length); /* get field value */ \
local_node->fldname = strtobool(token)
/* Read a character-string field */
#define READ_STRING_FIELD(fldname) \
- token = pg_strtok(&length); /* skip :fldname */ \
+ (void) pg_strtok(&length); /* skip :fldname */ \
token = pg_strtok(&length); /* get field value */ \
local_node->fldname = nullable_string(token, length)
/* Read a parse location field (and possibly throw away the value) */
#ifdef WRITE_READ_PARSE_PLAN_TREES
#define READ_LOCATION_FIELD(fldname) \
- token = pg_strtok(&length); /* skip :fldname */ \
+ (void) pg_strtok(&length); /* skip :fldname */ \
token = pg_strtok(&length); /* get field value */ \
local_node->fldname = restore_location_fields ? atoi(token) : -1
#else
#define READ_LOCATION_FIELD(fldname) \
- token = pg_strtok(&length); /* skip :fldname */ \
+ (void) pg_strtok(&length); /* skip :fldname */ \
token = pg_strtok(&length); /* get field value */ \
(void) token; /* in case not used elsewhere */ \
local_node->fldname = -1 /* set field to "unknown" */
@@ -145,22 +145,22 @@
/* Read an attribute number array */
#define READ_ATTRNUMBER_ARRAY(fldname, len) \
- token = pg_strtok(&length); /* skip :fldname */ \
+ (void) pg_strtok(&length); /* skip :fldname */ \
local_node->fldname = readAttrNumberCols(len)
/* Read an oid array */
#define READ_OID_ARRAY(fldname, len) \
- token = pg_strtok(&length); /* skip :fldname */ \
+ (void) pg_strtok(&length); /* skip :fldname */ \
local_node->fldname = readOidCols(len)
/* Read an int array */
#define READ_INT_ARRAY(fldname, len) \
- token = pg_strtok(&length); /* skip :fldname */ \
+ (void) pg_strtok(&length); /* skip :fldname */ \
local_node->fldname = readIntCols(len)
/* Read a bool array */
#define READ_BOOL_ARRAY(fldname, len) \
- token = pg_strtok(&length); /* skip :fldname */ \
+ (void) pg_strtok(&length); /* skip :fldname */ \
local_node->fldname = readBoolCols(len)
/* Routine exit */On Thu, Dec 1, 2022 at 8:02 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
Hi,
I believe that has room for improving generation node files.
The patch attached reduced the size of generated files by 27 kbytes.
From 891 kbytes to 864 kbytes.About the patch:
1. Avoid useless attribution when from->field is NULL, once that
the new node is palloc0.2. Avoid useless declaration variable Size, when it is unnecessary.
Not useless -- it prevents a multiple evaluation hazard, which this patch
introduces.
3. Optimize comparison functions like memcmp and strcmp, using
a short-cut comparison of the first element.
Not sure if the juice is worth the squeeze. Profiling would tell.
4. Switch several copy attributions like COPY_SCALAR_FIELD or
COPY_LOCATION_FIELD
by one memcpy call.
My first thought is, it would cause code churn.
5. Avoid useless attribution, ignoring the result of pg_strtok when it is
unnecessary.
Looks worse.
--
John Naylor
EDB: http://www.enterprisedb.com
Hi, thanks for reviewing this.
Em sex., 2 de dez. de 2022 às 09:24, John Naylor <
john.naylor@enterprisedb.com> escreveu:
On Thu, Dec 1, 2022 at 8:02 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
Hi,
I believe that has room for improving generation node files.
The patch attached reduced the size of generated files by 27 kbytes.
From 891 kbytes to 864 kbytes.About the patch:
1. Avoid useless attribution when from->field is NULL, once that
the new node is palloc0.2. Avoid useless declaration variable Size, when it is unnecessary.
Not useless -- it prevents a multiple evaluation hazard, which this patch
introduces.
It's doubting, that patch introduces some hazard here.
But I think that casting size_t (typedef Size) to size_t is worse and is
unnecessary.
Adjusted in the v1 patch.
3. Optimize comparison functions like memcmp and strcmp, using
a short-cut comparison of the first element.Not sure if the juice is worth the squeeze. Profiling would tell.
This is a cheaper test and IMO can really optimize, avoiding a function
call.
4. Switch several copy attributions like COPY_SCALAR_FIELD or
COPY_LOCATION_FIELD
by one memcpy call.
My first thought is, it would cause code churn.
It's a weak argument.
Reduced 27k from source code, really worth it.
5. Avoid useless attribution, ignoring the result of pg_strtok when it
is unnecessary.
Looks worse.
Better to inform the compiler that we really don't need the result.
regards,
Ranier Vilela
Attachments:
v1-optimize_gen_nodes_support.patchapplication/octet-stream; name=v1-optimize_gen_nodes_support.patchDownload
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 84f5e2e6ad..962baa40ce 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -40,7 +40,10 @@
/* Copy a field that is a pointer to a C string, or perhaps NULL */
#define COPY_STRING_FIELD(fldname) \
- (newnode->fldname = from->fldname ? pstrdup(from->fldname) : (char *) NULL)
+ do { \
+ if (from->fldname) \
+ newnode->fldname = pstrdup(from->fldname); \
+ } while (0)
/* Copy a field that is an inline array */
#define COPY_ARRAY_FIELD(fldname) \
@@ -49,11 +52,10 @@
/* Copy a field that is a pointer to a simple palloc'd object of size sz */
#define COPY_POINTER_FIELD(fldname, sz) \
do { \
- Size _size = (sz); \
- if (_size > 0) \
+ if ((sz) > 0) \
{ \
- newnode->fldname = palloc(_size); \
- memcpy(newnode->fldname, from->fldname, _size); \
+ newnode->fldname = palloc((sz)); \
+ memcpy(newnode->fldname, from->fldname, (sz)); \
} \
} while (0)
@@ -74,10 +76,7 @@ _copyConst(const Const *from)
{
Const *newnode = makeNode(Const);
- COPY_SCALAR_FIELD(consttype);
- COPY_SCALAR_FIELD(consttypmod);
- COPY_SCALAR_FIELD(constcollid);
- COPY_SCALAR_FIELD(constlen);
+ memcpy(newnode, from, sizeof(*from));
if (from->constbyval || from->constisnull)
{
@@ -97,10 +96,6 @@ _copyConst(const Const *from)
from->constlen);
}
- COPY_SCALAR_FIELD(constisnull);
- COPY_SCALAR_FIELD(constbyval);
- COPY_LOCATION_FIELD(location);
-
return newnode;
}
@@ -176,8 +171,6 @@ _copyBitmapset(const Bitmapset *from)
void *
copyObjectImpl(const void *from)
{
- void *retval;
-
if (from == NULL)
return NULL;
@@ -189,7 +182,7 @@ copyObjectImpl(const void *from)
#include "copyfuncs.switch.c"
case T_List:
- retval = list_copy_deep(from);
+ return list_copy_deep(from);
break;
/*
@@ -199,14 +192,13 @@ copyObjectImpl(const void *from)
case T_IntList:
case T_OidList:
case T_XidList:
- retval = list_copy(from);
+ return list_copy(from);
break;
default:
elog(ERROR, "unrecognized node type: %d", (int) nodeTag(from));
- retval = 0; /* keep compiler quiet */
break;
}
- return retval;
+ return NULL;
}
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index b2f07da62e..f3658e79a2 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -60,12 +60,12 @@
/* Macro for comparing string fields that might be NULL */
#define equalstr(a, b) \
- (((a) != NULL && (b) != NULL) ? (strcmp(a, b) == 0) : (a) == (b))
+ (((a) != NULL && (b) != NULL) ? (a[0] == b[0] && strcmp(a, b) == 0) : (a) == (b))
/* Compare a field that is an inline array */
#define COMPARE_ARRAY_FIELD(fldname) \
do { \
- if (memcmp(a->fldname, b->fldname, sizeof(a->fldname)) != 0) \
+ if (a->fldname[0] != b->fldname[0] || memcmp(a->fldname, b->fldname, sizeof(a->fldname)) != 0) \
return false; \
} while (0)
@@ -224,9 +224,7 @@ _equalList(const List *a, const List *b)
bool
equal(const void *a, const void *b)
{
- bool retval;
-
- if (a == b)
+ if (unlikely(a == b))
return true;
/*
@@ -252,15 +250,14 @@ equal(const void *a, const void *b)
case T_IntList:
case T_OidList:
case T_XidList:
- retval = _equalList(a, b);
+ return _equalList(a, b);
break;
default:
elog(ERROR, "unrecognized node type: %d",
(int) nodeTag(a));
- retval = false; /* keep compiler quiet */
break;
}
- return retval;
+ return false;
}
diff --git a/src/backend/nodes/gen_node_support.pl b/src/backend/nodes/gen_node_support.pl
index b6f086e262..b02ce7bbb2 100644
--- a/src/backend/nodes/gen_node_support.pl
+++ b/src/backend/nodes/gen_node_support.pl
@@ -656,12 +656,12 @@ foreach my $n (@node_types)
next if $struct_no_copy && $struct_no_equal;
print $cfs "\t\tcase T_${n}:\n"
- . "\t\t\tretval = _copy${n}(from);\n"
+ . "\t\t\treturn _copy${n}(from);\n"
. "\t\t\tbreak;\n"
unless $struct_no_copy;
print $efs "\t\tcase T_${n}:\n"
- . "\t\t\tretval = _equal${n}(a, b);\n"
+ . "\t\t\treturn _equal${n}(a, b);\n"
. "\t\t\tbreak;\n"
unless $struct_no_equal;
@@ -680,6 +680,7 @@ static bool
_equal${n}(const $n *a, const $n *b)
{
" unless $struct_no_equal;
+ my $memcpy_ignore = 0;
# print instructions for each field
foreach my $f (@{ $node_type_info{$n}->{fields} })
@@ -708,8 +709,14 @@ _equal${n}(const $n *a, const $n *b)
}
}
+ if (!$memcpy_ignore && $node_type_info{$n}->{fields} > 2)
+ {
+ print $cff "\tmemcpy(newnode, from, sizeof(*from));\n" unless $copy_ignore;
+ $memcpy_ignore = 1;
+ }
+
# override type-specific copy method if copy_as is specified
- if (defined $copy_as_field)
+ if (defined $copy_as_field && !$memcpy_ignore)
{
print $cff "\tnewnode->$f = $copy_as_field;\n"
unless $copy_ignore;
@@ -730,12 +737,12 @@ _equal${n}(const $n *a, const $n *b)
}
elsif ($t eq 'int' && $f =~ 'location$')
{
- print $cff "\tCOPY_LOCATION_FIELD($f);\n" unless $copy_ignore;
+ print $cff "\tCOPY_LOCATION_FIELD($f);\n" unless $copy_ignore || $memcpy_ignore;
print $eff "\tCOMPARE_LOCATION_FIELD($f);\n" unless $equal_ignore;
}
elsif (elem $t, @scalar_types or elem $t, @enum_types)
{
- print $cff "\tCOPY_SCALAR_FIELD($f);\n" unless $copy_ignore;
+ print $cff "\tCOPY_SCALAR_FIELD($f);\n" unless $copy_ignore || $memcpy_ignore;
if (elem 'equal_ignore_if_zero', @a)
{
print $eff
@@ -779,7 +786,7 @@ _equal${n}(const $n *a, const $n *b)
elsif ($t eq 'function pointer')
{
# we can copy and compare as a scalar
- print $cff "\tCOPY_SCALAR_FIELD($f);\n" unless $copy_ignore;
+ print $cff "\tCOPY_SCALAR_FIELD($f);\n" unless $copy_ignore || $memcpy_ignore;
print $eff "\tCOMPARE_SCALAR_FIELD($f);\n" unless $equal_ignore;
}
# node type
@@ -792,7 +799,7 @@ _equal${n}(const $n *a, const $n *b)
# array (inline)
elsif ($t =~ /^\w+\[\w+\]$/)
{
- print $cff "\tCOPY_ARRAY_FIELD($f);\n" unless $copy_ignore;
+ print $cff "\tCOPY_ARRAY_FIELD($f);\n" unless $copy_ignore || $memcpy_ignore;
print $eff "\tCOMPARE_ARRAY_FIELD($f);\n" unless $equal_ignore;
}
elsif ($t eq 'struct CustomPathMethods*'
@@ -801,7 +808,7 @@ _equal${n}(const $n *a, const $n *b)
# Fields of these types are required to be a pointer to a
# static table of callback functions. So we don't copy
# the table itself, just reference the original one.
- print $cff "\tCOPY_SCALAR_FIELD($f);\n" unless $copy_ignore;
+ print $cff "\tCOPY_SCALAR_FIELD($f);\n" unless $copy_ignore || $memcpy_ignore;
print $eff "\tCOMPARE_SCALAR_FIELD($f);\n" unless $equal_ignore;
}
else
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index 23776367c5..2ba2dc8358 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -58,74 +58,74 @@
/* Read an integer field (anything written as ":fldname %d") */
#define READ_INT_FIELD(fldname) \
- token = pg_strtok(&length); /* skip :fldname */ \
+ (void) pg_strtok(&length); /* skip :fldname */ \
token = pg_strtok(&length); /* get field value */ \
local_node->fldname = atoi(token)
/* Read an unsigned integer field (anything written as ":fldname %u") */
#define READ_UINT_FIELD(fldname) \
- token = pg_strtok(&length); /* skip :fldname */ \
+ (void) pg_strtok(&length); /* skip :fldname */ \
token = pg_strtok(&length); /* get field value */ \
local_node->fldname = atoui(token)
/* Read an unsigned integer field (anything written using UINT64_FORMAT) */
#define READ_UINT64_FIELD(fldname) \
- token = pg_strtok(&length); /* skip :fldname */ \
+ (void) pg_strtok(&length); /* skip :fldname */ \
token = pg_strtok(&length); /* get field value */ \
local_node->fldname = strtou64(token, NULL, 10)
/* Read a long integer field (anything written as ":fldname %ld") */
#define READ_LONG_FIELD(fldname) \
- token = pg_strtok(&length); /* skip :fldname */ \
+ (void) pg_strtok(&length); /* skip :fldname */ \
token = pg_strtok(&length); /* get field value */ \
local_node->fldname = atol(token)
/* Read an OID field (don't hard-wire assumption that OID is same as uint) */
#define READ_OID_FIELD(fldname) \
- token = pg_strtok(&length); /* skip :fldname */ \
+ (void) pg_strtok(&length); /* skip :fldname */ \
token = pg_strtok(&length); /* get field value */ \
local_node->fldname = atooid(token)
/* Read a char field (ie, one ascii character) */
#define READ_CHAR_FIELD(fldname) \
- token = pg_strtok(&length); /* skip :fldname */ \
+ (void) pg_strtok(&length); /* skip :fldname */ \
token = pg_strtok(&length); /* get field value */ \
/* avoid overhead of calling debackslash() for one char */ \
local_node->fldname = (length == 0) ? '\0' : (token[0] == '\\' ? token[1] : token[0])
/* Read an enumerated-type field that was written as an integer code */
#define READ_ENUM_FIELD(fldname, enumtype) \
- token = pg_strtok(&length); /* skip :fldname */ \
+ (void) pg_strtok(&length); /* skip :fldname */ \
token = pg_strtok(&length); /* get field value */ \
local_node->fldname = (enumtype) atoi(token)
/* Read a float field */
#define READ_FLOAT_FIELD(fldname) \
- token = pg_strtok(&length); /* skip :fldname */ \
+ (void) pg_strtok(&length); /* skip :fldname */ \
token = pg_strtok(&length); /* get field value */ \
local_node->fldname = atof(token)
/* Read a boolean field */
#define READ_BOOL_FIELD(fldname) \
- token = pg_strtok(&length); /* skip :fldname */ \
+ (void) pg_strtok(&length); /* skip :fldname */ \
token = pg_strtok(&length); /* get field value */ \
local_node->fldname = strtobool(token)
/* Read a character-string field */
#define READ_STRING_FIELD(fldname) \
- token = pg_strtok(&length); /* skip :fldname */ \
+ (void) pg_strtok(&length); /* skip :fldname */ \
token = pg_strtok(&length); /* get field value */ \
local_node->fldname = nullable_string(token, length)
/* Read a parse location field (and possibly throw away the value) */
#ifdef WRITE_READ_PARSE_PLAN_TREES
#define READ_LOCATION_FIELD(fldname) \
- token = pg_strtok(&length); /* skip :fldname */ \
+ (void) pg_strtok(&length); /* skip :fldname */ \
token = pg_strtok(&length); /* get field value */ \
local_node->fldname = restore_location_fields ? atoi(token) : -1
#else
#define READ_LOCATION_FIELD(fldname) \
- token = pg_strtok(&length); /* skip :fldname */ \
+ (void) pg_strtok(&length); /* skip :fldname */ \
token = pg_strtok(&length); /* get field value */ \
(void) token; /* in case not used elsewhere */ \
local_node->fldname = -1 /* set field to "unknown" */
@@ -145,22 +145,22 @@
/* Read an attribute number array */
#define READ_ATTRNUMBER_ARRAY(fldname, len) \
- token = pg_strtok(&length); /* skip :fldname */ \
+ (void) pg_strtok(&length); /* skip :fldname */ \
local_node->fldname = readAttrNumberCols(len)
/* Read an oid array */
#define READ_OID_ARRAY(fldname, len) \
- token = pg_strtok(&length); /* skip :fldname */ \
+ (void) pg_strtok(&length); /* skip :fldname */ \
local_node->fldname = readOidCols(len)
/* Read an int array */
#define READ_INT_ARRAY(fldname, len) \
- token = pg_strtok(&length); /* skip :fldname */ \
+ (void) pg_strtok(&length); /* skip :fldname */ \
local_node->fldname = readIntCols(len)
/* Read a bool array */
#define READ_BOOL_ARRAY(fldname, len) \
- token = pg_strtok(&length); /* skip :fldname */ \
+ (void) pg_strtok(&length); /* skip :fldname */ \
local_node->fldname = readBoolCols(len)
/* Routine exit */
@@ -283,7 +283,7 @@ _readBoolExpr(void)
READ_LOCALS(BoolExpr);
/* do-it-yourself enum representation */
- token = pg_strtok(&length); /* skip :boolop */
+ (void) pg_strtok(&length); /* skip :boolop */
token = pg_strtok(&length); /* get field value */
if (length == 3 && strncmp(token, "and", 3) == 0)
local_node->boolop = AND_EXPR;
@@ -333,7 +333,7 @@ _readConstraint(void)
READ_BOOL_FIELD(initdeferred);
READ_LOCATION_FIELD(location);
- token = pg_strtok(&length); /* skip :contype */
+ (void) pg_strtok(&length); /* skip :contype */
token = pg_strtok(&length); /* get field value */
if (length == 4 && strncmp(token, "NULL", 4) == 0)
local_node->contype = CONSTR_NULL;
@@ -643,7 +643,7 @@ _readExtensibleNode(void)
READ_TEMP_LOCALS();
- token = pg_strtok(&length); /* skip :extnodename */
+ (void) pg_strtok(&length); /* skip :extnodename */
token = pg_strtok(&length); /* get extnodename */
extnodename = nullable_string(token, length);On Fri, 2 Dec 2022 at 19:06, Ranier Vilela <ranier.vf@gmail.com> wrote:
Hi, thanks for reviewing this.
Em sex., 2 de dez. de 2022 às 09:24, John Naylor <john.naylor@enterprisedb.com> escreveu:
On Thu, Dec 1, 2022 at 8:02 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
Hi,
I believe that has room for improving generation node files.
The patch attached reduced the size of generated files by 27 kbytes.
From 891 kbytes to 864 kbytes.About the patch:
1. Avoid useless attribution when from->field is NULL, once that
the new node is palloc0.2. Avoid useless declaration variable Size, when it is unnecessary.
Not useless -- it prevents a multiple evaluation hazard, which this patch introduces.
It's doubting, that patch introduces some hazard here.
But I think that casting size_t (typedef Size) to size_t is worse and is unnecessary.
Adjusted in the v1 patch.3. Optimize comparison functions like memcmp and strcmp, using
a short-cut comparison of the first element.Not sure if the juice is worth the squeeze. Profiling would tell.
This is a cheaper test and IMO can really optimize, avoiding a function call.
4. Switch several copy attributions like COPY_SCALAR_FIELD or COPY_LOCATION_FIELD
by one memcpy call.My first thought is, it would cause code churn.
It's a weak argument.
Reduced 27k from source code, really worth it.5. Avoid useless attribution, ignoring the result of pg_strtok when it is unnecessary.
Looks worse.
Better to inform the compiler that we really don't need the result.
The patch does not apply on top of HEAD as in [1]http://cfbot.cputube.org/patch_41_4034.log, please post a rebased patch:
=== Applying patches on top of PostgreSQL commit ID
e351f85418313e97c203c73181757a007dfda6d0 ===
=== applying patch ./v1-optimize_gen_nodes_support.patch
patching file src/backend/nodes/gen_node_support.pl
Hunk #2 succeeded at 680 with fuzz 2.
Hunk #3 FAILED at 709.
...
Hunk #7 succeeded at 844 (offset 42 lines).
1 out of 7 hunks FAILED -- saving rejects to file
src/backend/nodes/gen_node_support.pl.rej
[1]: http://cfbot.cputube.org/patch_41_4034.log
Regards,
Vignesh
vignesh C <vignesh21@gmail.com> writes:
The patch does not apply on top of HEAD as in [1], please post a rebased patch:
Yeah. The way that I'd been thinking of optimizing the copy functions
was more or less as attached: continue to write all the COPY_SCALAR_FIELD
macro calls, but just make them expand to no-ops after an initial memcpy
of the whole node. This preserves flexibility to do something else while
still getting the benefit of substituting memcpy for retail field copies.
Having said that, I'm not very sure it's worth doing, because I do not
see any major reduction in code size:
HEAD:
text data bss dec hex filename
53601 0 0 53601 d161 copyfuncs.o
w/patch:
text data bss dec hex filename
49850 0 0 49850 c2ba copyfuncs.o
I've not looked at the generated assembly code, but I suspect that
my compiler is converting the memcpy's into inlined code that's
hardly any smaller than field-by-field assignment. Also, it's
rather debatable that it'd be faster, especially for node types
that are mostly pointer fields, where the memcpy is going to be
largely wasted effort.
I tend to agree with John that the rest of the changes proposed
in the v1 patch are not useful improvements, especially with
no evidence offered that they'd make the code smaller or faster.
regards, tom lane
Attachments:
v2-optimize_gen_nodes_support.patchtext/x-diff; charset=us-ascii; name=v2-optimize_gen_nodes_support.patchDownload
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index f2568ff5e6..1a8df587ce 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -27,8 +27,9 @@
*/
/* Copy a simple scalar field (int, float, bool, enum, etc) */
+/* We now assume that scalar fields are flat-copied via initial memcpy */
#define COPY_SCALAR_FIELD(fldname) \
- (newnode->fldname = from->fldname)
+ ((void) 0)
/* Copy a field that is a pointer to some kind of Node or Node tree */
#define COPY_NODE_FIELD(fldname) \
@@ -43,8 +44,9 @@
(newnode->fldname = from->fldname ? pstrdup(from->fldname) : (char *) NULL)
/* Copy a field that is an inline array */
+/* These too should be taken care of by initial memcpy */
#define COPY_ARRAY_FIELD(fldname) \
- memcpy(newnode->fldname, from->fldname, sizeof(newnode->fldname))
+ ((void) 0)
/* Copy a field that is a pointer to a simple palloc'd object of size sz */
#define COPY_POINTER_FIELD(fldname, sz) \
@@ -59,7 +61,7 @@
/* Copy a parse location field (for Copy, this is same as scalar case) */
#define COPY_LOCATION_FIELD(fldname) \
- (newnode->fldname = from->fldname)
+ ((void) 0)
#include "copyfuncs.funcs.c"
@@ -72,8 +74,9 @@
static Const *
_copyConst(const Const *from)
{
- Const *newnode = makeNode(Const);
+ Const *newnode = palloc_object(Const);
+ memcpy(newnode, from, sizeof(Const));
COPY_SCALAR_FIELD(consttype);
COPY_SCALAR_FIELD(consttypmod);
COPY_SCALAR_FIELD(constcollid);
@@ -107,8 +110,9 @@ _copyConst(const Const *from)
static A_Const *
_copyA_Const(const A_Const *from)
{
- A_Const *newnode = makeNode(A_Const);
+ A_Const *newnode = palloc_object(A_Const);
+ memcpy(newnode, from, sizeof(A_Const));
COPY_SCALAR_FIELD(isnull);
if (!from->isnull)
{
@@ -150,8 +154,10 @@ _copyExtensibleNode(const ExtensibleNode *from)
const ExtensibleNodeMethods *methods;
methods = GetExtensibleNodeMethods(from->extnodename, false);
- newnode = (ExtensibleNode *) newNode(methods->node_size,
- T_ExtensibleNode);
+
+ newnode = (ExtensibleNode *) palloc(methods->node_size);
+ memcpy(newnode, from, methods->node_size);
+
COPY_STRING_FIELD(extnodename);
/* copy the private fields */
diff --git a/src/backend/nodes/gen_node_support.pl b/src/backend/nodes/gen_node_support.pl
index b3c1ead496..4f62fa09cb 100644
--- a/src/backend/nodes/gen_node_support.pl
+++ b/src/backend/nodes/gen_node_support.pl
@@ -671,8 +671,9 @@ foreach my $n (@node_types)
static $n *
_copy${n}(const $n *from)
{
-\t${n} *newnode = makeNode($n);
+\t${n} *newnode = palloc_object($n);
+\tmemcpy(newnode, from, sizeof($n));
" unless $struct_no_copy;
print $eff "
Em qua., 4 de jan. de 2023 às 19:39, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
vignesh C <vignesh21@gmail.com> writes:
The patch does not apply on top of HEAD as in [1], please post a rebased
patch:
Yeah. The way that I'd been thinking of optimizing the copy functions
was more or less as attached: continue to write all the COPY_SCALAR_FIELD
macro calls, but just make them expand to no-ops after an initial memcpy
of the whole node. This preserves flexibility to do something else while
still getting the benefit of substituting memcpy for retail field copies.
Having said that, I'm not very sure it's worth doing, because I do not
see any major reduction in code size:
I think this option is worse.
By disabling these macros, you lose their use in other areas.
By putting more intelligence into gen_node_support.pl, to either use memcpy
or the macros is better, IMO.
In cases of one or two macros, would it be faster than memset?
HEAD:
text data bss dec hex filename
53601 0 0 53601 d161 copyfuncs.o
w/patch:
text data bss dec hex filename
49850 0 0 49850 c2ba copyfuncs.o
I haven't tested it on Linux, but on Windows, there is a significant
reduction.
head:
8,114,688 postgres.exe
121.281 copyfuncs.funcs.c
patched:
8,108,544 postgres.exe
95.321 copyfuncs.funcs.c
I've not looked at the generated assembly code, but I suspect that
my compiler is converting the memcpy's into inlined code that's
hardly any smaller than field-by-field assignment. Also, it's
rather debatable that it'd be faster, especially for node types
that are mostly pointer fields, where the memcpy is going to be
largely wasted effort.
IMO, with many field assignments, memcpy would be more faster.
I tend to agree with John that the rest of the changes proposed
in the v1 patch are not useful improvements, especially with
no evidence offered that they'd make the code smaller or faster.
I tried using palloc_object, as you proposed, but several tests failed.
So I suspect that some fields are not filled in correctly.
It would be an advantage to avoid memset in the allocation (palloc0), but I
chose to keep it because of the errors.
This way, if we use palloc0, there is no need to set NULL on
COPY_STRING_FIELD.
Regarding COPY_POINTER_FIELD, it is wasteful to cast size_t to size_t.
v3 attached.
regards,
Ranier Vilela
Show quoted text
regards, tom lane
Attachments:
v3-optimize_gen_node_support.patchapplication/octet-stream; name=v3-optimize_gen_node_support.patchDownload
index f2568ff5e6..8233c9b7d3 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -40,7 +40,10 @@
/* Copy a field that is a pointer to a C string, or perhaps NULL */
#define COPY_STRING_FIELD(fldname) \
- (newnode->fldname = from->fldname ? pstrdup(from->fldname) : (char *) NULL)
+ do { \
+ if (from->fldname) \
+ newnode->fldname = pstrdup(from->fldname); \
+ } while (0)
/* Copy a field that is an inline array */
#define COPY_ARRAY_FIELD(fldname) \
@@ -49,11 +52,10 @@
/* Copy a field that is a pointer to a simple palloc'd object of size sz */
#define COPY_POINTER_FIELD(fldname, sz) \
do { \
- Size _size = (sz); \
- if (_size > 0) \
+ if (from->fldname && (sz) > 0) \
{ \
- newnode->fldname = palloc(_size); \
- memcpy(newnode->fldname, from->fldname, _size); \
+ newnode->fldname = palloc((sz)); \
+ memcpy(newnode->fldname, from->fldname, (sz)); \
} \
} while (0)
@@ -74,10 +76,7 @@ _copyConst(const Const *from)
{
Const *newnode = makeNode(Const);
- COPY_SCALAR_FIELD(consttype);
- COPY_SCALAR_FIELD(consttypmod);
- COPY_SCALAR_FIELD(constcollid);
- COPY_SCALAR_FIELD(constlen);
+ memcpy(newnode, from, sizeof(*from));
if (from->constbyval || from->constisnull)
{
@@ -97,10 +96,6 @@ _copyConst(const Const *from)
from->constlen);
}
- COPY_SCALAR_FIELD(constisnull);
- COPY_SCALAR_FIELD(constbyval);
- COPY_LOCATION_FIELD(location);
-
return newnode;
}
@@ -109,7 +104,8 @@ _copyA_Const(const A_Const *from)
{
A_Const *newnode = makeNode(A_Const);
- COPY_SCALAR_FIELD(isnull);
+ memcpy(newnode, from, sizeof(*from));
+
if (!from->isnull)
{
/* This part must duplicate other _copy*() functions. */
@@ -138,8 +134,6 @@ _copyA_Const(const A_Const *from)
}
}
- COPY_LOCATION_FIELD(location);
-
return newnode;
}
diff --git a/src/backend/nodes/gen_node_support.pl b/src/backend/nodes/gen_node_support.pl
index b3c1ead496..39560fbd0e 100644
--- a/src/backend/nodes/gen_node_support.pl
+++ b/src/backend/nodes/gen_node_support.pl
@@ -680,6 +680,7 @@ static bool
_equal${n}(const $n *a, const $n *b)
{
" unless $struct_no_equal;
+ my $memcpy_ignore = 0;
# track already-processed fields to support field order checks
my %previous_fields;
@@ -725,8 +726,14 @@ _equal${n}(const $n *a, const $n *b)
}
}
+ if (!$memcpy_ignore && $node_type_info{$n}->{fields} > 2)
+ {
+ print $cff "\tmemcpy(newnode, from, sizeof(*from));\n" unless $copy_ignore;
+ $memcpy_ignore = 1;
+ }
+
# override type-specific copy method if requested
- if (defined $copy_as_field)
+ if (defined $copy_as_field && !$memcpy_ignore)
{
print $cff "\tnewnode->$f = $copy_as_field;\n"
unless $copy_ignore;
@@ -735,7 +742,7 @@ _equal${n}(const $n *a, const $n *b)
elsif ($copy_as_scalar)
{
print $cff "\tCOPY_SCALAR_FIELD($f);\n"
- unless $copy_ignore;
+ unless $copy_ignore || $memcpy_ignore;
$copy_ignore = 1;
}
@@ -761,12 +768,12 @@ _equal${n}(const $n *a, const $n *b)
}
elsif ($t eq 'int' && $f =~ 'location$')
{
- print $cff "\tCOPY_LOCATION_FIELD($f);\n" unless $copy_ignore;
+ print $cff "\tCOPY_LOCATION_FIELD($f);\n" unless $copy_ignore || $memcpy_ignore;
print $eff "\tCOMPARE_LOCATION_FIELD($f);\n" unless $equal_ignore;
}
elsif (elem $t, @scalar_types or elem $t, @enum_types)
{
- print $cff "\tCOPY_SCALAR_FIELD($f);\n" unless $copy_ignore;
+ print $cff "\tCOPY_SCALAR_FIELD($f);\n" unless $copy_ignore || $memcpy_ignore;
if (elem 'equal_ignore_if_zero', @a)
{
print $eff
@@ -810,7 +817,7 @@ _equal${n}(const $n *a, const $n *b)
elsif ($t eq 'function pointer')
{
# we can copy and compare as a scalar
- print $cff "\tCOPY_SCALAR_FIELD($f);\n" unless $copy_ignore;
+ print $cff "\tCOPY_SCALAR_FIELD($f);\n" unless $copy_ignore || $memcpy_ignore;
print $eff "\tCOMPARE_SCALAR_FIELD($f);\n" unless $equal_ignore;
}
# node type
@@ -834,7 +841,7 @@ _equal${n}(const $n *a, const $n *b)
# array (inline)
elsif ($t =~ /^\w+\[\w+\]$/)
{
- print $cff "\tCOPY_ARRAY_FIELD($f);\n" unless $copy_ignore;
+ print $cff "\tCOPY_ARRAY_FIELD($f);\n" unless $copy_ignore || $memcpy_ignore;
print $eff "\tCOMPARE_ARRAY_FIELD($f);\n" unless $equal_ignore;
}
elsif ($t eq 'struct CustomPathMethods*'
@@ -843,7 +850,7 @@ _equal${n}(const $n *a, const $n *b)
# Fields of these types are required to be a pointer to a
# static table of callback functions. So we don't copy
# the table itself, just reference the original one.
- print $cff "\tCOPY_SCALAR_FIELD($f);\n" unless $copy_ignore;
+ print $cff "\tCOPY_SCALAR_FIELD($f);\n" unless $copy_ignore || $memcpy_ignore;
print $eff "\tCOMPARE_SCALAR_FIELD($f);\n" unless $equal_ignore;
}
else
Ranier Vilela <ranier.vf@gmail.com> writes:
Em qua., 4 de jan. de 2023 às 19:39, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
Yeah. The way that I'd been thinking of optimizing the copy functions
was more or less as attached: continue to write all the COPY_SCALAR_FIELD
macro calls, but just make them expand to no-ops after an initial memcpy
of the whole node.
I think this option is worse.
By disabling these macros, you lose their use in other areas.
What other areas? They're local to copyfuncs.c.
The bigger picture here is that as long as we have any manually-maintained
node copy functions, it seems best to adhere to the existing convention
of explicitly listing each and every field in them. I'm far more
concerned about errors-of-omission than I am about incremental performance
gains (which still haven't been demonstrated to exist, anyway).
v3 attached.
I think you're wasting people's time if you don't provide some
performance measurements showing that this is worth doing from
a speed standpoint.
regards, tom lane
I think it's clear we aren't going to be taking this patch in its
current form. Perhaps there are better ways to do what these files do
(I sure think there are!) but I don't think microoptimizing the
copying is something people are super excited about. It sounds like
rethinking how to make these functions more convenient for programmers
to maintain reliably would be more valuable.
I guess I'll mark it Returned with Feedback -- if there are
significant performance gains to show without making the code harder
to maintain and/or a nicer way to structure this code in general then
we can revisit this.
--
Gregory Stark
As Commitfest Manager