Обсуждение: Use appendStringInfoSpaces more

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.

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] https://postgr.es/m/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