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
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