Use appendStringInfoSpaces more
In [1]/messages/by-id/CAApHDvrrFNSm8dF24tmYOZpvo-R5ZP+0FoqVo2XcYhRftehoRQ@mail.gmail.com I noticed a bit of a poor usage of appendStringInfoString which
just appends 4 spaces in a loop, one for each indent level of the
jsonb. It should be better just to use appendStringInfoSpaces and
just append all the spaces in one go rather than appending 4 spaces in
a loop. That'll save having to check enlargeStringInfo() once for each
loop.
I'm aiming this mostly as a cleanup patch, but after looking at the
appendStringInfoSpaces code, I thought it could be done a bit more
efficiently by using memset instead of using the while loop that keeps
track of 2 counters. memset has the option of doing more than a char
at a time, which should be useful for larger numbers of spaces.
It does seem a bit faster when appending 8 chars at least going by the
attached spaces.c file.
With -O1
$ ./spaces
while 0.536577 seconds
memset 0.326532 seconds
However, I'm not really expecting much of a performance increase from
this change. I do at least want to make sure I've not made anything
worse, so I used pgbench to run:
select jsonb_pretty(row_to_json(pg_class)::jsonb) from pg_class;
perf top says:
Master:
0.96% postgres [.] add_indent.part.0
Patched
0.25% postgres [.] add_indent.part.0
I can't really detect a certain enough TPS change over the noise. I
expect it might become more significant with more complex json that
has more than a single indent level.
I could only find 1 other instance where we use appendStringInfoString
to append spaces. I've adjusted that one too.
David
[1]: /messages/by-id/CAApHDvrrFNSm8dF24tmYOZpvo-R5ZP+0FoqVo2XcYhRftehoRQ@mail.gmail.com
Attachments:
use_appendStringInfoSpaces_more.patchtext/plain; charset=US-ASCII; name=use_appendStringInfoSpaces_more.patchDownload
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index e4621ef8d6..5212a64b1e 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -3324,7 +3324,7 @@ show_hashagg_info(AggState *aggstate, ExplainState *es)
if (!gotone)
ExplainIndentText(es);
else
- appendStringInfoString(es->str, " ");
+ appendStringInfoSpaces(es->str, 2);
appendStringInfo(es->str, "Batches: %d Memory Usage: " INT64_FORMAT "kB",
aggstate->hash_batches_used, memPeakKb);
diff --git a/src/backend/utils/adt/jsonb.c b/src/backend/utils/adt/jsonb.c
index 4ff2eced4c..0539f41c17 100644
--- a/src/backend/utils/adt/jsonb.c
+++ b/src/backend/utils/adt/jsonb.c
@@ -626,11 +626,8 @@ add_indent(StringInfo out, bool indent, int level)
{
if (indent)
{
- int i;
-
appendStringInfoCharMacro(out, '\n');
- for (i = 0; i < level; i++)
- appendBinaryStringInfo(out, " ", 4);
+ appendStringInfoSpaces(out, level * 4);
}
}
diff --git a/src/common/stringinfo.c b/src/common/stringinfo.c
index b3d3c99b8c..05b22b5c53 100644
--- a/src/common/stringinfo.c
+++ b/src/common/stringinfo.c
@@ -211,8 +211,8 @@ appendStringInfoSpaces(StringInfo str, int count)
enlargeStringInfo(str, count);
/* OK, append the spaces */
- while (--count >= 0)
- str->data[str->len++] = ' ';
+ memset(&str->data[str->len], ' ', count);
+ str->len += count;
str->data[str->len] = '\0';
}
}
On Thu, Jan 19, 2023 at 8:45 PM David Rowley <dgrowleyml@gmail.com> wrote:
In [1] I noticed a bit of a poor usage of appendStringInfoString which
just appends 4 spaces in a loop, one for each indent level of the
jsonb. It should be better just to use appendStringInfoSpaces and
just append all the spaces in one go rather than appending 4 spaces in
a loop. That'll save having to check enlargeStringInfo() once for each
loop.
Should the add_indent function also have a check to avoid making
unnecessary calls to appendStringInfoSpaces when the level is 0?
e.g.
if (indent)
{
appendStringInfoCharMacro(out, '\n');
if (level > 0)
appendStringInfoSpaces(out, level * 4);
}
V.
if (indent)
{
appendStringInfoCharMacro(out, '\n');
appendStringInfoSpaces(out, level * 4);
}
------
Kind Regards,
Peter Smith.
Fujitsu Australia
Peter Smith <smithpb2250@gmail.com> writes:
Should the add_indent function also have a check to avoid making
unnecessary calls to appendStringInfoSpaces when the level is 0?
Seems like unnecessary extra notation, seeing that appendStringInfoSpaces
will fall out quickly for a zero argument.
regards, tom lane
On Fri, 20 Jan 2023 at 10:25, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Peter Smith <smithpb2250@gmail.com> writes:
Should the add_indent function also have a check to avoid making
unnecessary calls to appendStringInfoSpaces when the level is 0?Seems like unnecessary extra notation, seeing that appendStringInfoSpaces
will fall out quickly for a zero argument.
Yeah agreed. As far as I see it, the level will only be 0 before the
first WJB_BEGIN_OBJECT and those appear to be the first thing in the
document, so we'll only indent level 0 once and everything else will
be > 0. So, it also seems to me that the additional check is more
likely to cost more than it would save.
David
On Fri, 20 Jan 2023 at 10:23, Peter Smith <smithpb2250@gmail.com> wrote:
Should the add_indent function also have a check to avoid making
unnecessary calls to appendStringInfoSpaces when the level is 0?
Although I didn't opt to do that, thank you for having a look.
I do think the patch is trivially simple and nobody seems against it,
so I've now pushed it.
David