public inbox for [email protected]  

help / color / mirror / Atom feed

From: David Rowley <[email protected]>
To: Andres Freund <[email protected]>
Cc: [email protected]
Cc: Robert Haas <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: Tomas Vondra <[email protected]>
Subject: Re: Avoid stack frame setup in performance critical routines using tail calls
Date: Wed, 9 Aug 2023 20:44:43 +1200
Message-ID: <CAApHDvp9dA8KSEr7oE2dQn-tG6faH2Mtc3qnFgNghwNvBoq8Nw@mail.gmail.com> (raw)
In-Reply-To: <CAApHDvrZmQ6RvJ1KDj_v30FmTmAgegtmf=Woxx7HYM0g05wQHw@mail.gmail.com>
References: <[email protected]>
	<[email protected]>
	<CAApHDvrZmQ6RvJ1KDj_v30FmTmAgegtmf=Woxx7HYM0g05wQHw@mail.gmail.com>

On Fri, 21 Jul 2023 at 14:03, David Rowley <[email protected]> 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

view thread (12+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Avoid stack frame setup in performance critical routines using tail calls
  In-Reply-To: <CAApHDvp9dA8KSEr7oE2dQn-tG6faH2Mtc3qnFgNghwNvBoq8Nw@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox