Re: Reducing System Allocator Thrashing of ExecutorState to Alleviate FDW-related Performance Degradations

On Fri, 17 Feb 2023 at 17:40, Jonah H. Harris <jonah(dot)harris(at)gmail(dot)com> wrote:
> Yeah. There’s definitely a smarter and more reusable approach than I was proposing. A lot of that code is fairly mature and I figured more people wouldn’t want to alter it in such ways - but I’m up for it if an approach like this is the direction we’d want to go in.

I've spent quite a bit of time in this area recently and I think that
context_freelists[] is showing its age now. It does seem that slab and
generation were added before context_freelists[] (9fa6f00b), but not
by much, and those new contexts had fewer users back then. It feels a
little unfair that aset should get to cache but the other context
types don't. I don't think each context type should have some
separate cache either as that probably means more memory wasted.
Having something agnostic to if it's allocating a new context or
adding a block to an existing one seems like a good idea to me.

I think the tricky part will be the discussion around which size
classes to keep around and in which cases can we use a larger
allocation without worrying too much that it'll be wasted. We also
don't really want to make the minimum memory that a backend can keep
around too bad. Patches such as [1] are trying to reduce that. Maybe
we can just keep a handful of blocks of 1KB, 8KB and 16KB around, or
more accurately put, ALLOCSET_SMALL_INITSIZE,
ALLOCSET_DEFAULT_INITSIZE and ALLOCSET_DEFAULT_INITSIZE * 2, so that
it works correctly if someone adjusts those definitions.

I think you'll want to look at what the maximum memory a backend can
keep around in context_freelists[] and not make the worst-case memory
consumption worse than it is today.

I imagine this would be some new .c file in src/backend/utils/mmgr
which aset.c, generation.c and slab.c each call a function from to see
if we have any cached blocks of that size. You'd want to call that in
all places we call malloc() from those files apart from when aset.c
and generation.c malloc() for a dedicated block. You can probably get
away with replacing all of the free() calls with a call to another
function where you pass the pointer and the size of the block to have
it decide if it's going to free() it or cache it. I doubt you need to
care too much if the block is from a dedicated allocation or a normal
block. We'd just always free() if it's not in the size classes that
we care about.

David

[1] https://commitfest.postgresql.org/42/3867/