From: Andy Fan <zhihuifan1213@163.com>
To: David Rowley <dgrowleyml@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Make printtup a bit faster
Date: Fri, 30 Aug 2024 13:00:50 +0800
Message-ID: <87v7zihaf1.fsf@163.com> (raw)
In-Reply-To: <CAApHDvoE1EiHeQaHKh+NnB8GqhSVDUTjkHovTcXzp0W-d0yK0g@mail.gmail.com>
References: <87wmjzfz0h.fsf@163.com>
<CAApHDvrBNA-QRsbn-SJyRAsywjHCNenfcbi14c3O3c7=OimQ8Q@mail.gmail.com>
<3601908.1724945588@sss.pgh.pa.us>
<CAApHDvoE1EiHeQaHKh+NnB8GqhSVDUTjkHovTcXzp0W-d0yK0g@mail.gmail.com>
David Rowley <dgrowleyml@gmail.com> writes:
> On Fri, 30 Aug 2024 at 03:33, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> David Rowley <dgrowleyml@gmail.com> writes:
>> > [ redesign I/O function APIs ]
>> > I had planned to work on this for PG18, but I'd be happy for some
>> > assistance if you're willing.
>>
>> I'm skeptical that such a thing will ever be practical. To avoid
>> breaking un-converted data types, all the call sites would have to
>> support both old and new APIs. To avoid breaking non-core callers,
>> all the I/O functions would have to support both old and new APIs.
>> That probably adds enough overhead to negate whatever benefit you'd
>> get.
>
> So, we currently return cstrings in our output functions. Let's take
> jsonb_out() as an example, to build that cstring, we make a *new*
> StringInfoData on *every call* inside JsonbToCStringWorker(). That
> gives you 1024 bytes before you need to enlarge it. However, it's
> maybe not all bad as we have some size estimations there to call
> enlargeStringInfo(), only that's a bit wasteful as it does a
> repalloc() which memcpys the freshly allocated 1024 bytes allocated in
> initStringInfo() when it doesn't yet contain any data. After
> jsonb_out() has returned and we have the cstring, only we forgot the
> length of the string, so most places will immediately call strlen() or
> do that indirectly via appendStringInfoString(). For larger JSON
> documents, that'll likely require pulling cachelines back into L1
> again. I don't know how modern CPU cacheline eviction works, but if it
> was as simple as FIFO then the strlen() would flush all those
> cachelines only for memcpy() to have to read them back again for
> output strings larger than L1.
The attached is PoC of this idea, not matter which method are adopted
(rewrite all the outfunction or a optional print function), I think the
benefit will be similar. In the blew test case, it shows us 10%+
improvements. (0.134ms vs 0.110ms)
create table demo as select oid as oid1, relname::text as text1, relam,
relname::text as text2 from pg_class;
pgbench:
select * from demo;
--
Best Regards
Andy Fan
Attachments:v20240830-0001-Avoiding-some-memcpy-strlen-palloc-in-prin.patch (text/x-diff, 5.6K) download
From 5ace763a5126478da1c3cb68d5221d83e45d2f34 Mon Sep 17 00:00:00 2001
From: Andy Fan <zhihuifan1213@163.com>
Date: Fri, 30 Aug 2024 12:50:54 +0800
Subject: [PATCH v20240830 1/1] Avoiding some memcpy, strlen, palloc in
printtup.
https://www.postgresql.org/message-id/87wmjzfz0h.fsf%40163.com
---
src/backend/access/common/printtup.c | 40 ++++++++++++++++++++++------
src/backend/utils/adt/oid.c | 20 ++++++++++++++
src/backend/utils/adt/varlena.c | 17 ++++++++++++
src/include/catalog/pg_proc.dat | 9 ++++++-
4 files changed, 77 insertions(+), 9 deletions(-)
diff --git a/src/backend/access/common/printtup.c b/src/backend/access/common/printtup.cindex c78cc39308..ecba4a7113 100644--- a/src/backend/access/common/printtup.c+++ b/src/backend/access/common/printtup.c@@ -19,6 +19,7 @@
#include "libpq/pqformat.h"
#include "libpq/protocol.h"
#include "tcop/pquery.h"
+#include "utils/fmgroids.h"
#include "utils/lsyscache.h"
#include "utils/memdebug.h"
#include "utils/memutils.h"
@@ -49,6 +50,7 @@ typedef struct
bool typisvarlena; /* is it varlena (ie possibly toastable)? */
int16 format; /* format code for this column */
FmgrInfo finfo; /* Precomputed call info for output fn */
+ FmgrInfo p_finfo; /* Precomputed call info for print fn if any */
} PrinttupAttrInfo;
typedef struct
@@ -274,10 +276,25 @@ printtup_prepare_info(DR_printtup *myState, TupleDesc typeinfo, int numAttrs)
thisState->format = format;
if (format == 0)
{
- getTypeOutputInfo(attr->atttypid,- &thisState->typoutput,- &thisState->typisvarlena);- fmgr_info(thisState->typoutput, &thisState->finfo);+ /*+ * If the type defines a print function, then use it+ * rather than outfunction.+ *+ * XXX: need a generic function to improve the if-elseif.+ */+ if (attr->atttypid == OIDOID)+ fmgr_info(F_OIDPRINT, &thisState->p_finfo);+ else if (attr->atttypid == TEXTOID)+ fmgr_info(F_TEXTPRINT, &thisState->p_finfo);+ else+ {+ getTypeOutputInfo(attr->atttypid,+ &thisState->typoutput,+ &thisState->typisvarlena);+ fmgr_info(thisState->typoutput, &thisState->finfo);+ /* mark print function is invalid */+ thisState->p_finfo.fn_oid = InvalidOid;+ }
}
else if (format == 1)
{
@@ -355,10 +372,17 @@ printtup(TupleTableSlot *slot, DestReceiver *self)
if (thisState->format == 0)
{
/* Text output */
- char *outputstr;-- outputstr = OutputFunctionCall(&thisState->finfo, attr);- pq_sendcountedtext(buf, outputstr, strlen(outputstr));+ if (thisState->p_finfo.fn_oid)+ {+ FunctionCall2(&thisState->p_finfo, attr, PointerGetDatum(buf));+ }+ else+ {+ char *outputstr;++ outputstr = OutputFunctionCall(&thisState->finfo, attr);+ pq_sendcountedtext(buf, outputstr, strlen(outputstr));+ }
}
else
{
diff --git a/src/backend/utils/adt/oid.c b/src/backend/utils/adt/oid.cindex 56fb1fd77c..cc85d920c8 100644--- a/src/backend/utils/adt/oid.c+++ b/src/backend/utils/adt/oid.c@@ -53,6 +53,26 @@ oidout(PG_FUNCTION_ARGS)
PG_RETURN_CSTRING(result);
}
+Datum+oidprint(PG_FUNCTION_ARGS)+{+ Oid o = PG_GETARG_OID(0);+ StringInfo buf = (StringInfo) PG_GETARG_POINTER(1);+ uint32 *lenp;+ uint32 data_len;++ /* 12 is the max length for an oid's text presentation. */+ enlargeStringInfo(buf, sizeof(int32) + 12);++ /* note the position for len */+ lenp = (uint32 *) (buf->data + buf->len);+ data_len = pg_snprintf(buf->data + buf->len + sizeof(int), 12, "%u", o);+ *lenp = pg_hton32(data_len);+ buf->len += sizeof(uint32) + data_len;++ PG_RETURN_VOID();+}+
/*
* oidrecv - converts external binary format to oid
*/
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.cindex 7c6391a276..3b7006d54a 100644--- a/src/backend/utils/adt/varlena.c+++ b/src/backend/utils/adt/varlena.c@@ -594,6 +594,23 @@ textout(PG_FUNCTION_ARGS)
PG_RETURN_CSTRING(TextDatumGetCString(txt));
}
++Datum+textprint(PG_FUNCTION_ARGS)+{+ text *txt = (text *) pg_detoast_datum((struct varlena *)PG_GETARG_POINTER(0));+ StringInfo buf = (StringInfo) PG_GETARG_POINTER(1);+ uint32 text_len = VARSIZE(txt) - VARHDRSZ;+ uint32 ni = pg_hton32(text_len);++ enlargeStringInfo(buf, sizeof(int32) + text_len);+ memcpy((char *pg_restrict) buf->data + buf->len, &ni, sizeof(uint32));+ memcpy(buf->data + buf->len + sizeof(int), VARDATA(txt), text_len);+ buf->len += sizeof(uint32) + text_len;++ PG_RETURN_VOID();+}+
/*
* textrecv - converts external binary format to text
*/
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.datindex 85f42be1b3..74eeead4de 100644--- a/src/include/catalog/pg_proc.dat+++ b/src/include/catalog/pg_proc.dat@@ -100,6 +100,10 @@
{ oid => '47', descr => 'I/O',
proname => 'textout', prorettype => 'cstring', proargtypes => 'text',
prosrc => 'textout' },
+{+ oid => '8907', descr => 'I/O',+ proname => 'textprint', prorettype => 'void', proargtypes => 'text internal',+ prosrc => 'textprint' },
{ oid => '48', descr => 'I/O',
proname => 'tidin', prorettype => 'tid', proargtypes => 'cstring',
prosrc => 'tidin' },
@@ -4718,7 +4722,10 @@
{ oid => '1799', descr => 'I/O',
proname => 'oidout', prorettype => 'cstring', proargtypes => 'oid',
prosrc => 'oidout' },
-+{+ oid => '9771', descr => 'I/O',+ proname => 'oidprint', prorettype => 'void', proargtypes => 'oid internal',+ prosrc => 'oidprint'},
{ oid => '3058', descr => 'concatenate values',
proname => 'concat', provariadic => 'any', proisstrict => 'f',
provolatile => 's', prorettype => 'text', proargtypes => 'any',
--
2.45.1
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: pgsql-hackers@postgresql.org
Cc: zhihuifan1213@163.com, dgrowleyml@gmail.com, tgl@sss.pgh.pa.us
Subject: Re: Make printtup a bit faster
In-Reply-To: <87v7zihaf1.fsf@163.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox