Re: Optimizing Node Files Support

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

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 "