On 2017-10-03 11:06:08 -0400, Robert Haas wrote:
> On Tue, Oct 3, 2017 at 3:55 AM, Andres Freund <andres@anarazel.de> wrote:
> > Attached is a revised version of this patchset.
>
> I don't much like the functions with "_pre" affixed to their names.
> It's not at all clear that "pre" means "preallocated"; it sounds more
> like you're doing something ahead of time. I wonder about maybe
> calling these e.g. pq_writeint16, with "write" meaning "assume
> preallocation" and "send" meaning "don't assume preallocation". There
> could be other ideas, too.
I can live with write, although I don't think it jibes well with
the pq_send* naming.
> > 3) The use of restrict, with a configure based fallback, is something
> > we've not done before, but it's C99 and delivers significantly more
> > efficient code. Any arguments against?
> Also, unless I'm missing something, there's nothing to keep
> pq_sendintXX_pre from causing an alignment fault except unbridled
> optimism...
Fair argument, I'll replace it back with a fixed-length memcpy. At least
my gcc optimizes that away again - I ended up with the plain assignment
while debugging the above, due to the lack of restrict.
> It's pretty unobvious why it helps here. I think you should add
> comments.
Will. I'd stared at this long enough that I thought it'd be obvious. But
it took me a couple hours to get there, so ... yes. The reason it's
needed here is that given:
static inline void
pq_sendint8_pre(StringInfo restrict buf, int8 i)
{int32 ni = pg_hton32(i);
Assert(buf->len + sizeof(i) <= buf->maxlen);memcpy((char* restrict) (buf->data + buf->len), &ni, sizeof(i));buf->len +=
sizeof(i);
}
without the restrict the compiler has no way to know that buf, buf->len,
*(buf->data + x) do not overlap. Therefore buf->len cannot be kept in a
register across subsequent pq_sendint*_pre calls, but has to be stored
and loaded before each of the the memcpy calls. There's two reasons for
that:
- We compile -fno-strict-aliasing. That prevents the compiler from doing type based inference that buf and buf->len do
notoverlap with buf->data
- Even with type based strict aliasing, using char * type data and memcpy prevents that type of analysis - but restrict
promisesthat there's no overlap - which we know there isn't.
Makes sense?
Greetings,
Andres Freund
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
