From c69840cd85767fa7ee4d1688a605a0978c3fc39f Mon Sep 17 00:00:00 2001 From: David Rowley Date: Mon, 26 Feb 2024 20:05:34 +1300 Subject: [PATCH v4 3/3] Make unsetting the isReset flag the MemoryContext's job Up until now, setting this boolean flag to false has been the job of the allocation function in mcxt.c. Making this the job of the MemoryContext implementation can make things more efficient as adjusting the flag can just be put in code paths that are only possible to reach in cases where we may not have already done an allocation since the context was created or reset. Discussion: https://postgr.es/m/20240222225343.7a57fiv7quehhlur@awork3.anarazel.de --- src/backend/utils/mmgr/aset.c | 11 ++++++++++ src/backend/utils/mmgr/generation.c | 2 ++ src/backend/utils/mmgr/mcxt.c | 31 ++++++++++++++++------------- src/backend/utils/mmgr/slab.c | 14 +++++++++++++ src/include/nodes/memnodes.h | 5 ++++- 5 files changed, 48 insertions(+), 15 deletions(-) diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c index 4e52eb063c..a1c59381d5 100644 --- a/src/backend/utils/mmgr/aset.c +++ b/src/backend/utils/mmgr/aset.c @@ -711,6 +711,7 @@ AllocSetAllocLarge(MemoryContext context, Size size, int flags) chunk_size = MAXALIGN(size); #endif + context->isReset = false; blksize = chunk_size + ALLOC_BLOCKHDRSZ + ALLOC_CHUNKHDRSZ; block = (AllocBlock) malloc(blksize); if (block == NULL) @@ -1000,6 +1001,9 @@ AllocSetAlloc(MemoryContext context, Size size, int flags) { AllocFreeListLink *link = GetFreeListLink(chunk); + /* should already be unset if we've something in the freelist */ + Assert(context->isReset == false); + /* Allow access to the chunk header. */ VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOC_CHUNKHDRSZ); @@ -1040,6 +1044,13 @@ AllocSetAlloc(MemoryContext context, Size size, int flags) block = set->blocks; availspace = block->endptr - block->freeptr; + /* + * We must un-reset the context here as lack of space for this allocation + * on a block is no guarantee that we've already seen an allocation as + * the given block could be the keeper block. + */ + context->isReset = false; + /* * If there is enough room in the active allocation block, we will put the * chunk into that block. Else must start a new one. diff --git a/src/backend/utils/mmgr/generation.c b/src/backend/utils/mmgr/generation.c index 4b4155b863..5a351c26df 100644 --- a/src/backend/utils/mmgr/generation.c +++ b/src/backend/utils/mmgr/generation.c @@ -352,6 +352,8 @@ GenerationAlloc(MemoryContext context, Size size, int flags) Size chunk_size; Size required_size; + context->isReset = false; + Assert(GenerationIsValid(set)); #ifdef MEMORY_CONTEXT_CHECKING diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c index c214d323c8..01f3bec028 100644 --- a/src/backend/utils/mmgr/mcxt.c +++ b/src/backend/utils/mmgr/mcxt.c @@ -1070,8 +1070,6 @@ MemoryContextAlloc(MemoryContext context, Size size) Assert(MemoryContextIsValid(context)); AssertNotInCriticalSection(context); - context->isReset = false; - /* * For efficiency reasons, we purposefully offload the handling of * allocation failures to the MemoryContextMethods implementation as this @@ -1084,6 +1082,8 @@ MemoryContextAlloc(MemoryContext context, Size size) */ ret = context->methods->alloc(context, size, 0); + Assert(!context->isReset); + VALGRIND_MEMPOOL_ALLOC(context, ret, size); return ret; @@ -1104,10 +1104,10 @@ MemoryContextAllocZero(MemoryContext context, Size size) Assert(MemoryContextIsValid(context)); AssertNotInCriticalSection(context); - context->isReset = false; - ret = context->methods->alloc(context, size, 0); + Assert(!context->isReset); + VALGRIND_MEMPOOL_ALLOC(context, ret, size); /* @@ -1135,9 +1135,10 @@ MemoryContextAllocExtended(MemoryContext context, Size size, int flags) AllocSizeIsValid(size))) elog(ERROR, "invalid memory alloc request size %zu", size); - context->isReset = false; - ret = context->methods->alloc(context, size, flags); + + Assert(!context->isReset); + if (unlikely(ret == NULL)) return NULL; @@ -1211,8 +1212,6 @@ palloc(Size size) Assert(MemoryContextIsValid(context)); AssertNotInCriticalSection(context); - context->isReset = false; - /* * For efficiency reasons, we purposefully offload the handling of * allocation failures to the MemoryContextMethods implementation as this @@ -1224,6 +1223,9 @@ palloc(Size size) * function instead. */ ret = context->methods->alloc(context, size, 0); + + Assert(!context->isReset); + /* We expect OOM to be handled by the alloc function */ Assert(ret != NULL); VALGRIND_MEMPOOL_ALLOC(context, ret, size); @@ -1241,10 +1243,10 @@ palloc0(Size size) Assert(MemoryContextIsValid(context)); AssertNotInCriticalSection(context); - context->isReset = false; - ret = context->methods->alloc(context, size, 0); + Assert(!context->isReset); + VALGRIND_MEMPOOL_ALLOC(context, ret, size); MemSetAligned(ret, 0, size); @@ -1262,9 +1264,10 @@ palloc_extended(Size size, int flags) Assert(MemoryContextIsValid(context)); AssertNotInCriticalSection(context); - context->isReset = false; - ret = context->methods->alloc(context, size, flags); + + Assert(!context->isReset); + if (unlikely(ret == NULL)) { return NULL; @@ -1532,8 +1535,6 @@ MemoryContextAllocHuge(MemoryContext context, Size size) Assert(MemoryContextIsValid(context)); AssertNotInCriticalSection(context); - context->isReset = false; - /* * For efficiency reasons, we purposefully offload the handling of * allocation failures to the MemoryContextMethods implementation as this @@ -1546,6 +1547,8 @@ MemoryContextAllocHuge(MemoryContext context, Size size) */ ret = context->methods->alloc(context, size, MCXT_ALLOC_HUGE); + Assert(!context->isReset); + VALGRIND_MEMPOOL_ALLOC(context, ret, size); return ret; diff --git a/src/backend/utils/mmgr/slab.c b/src/backend/utils/mmgr/slab.c index bc91446cb3..1160aa7e20 100644 --- a/src/backend/utils/mmgr/slab.c +++ b/src/backend/utils/mmgr/slab.c @@ -533,6 +533,12 @@ SlabAlloc(MemoryContext context, Size size, int flags) { dlist_node *node = dclist_pop_head_node(&slab->emptyblocks); + /* + * We should already have already un-reset the context if we've + * got a block on the emptyblocks list. + */ + Assert(!context->isReset); + block = dlist_container(SlabBlock, node, node); /* @@ -546,6 +552,8 @@ SlabAlloc(MemoryContext context, Size size, int flags) } else { + context->isReset = false; + block = (SlabBlock *) malloc(slab->blockSize); if (unlikely(block == NULL)) @@ -581,6 +589,12 @@ SlabAlloc(MemoryContext context, Size size, int flags) Assert(!dlist_is_empty(blocklist)); + /* + * We should already have already un-reset the context if we've got a + * non-zero curBlocklistIndex + */ + Assert(!context->isReset); + /* grab the block from the blocklist */ block = dlist_head_element(SlabBlock, node, blocklist); diff --git a/src/include/nodes/memnodes.h b/src/include/nodes/memnodes.h index edc0257f36..72539cfc5d 100644 --- a/src/include/nodes/memnodes.h +++ b/src/include/nodes/memnodes.h @@ -61,7 +61,10 @@ typedef struct MemoryContextMethods * Function to handle memory allocation requests of 'size' to allocate * memory into the given 'context'. The function must handle flags * MCXT_ALLOC_HUGE and MCXT_ALLOC_NO_OOM. MCXT_ALLOC_ZERO is handled by - * the calling function. + * the calling function. The implementation must handle setting + * MemoryContext.isReset to false before returning. Leaving this up to + * the implementing function can allow this flag to be unset more + * efficiently. */ void *(*alloc) (MemoryContext context, Size size, int flags); -- 2.40.1.windows.1