MIME-Version: 1.0
References: <20210719195950.gavgs6ujzmjfaiig@alap3.anarazel.de>
 <20230719085236.jltxal2eztqrprfg@awork3.anarazel.de> <CAApHDvrZmQ6RvJ1KDj_v30FmTmAgegtmf=Woxx7HYM0g05wQHw@mail.gmail.com>
In-Reply-To: <CAApHDvrZmQ6RvJ1KDj_v30FmTmAgegtmf=Woxx7HYM0g05wQHw@mail.gmail.com>
From: David Rowley <dgrowleyml@gmail.com>
Date: Wed, 9 Aug 2023 20:44:43 +1200
Message-ID: <CAApHDvp9dA8KSEr7oE2dQn-tG6faH2Mtc3qnFgNghwNvBoq8Nw@mail.gmail.com>
Subject: Re: Avoid stack frame setup in performance critical routines using
 tail calls
To: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers@postgresql.org, Robert Haas <robertmhaas@gmail.com>, 
	Michael Paquier <michael.paquier@gmail.com>, Tomas Vondra <tv@fuzzy.cz>
Content-Type: text/plain; charset="UTF-8"
Archived-At: <https://www.postgresql.org/message-id/CAApHDvp9dA8KSEr7oE2dQn-tG6faH2Mtc3qnFgNghwNvBoq8Nw%40mail.gmail.com>
Precedence: bulk

On Fri, 21 Jul 2023 at 14:03, David Rowley <dgrowleyml@gmail.com> wrote:
> I'll reply back with a more detailed review next week.

Here's a review of v2-0001:

1.

/*
* XXX: Should this also be moved into alloc()? We could possibly avoid
* zeroing in some cases (e.g. if we used mmap() ourselves.
*/
MemSetAligned(ret, 0, size);

Maybe this should be moved to the alloc function.  It would allow us
to get rid of this:

#define palloc0fast(sz) \
( MemSetTest(0, sz) ? \
MemoryContextAllocZeroAligned(CurrentMemoryContext, sz) : \
MemoryContextAllocZero(CurrentMemoryContext, sz) )

If we do the zeroing inside the alloc function then it can always use
the MemoryContextAllocZeroAligned version providing we zero before
setting the sentinel byte.

It would allow the tail call in the palloc0() case, but the drawback
would be having to check for the MCXT_ALLOC_ZERO flag in the alloc
function. I wonder if that branch would be predictable in most cases,
e.g. the parser will be making lots of nodes and want to zero all
allocations, but the executor won't be doing much of that. There will
be a mix of zeroing and not zeroing in the planner, mostly not, I
think.

2. Why do you need to add the NULL check here?

 #ifdef USE_VALGRIND
- if (method != MCTX_ALIGNED_REDIRECT_ID)
+ if (ret != NULL && method != MCTX_ALIGNED_REDIRECT_ID)
  VALGRIND_MEMPOOL_CHANGE(context, pointer, ret, size);
 #endif

I know it's just valgrind code and performance does not matter, but
the realloc flags are being passed as 0, so allocation failures won't
return.

3.

/*
* XXX: Probably no need to check for huge allocations, we only support
* one size? Which could theoretically be huge, but that'd not make
* sense...
*/

They can't be huge per Assert(fullChunkSize <= MEMORYCHUNK_MAX_VALUE)
in SlabContextCreate().

4. It would be good to see some API documentation in the
MemoryContextMethods struct.  This adds a lot of responsibility onto
the context implementation without any extra documentation to explain
what, for example, palloc is responsible for and what the alloc
function needs to do itself.

David