public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v1] elog.c: Remove special case which avoided %*s format strings..
3+ messages / 3 participants
[nested] [flat]
* [PATCH v1] elog.c: Remove special case which avoided %*s format strings..
@ 2020-08-01 02:53 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: Justin Pryzby @ 2020-08-01 02:53 UTC (permalink / raw)
..which should no longer be needed since it was a performance hack for specific
platform snprintf, which are no longer used.
See also:
4334639f4 Allow printf-style padding specifications in log_line_prefix.
96bf88d52 Always use our own versions of *printf().
abd9ca377 Make assorted performance improvements in snprintf.c.
---
src/backend/utils/error/elog.c | 134 ++++++++-------------------------
1 file changed, 32 insertions(+), 102 deletions(-)
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index d0b368530e..6b6749965f 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -2350,11 +2350,6 @@ log_line_prefix(StringInfo buf, ErrorData *edata)
* Note: Since only '-', '0' to '9' are valid formatting characters we
* can do a quick check here to pre-check for formatting. If the char
* is not formatting then we can skip a useless function call.
- *
- * Further note: At least on some platforms, passing %*s rather than
- * %s to appendStringInfo() is substantially slower, so many of the
- * cases below avoid doing that unless non-zero padding is in fact
- * specified.
*/
if (*p > '9')
padding = 0;
@@ -2371,10 +2366,7 @@ log_line_prefix(StringInfo buf, ErrorData *edata)
if (appname == NULL || *appname == '\0')
appname = _("[unknown]");
- if (padding != 0)
- appendStringInfo(buf, "%*s", padding, appname);
- else
- appendStringInfoString(buf, appname);
+ appendStringInfo(buf, "%*s", padding, appname);
}
else if (padding != 0)
appendStringInfoSpaces(buf,
@@ -2392,10 +2384,7 @@ log_line_prefix(StringInfo buf, ErrorData *edata)
else
backend_type_str = GetBackendTypeDesc(MyBackendType);
- if (padding != 0)
- appendStringInfo(buf, "%*s", padding, backend_type_str);
- else
- appendStringInfoString(buf, backend_type_str);
+ appendStringInfo(buf, "%*s", padding, backend_type_str);
break;
}
case 'u':
@@ -2405,10 +2394,7 @@ log_line_prefix(StringInfo buf, ErrorData *edata)
if (username == NULL || *username == '\0')
username = _("[unknown]");
- if (padding != 0)
- appendStringInfo(buf, "%*s", padding, username);
- else
- appendStringInfoString(buf, username);
+ appendStringInfo(buf, "%*s", padding, username);
}
else if (padding != 0)
appendStringInfoSpaces(buf,
@@ -2421,17 +2407,13 @@ log_line_prefix(StringInfo buf, ErrorData *edata)
if (dbname == NULL || *dbname == '\0')
dbname = _("[unknown]");
- if (padding != 0)
- appendStringInfo(buf, "%*s", padding, dbname);
- else
- appendStringInfoString(buf, dbname);
+ appendStringInfo(buf, "%*s", padding, dbname);
}
else if (padding != 0)
appendStringInfoSpaces(buf,
padding > 0 ? padding : -padding);
break;
case 'c':
- if (padding != 0)
{
char strfbuf[128];
@@ -2439,14 +2421,9 @@ log_line_prefix(StringInfo buf, ErrorData *edata)
(long) (MyStartTime), MyProcPid);
appendStringInfo(buf, "%*s", padding, strfbuf);
}
- else
- appendStringInfo(buf, "%lx.%x", (long) (MyStartTime), MyProcPid);
break;
case 'p':
- if (padding != 0)
- appendStringInfo(buf, "%*d", padding, MyProcPid);
- else
- appendStringInfo(buf, "%d", MyProcPid);
+ appendStringInfo(buf, "%*d", padding, MyProcPid);
break;
case 'P':
@@ -2472,17 +2449,11 @@ log_line_prefix(StringInfo buf, ErrorData *edata)
break;
case 'l':
- if (padding != 0)
- appendStringInfo(buf, "%*ld", padding, log_line_number);
- else
- appendStringInfo(buf, "%ld", log_line_number);
+ appendStringInfo(buf, "%*ld", padding, log_line_number);
break;
case 'm':
setup_formatted_log_time();
- if (padding != 0)
- appendStringInfo(buf, "%*s", padding, formatted_log_time);
- else
- appendStringInfoString(buf, formatted_log_time);
+ appendStringInfo(buf, "%*s", padding, formatted_log_time);
break;
case 't':
{
@@ -2492,10 +2463,7 @@ log_line_prefix(StringInfo buf, ErrorData *edata)
pg_strftime(strfbuf, sizeof(strfbuf),
"%Y-%m-%d %H:%M:%S %Z",
pg_localtime(&stamp_time, log_timezone));
- if (padding != 0)
- appendStringInfo(buf, "%*s", padding, strfbuf);
- else
- appendStringInfoString(buf, strfbuf);
+ appendStringInfo(buf, "%*s", padding, strfbuf);
}
break;
case 'n':
@@ -2512,19 +2480,13 @@ log_line_prefix(StringInfo buf, ErrorData *edata)
(long) saved_timeval.tv_sec,
(int) (saved_timeval.tv_usec / 1000));
- if (padding != 0)
- appendStringInfo(buf, "%*s", padding, strfbuf);
- else
- appendStringInfoString(buf, strfbuf);
+ appendStringInfo(buf, "%*s", padding, strfbuf);
}
break;
case 's':
if (formatted_start_time[0] == '\0')
setup_formatted_start_time();
- if (padding != 0)
- appendStringInfo(buf, "%*s", padding, formatted_start_time);
- else
- appendStringInfoString(buf, formatted_start_time);
+ appendStringInfo(buf, "%*s", padding, formatted_start_time);
break;
case 'i':
if (MyProcPort)
@@ -2533,10 +2495,7 @@ log_line_prefix(StringInfo buf, ErrorData *edata)
int displen;
psdisp = get_ps_display(&displen);
- if (padding != 0)
- appendStringInfo(buf, "%*s", padding, psdisp);
- else
- appendBinaryStringInfo(buf, psdisp, displen);
+ appendStringInfo(buf, "%*s", padding, psdisp);
}
else if (padding != 0)
@@ -2546,37 +2505,24 @@ log_line_prefix(StringInfo buf, ErrorData *edata)
case 'r':
if (MyProcPort && MyProcPort->remote_host)
{
- if (padding != 0)
+ if (MyProcPort->remote_port && MyProcPort->remote_port[0] != '\0')
{
- if (MyProcPort->remote_port && MyProcPort->remote_port[0] != '\0')
- {
- /*
- * This option is slightly special as the port
- * number may be appended onto the end. Here we
- * need to build 1 string which contains the
- * remote_host and optionally the remote_port (if
- * set) so we can properly align the string.
- */
-
- char *hostport;
-
- hostport = psprintf("%s(%s)", MyProcPort->remote_host, MyProcPort->remote_port);
- appendStringInfo(buf, "%*s", padding, hostport);
- pfree(hostport);
- }
- else
- appendStringInfo(buf, "%*s", padding, MyProcPort->remote_host);
+ /*
+ * This option is slightly special as the port
+ * number may be appended onto the end. Here we
+ * need to build 1 string which contains the
+ * remote_host and optionally the remote_port (if
+ * set) so we can properly align the string.
+ */
+
+ char *hostport;
+
+ hostport = psprintf("%s(%s)", MyProcPort->remote_host, MyProcPort->remote_port);
+ appendStringInfo(buf, "%*s", padding, hostport);
+ pfree(hostport);
}
else
- {
- /* padding is 0, so we don't need a temp buffer */
- appendStringInfoString(buf, MyProcPort->remote_host);
- if (MyProcPort->remote_port &&
- MyProcPort->remote_port[0] != '\0')
- appendStringInfo(buf, "(%s)",
- MyProcPort->remote_port);
- }
-
+ appendStringInfo(buf, "%*s", padding, MyProcPort->remote_host);
}
else if (padding != 0)
appendStringInfoSpaces(buf,
@@ -2584,12 +2530,7 @@ log_line_prefix(StringInfo buf, ErrorData *edata)
break;
case 'h':
if (MyProcPort && MyProcPort->remote_host)
- {
- if (padding != 0)
- appendStringInfo(buf, "%*s", padding, MyProcPort->remote_host);
- else
- appendStringInfoString(buf, MyProcPort->remote_host);
- }
+ appendStringInfo(buf, "%*s", padding, MyProcPort->remote_host);
else if (padding != 0)
appendStringInfoSpaces(buf,
padding > 0 ? padding : -padding);
@@ -2604,32 +2545,21 @@ log_line_prefix(StringInfo buf, ErrorData *edata)
/* keep VXID format in sync with lockfuncs.c */
if (MyProc != NULL && MyProc->backendId != InvalidBackendId)
{
- if (padding != 0)
- {
- char strfbuf[128];
+ char strfbuf[128];
- snprintf(strfbuf, sizeof(strfbuf) - 1, "%d/%u",
- MyProc->backendId, MyProc->lxid);
+ snprintf(strfbuf, sizeof(strfbuf) - 1, "%d/%u",
+ MyProc->backendId, MyProc->lxid);
appendStringInfo(buf, "%*s", padding, strfbuf);
- }
- else
- appendStringInfo(buf, "%d/%u", MyProc->backendId, MyProc->lxid);
}
else if (padding != 0)
appendStringInfoSpaces(buf,
padding > 0 ? padding : -padding);
break;
case 'x':
- if (padding != 0)
- appendStringInfo(buf, "%*u", padding, GetTopTransactionIdIfAny());
- else
- appendStringInfo(buf, "%u", GetTopTransactionIdIfAny());
+ appendStringInfo(buf, "%*u", padding, GetTopTransactionIdIfAny());
break;
case 'e':
- if (padding != 0)
- appendStringInfo(buf, "%*s", padding, unpack_sql_state(edata->sqlerrcode));
- else
- appendStringInfoString(buf, unpack_sql_state(edata->sqlerrcode));
+ appendStringInfo(buf, "%*s", padding, unpack_sql_state(edata->sqlerrcode));
break;
default:
/* format error - ignore it */
--
2.17.0
--pAwQNkOnpTn9IO2O--
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Popcount optimization using AVX512
@ 2024-04-03 22:50 Nathan Bossart <[email protected]>
0 siblings, 1 reply; 3+ messages in thread
From: Nathan Bossart @ 2024-04-03 22:50 UTC (permalink / raw)
To: Ants Aasma <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Amonson, Paul D <[email protected]>; Tom Lane <[email protected]>; David Rowley <[email protected]>; Andres Freund <[email protected]>; Shankaran, Akash <[email protected]>; Noah Misch <[email protected]>; Matthias van de Meent <[email protected]>; [email protected] <[email protected]>
On Tue, Apr 02, 2024 at 11:30:39PM +0300, Ants Aasma wrote:
> On Tue, 2 Apr 2024 at 00:31, Nathan Bossart <[email protected]> wrote:
>> On Tue, Apr 02, 2024 at 12:11:59AM +0300, Ants Aasma wrote:
>> > What about using the masking capabilities of AVX-512 to handle the
>> > tail in the same code path? Masked out portions of a load instruction
>> > will not generate an exception. To allow byte level granularity
>> > masking, -mavx512bw is needed. Based on wikipedia this will only
>> > disable this fast path on Knights Mill (Xeon Phi), in all other cases
>> > VPOPCNTQ implies availability of BW.
>>
>> Sounds promising. IMHO we should really be sure that these kinds of loads
>> won't generate segfaults and the like due to the masked-out portions. I
>> searched around a little bit but haven't found anything that seemed
>> definitive.
>
> After sleeping on the problem, I think we can avoid this question
> altogether while making the code faster by using aligned accesses.
> Loads that straddle cache line boundaries run internally as 2 load
> operations. Gut feel says that there are enough out-of-order resources
> available to make it not matter in most cases. But even so, not doing
> the extra work is surely better. Attached is another approach that
> does aligned accesses, and thereby avoids going outside bounds.
>
> Would be interesting to see how well that fares in the small use case.
> Anything that fits into one aligned cache line should be constant
> speed, and there is only one branch, but the mask setup and folding
> the separate popcounts together should add up to about 20-ish cycles
> of overhead.
I tested your patch in comparison to v25 and saw the following:
bytes v25 v25+ants
2 1108.205 1033.132
4 1311.227 1289.373
8 1927.954 2360.113
16 2281.091 2365.408
32 3856.992 2390.688
64 3648.72 3242.498
128 4108.549 3607.148
256 4910.076 4496.852
For 2 bytes and 4 bytes, the inlining should take effect, so any difference
there is likely just noise. At 8 bytes, we are calling the function
pointer, and there is a small regression with the masking approach.
However, by 16 bytes, the masking approach is on par with v25, and it wins
for all larger buffers, although the gains seem to taper off a bit.
If we can verify this approach won't cause segfaults and can stomach the
regression between 8 and 16 bytes, I'd happily pivot to this approach so
that we can avoid the function call dance that I have in v25.
Thoughts?
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Popcount optimization using AVX512
@ 2024-04-04 03:28 David Rowley <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: David Rowley @ 2024-04-04 03:28 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Ants Aasma <[email protected]>; Alvaro Herrera <[email protected]>; Amonson, Paul D <[email protected]>; Tom Lane <[email protected]>; Andres Freund <[email protected]>; Shankaran, Akash <[email protected]>; Noah Misch <[email protected]>; Matthias van de Meent <[email protected]>; [email protected] <[email protected]>
On Thu, 4 Apr 2024 at 11:50, Nathan Bossart <[email protected]> wrote:
> If we can verify this approach won't cause segfaults and can stomach the
> regression between 8 and 16 bytes, I'd happily pivot to this approach so
> that we can avoid the function call dance that I have in v25.
>
> Thoughts?
If we're worried about regressions with some narrow range of byte
values, wouldn't it make more sense to compare that to cc4826dd5~1 at
the latest rather than to some version that's already probably faster
than PG16?
David
^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2024-04-04 03:28 UTC | newest]
Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-08-01 02:53 [PATCH v1] elog.c: Remove special case which avoided %*s format strings.. Justin Pryzby <[email protected]>
2024-04-03 22:50 Re: Popcount optimization using AVX512 Nathan Bossart <[email protected]>
2024-04-04 03:28 ` Re: Popcount optimization using AVX512 David Rowley <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox