Re: SendRowDescriptionMessage() is slow for queries with a lot of columns

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 not overlap with
  buf->data
- Even with type based strict aliasing, using char * type data and
  memcpy prevents that type of analysis - but restrict promises that
  there's no overlap - which we know there isn't.

Makes sense?

Greetings,

Andres Freund