On Fri, Jul 31, 2020 at 11:50 AM Andres Freund <andres@anarazel.de> wrote:
> I hope the above makes this make sene now? It's about subsequent uses of
> the StringInfo, rather than the body of resetStringInfo itself.
That does make sense, except that
https://en.cppreference.com/w/c/language/restrict says "During each
execution of a block in which a restricted pointer P is declared
(typically each execution of a function body in which P is a function
parameter), if some object that is accessible through P (directly or
indirectly) is modified, by any means, then all accesses to that
object (both reads and writes) in that block must occur through P
(directly or indirectly), otherwise the behavior is undefined." So my
interpretation of this was that it couldn't really affect what
happened outside of the function itself, even if the compiler chose to
perform inlining. But I think see what you're saying: the *inference*
is only valid with respect to restrict pointers in a particular
function, but what can be optimized as a result of that inference may
be something further afield, if inlining is performed. Perhaps we
could add a comment about this, e.g.
Marking these pointers with pg_restrict tells the compiler that str
and str->data can't overlap, which may allow the compiler to optimize
better when this code is inlined. For example, it may be possible to
keep str->data in a register across consecutive appendStringInfoString
operations.
Since pg_restrict is not widely used, I think it's worth adding this
kind of annotation, lest other hackers get confused. I'm probably not
the only one who isn't on top of this.
> > In appendStringInfoChar, why do we need to cast to restrict twice? Can
> > we not just do something like this:
> >
> > char *pg_restrict ep = str->data+str->len;
> > ep[0] = ch;
> > ep[1] = '\0';
>
> I don't think that'd tell the compiler that this couldn't overlap with
> str itself? A single 'restrict' can never (?) help, you need *two*
> things that are marked as not overlapping in any way.
But what's the difference between:
+ *(char *pg_restrict) (str->data + str->len) = ch;
+ str->len++;
+ *(char *pg_restrict) (str->data + str->len) = '\0';
And:
char *pg_restrict ep = str->data+str->len;
ep[0] = ch;
ep[1] = '\0';
++str->len;
Whether or not str itself is marked restricted is another question;
what I'm talking about is why we need to repeat (char *pg_restrict)
(str->data + str->len).
I don't have any further comment on the remainder of your reply.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
