public inbox for [email protected]
help / color / mirror / Atom feedFrom: Ranier Vilela <[email protected]>
To: Tom Lane <[email protected]>
Cc: vignesh C <[email protected]>
Cc: John Naylor <[email protected]>
Cc: Pg Hackers <[email protected]>
Subject: Re: Optimizing Node Files Support
Date: Fri, 6 Jan 2023 11:49:34 -0300
Message-ID: <CAEudQArMiszB2x6F3H3TO+3AM46qKuEdyj_XvsMUky2MG2NNVg@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAEudQAoJPmuJcymSN4ogXFY8AHGcLgSGj4xeXWKD1WrzZ9Azvw@mail.gmail.com>
<CAFBsxsE+cuaUGTJfDGLK8tx+a5DKyk-9+PA7qnb2OgUBYORZtQ@mail.gmail.com>
<CAEudQAp0fDc_3ps0F-NAnmOUpH_r08uwYx0YZigi9=cXp72Eww@mail.gmail.com>
<CALDaNm3VeiZ4r_hpmcoyPTEHUgq-7bsWSg3dC734kiOfz1_CFQ@mail.gmail.com>
<[email protected]>
Em qua., 4 de jan. de 2023 às 19:39, Tom Lane <[email protected]> escreveu:
> vignesh C <[email protected]> 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
> regards, tom lane
>
>
Attachments:
[application/octet-stream] v3-optimize_gen_node_support.patch (4.7K, ../CAEudQArMiszB2x6F3H3TO+3AM46qKuEdyj_XvsMUky2MG2NNVg@mail.gmail.com/3-v3-optimize_gen_node_support.patch)
download | inline diff:
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
view thread (4+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: Optimizing Node Files Support
In-Reply-To: <CAEudQArMiszB2x6F3H3TO+3AM46qKuEdyj_XvsMUky2MG2NNVg@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox