v3-0002-fixup-Optimize-palloc-etc-to-allow-sibling-calls.patch

From 9b6ffdceb3c6f592d3d1597b19f1240d4a78d848 Mon Sep 17 00:00:00 2001
From: David Rowley <dgrowley@gmail.com>
Date: Fri, 23 Feb 2024 00:27:07 +1300
Subject: [PATCH v3 2/2] fixup! Optimize palloc() etc to allow sibling calls

---
 src/backend/utils/mmgr/mcxt.c         | 67 +++++++++++++++++++++++++--
 src/backend/utils/mmgr/slab.c         | 11 ++---
 src/include/nodes/memnodes.h          | 38 +++++++++++++++
 src/include/utils/memutils_internal.h |  7 +--
 4 files changed, 110 insertions(+), 13 deletions(-)

diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 2947b2046b..c214d323c8 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -1023,6 +1023,12 @@ MemoryContextCreate(MemoryContext node,
 	VALGRIND_CREATE_MEMPOOL(node, 0, false);
 }
 
+/*
+ * MemoryContextAllocationFailure
+ *		For use by MemoryContextMethods implementations to handle when malloc
+ *		returns NULL.  The bahavior is specific to whether MCXT_ALLOC_NO_OOM
+ *		is in 'flags'.
+ */
 void *
 MemoryContextAllocationFailure(MemoryContext context, Size size, int flags)
 {
@@ -1038,6 +1044,11 @@ MemoryContextAllocationFailure(MemoryContext context, Size size, int flags)
 	return NULL;
 }
 
+/*
+ * MemoryContextSizeFailure
+ *		For use by MemoryContextMethods implementations to handle invalid
+ *		memory allocation request sizes.
+ */
 void
 MemoryContextSizeFailure(MemoryContext context, Size size, int flags)
 {
@@ -1061,6 +1072,16 @@ MemoryContextAlloc(MemoryContext context, Size size)
 
 	context->isReset = false;
 
+	/*
+	 * For efficiency reasons, we purposefully offload the handling of
+	 * allocation failures to the MemoryContextMethods implementation as this
+	 * allows these checks to be performed only when an actual malloc needs to
+	 * be done to request more memory from the OS.  Additionally, not having
+	 * to execute any instructions after this call allows the compiler to use
+	 * the tailcall optimization.  If you're considering adding code after
+	 * this call, consider making it the responsibility of the 'alloc'
+	 * function instead.
+	 */
 	ret = context->methods->alloc(context, size, 0);
 
 	VALGRIND_MEMPOOL_ALLOC(context, ret, size);
@@ -1192,9 +1213,19 @@ palloc(Size size)
 
 	context->isReset = false;
 
+	/*
+	 * For efficiency reasons, we purposefully offload the handling of
+	 * allocation failures to the MemoryContextMethods implementation as this
+	 * allows these checks to be performed only when an actual malloc needs to
+	 * be done to request more memory from the OS.  Additionally, not having
+	 * to execute any instructions after this call allows the compiler to use
+	 * the tailcall optimization.  If you're considering adding code after
+	 * this call, consider making it the responsibility of the 'alloc'
+	 * function instead.
+	 */
 	ret = context->methods->alloc(context, size, 0);
-
-	Assert(ret != 0);
+	/* We expect OOM to be handled by the alloc function */
+	Assert(ret != NULL);
 	VALGRIND_MEMPOOL_ALLOC(context, ret, size);
 
 	return ret;
@@ -1410,10 +1441,20 @@ repalloc(void *pointer, Size size)
 	/* isReset must be false already */
 	Assert(!context->isReset);
 
+	/*
+	 * For efficiency reasons, we purposefully offload the handling of
+	 * allocation failures to the MemoryContextMethods implementation as this
+	 * allows these checks to be performed only when an actual malloc needs to
+	 * be done to request more memory from the OS.  Additionally, not having
+	 * to execute any instructions after this call allows the compiler to use
+	 * the tailcall optimization.  If you're considering adding code after
+	 * this call, consider making it the responsibility of the 'realloc'
+	 * function instead.
+	 */
 	ret = MCXT_METHOD(pointer, realloc) (pointer, size, 0);
 
 #ifdef USE_VALGRIND
-	if (ret != NULL && method != MCTX_ALIGNED_REDIRECT_ID)
+	if (method != MCTX_ALIGNED_REDIRECT_ID)
 		VALGRIND_MEMPOOL_CHANGE(context, pointer, ret, size);
 #endif
 
@@ -1438,6 +1479,16 @@ repalloc_extended(void *pointer, Size size, int flags)
 	/* isReset must be false already */
 	Assert(!context->isReset);
 
+	/*
+	 * For efficiency reasons, we purposefully offload the handling of
+	 * allocation failures to the MemoryContextMethods implementation as this
+	 * allows these checks to be performed only when an actual malloc needs to
+	 * be done to request more memory from the OS.  Additionally, not having
+	 * to execute any instructions after this call allows the compiler to use
+	 * the tailcall optimization.  If you're considering adding code after
+	 * this call, consider making it the responsibility of the 'realloc'
+	 * function instead.
+	 */
 	ret = MCXT_METHOD(pointer, realloc) (pointer, size, flags);
 	if (unlikely(ret == NULL))
 		return NULL;
@@ -1483,6 +1534,16 @@ MemoryContextAllocHuge(MemoryContext context, Size size)
 
 	context->isReset = false;
 
+	/*
+	 * For efficiency reasons, we purposefully offload the handling of
+	 * allocation failures to the MemoryContextMethods implementation as this
+	 * allows these checks to be performed only when an actual malloc needs to
+	 * be done to request more memory from the OS.  Additionally, not having
+	 * to execute any instructions after this call allows the compiler to use
+	 * the tailcall optimization.  If you're considering adding code after
+	 * this call, consider making it the responsibility of the 'alloc'
+	 * function instead.
+	 */
 	ret = context->methods->alloc(context, size, MCXT_ALLOC_HUGE);
 
 	VALGRIND_MEMPOOL_ALLOC(context, ret, size);
diff --git a/src/backend/utils/mmgr/slab.c b/src/backend/utils/mmgr/slab.c
index 1345c8d500..bc91446cb3 100644
--- a/src/backend/utils/mmgr/slab.c
+++ b/src/backend/utils/mmgr/slab.c
@@ -504,17 +504,14 @@ SlabAlloc(MemoryContext context, Size size, int flags)
 
 	Assert(SlabIsValid(slab));
 
-	/*
-	 * 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...
-	 */
-
 	/* sanity check that this is pointing to a valid blocklist */
 	Assert(slab->curBlocklistIndex >= 0);
 	Assert(slab->curBlocklistIndex <= SlabBlocklistIndex(slab, slab->chunksPerBlock));
 
-	/* make sure we only allow correct request size */
+	/*
+	 * Make sure we only allow correct request size.  This doubles as the
+	 * MemoryContextCheckSize check.
+	 */
 	if (unlikely(size != slab->chunkSize))
 		elog(ERROR, "unexpected alloc chunk size %zu (expected %u)",
 			 size, slab->chunkSize);
diff --git a/src/include/nodes/memnodes.h b/src/include/nodes/memnodes.h
index cc8cc58a0c..edc0257f36 100644
--- a/src/include/nodes/memnodes.h
+++ b/src/include/nodes/memnodes.h
@@ -57,20 +57,58 @@ typedef void (*MemoryStatsPrintFunc) (MemoryContext context, void *passthru,
 
 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.
+	 */
 	void	   *(*alloc) (MemoryContext context, Size size, int flags);
+
 	/* call this free_p in case someone #define's free() */
 	void		(*free_p) (void *pointer);
+
+	/*
+	 * Function to handle a size change request for an existing allocation.
+	 * The implementation must handle flags MCXT_ALLOC_HUGE and
+	 * MCXT_ALLOC_NO_OOM.  MCXT_ALLOC_ZERO is handled by the calling function.
+	 */
 	void	   *(*realloc) (void *pointer, Size size, int flags);
+
+	/*
+	 * Invalidate all previous allocations in the given memory context and
+	 * prepare the context for a new set of allocations.  Implementations may
+	 * optionally free() excess memory back to the OS during this time.
+	 */
 	void		(*reset) (MemoryContext context);
+
+	/* Free all memory consumed by the given MemoryContext. */
 	void		(*delete_context) (MemoryContext context);
+
+	/* Return the MemoryContext that the given pointer belongs to. */
 	MemoryContext (*get_chunk_context) (void *pointer);
+
+	/*
+	 * Return the number of bytes consumed by the given pointer within its
+	 * memory context, including the overhead of alignment and chunk headers.
+	 */
 	Size		(*get_chunk_space) (void *pointer);
+
+	/*
+	 * Return true if the given MemoryContext has not had any allocations
+	 * since it was created or last reset.
+	 */
 	bool		(*is_empty) (MemoryContext context);
 	void		(*stats) (MemoryContext context,
 						  MemoryStatsPrintFunc printfunc, void *passthru,
 						  MemoryContextCounters *totals,
 						  bool print_to_stderr);
 #ifdef MEMORY_CONTEXT_CHECKING
+
+	/*
+	 * Perform validation checks on the given context and raise any discovered
+	 * anomalies as WARNINGs.
+	 */
 	void		(*check) (MemoryContext context);
 #endif
 } MemoryContextMethods;
diff --git a/src/include/utils/memutils_internal.h b/src/include/utils/memutils_internal.h
index e5cecd7122..ad1048fd82 100644
--- a/src/include/utils/memutils_internal.h
+++ b/src/include/utils/memutils_internal.h
@@ -133,9 +133,11 @@ extern void MemoryContextCreate(MemoryContext node,
 								MemoryContext parent,
 								const char *name);
 
-extern void *MemoryContextAllocationFailure(MemoryContext context, Size size, int flags);
+extern void *MemoryContextAllocationFailure(MemoryContext context, Size size,
+											int flags);
 
-extern void MemoryContextSizeFailure(MemoryContext context, Size size, int flags) pg_attribute_noreturn();
+extern void MemoryContextSizeFailure(MemoryContext context, Size size,
+									 int flags) pg_attribute_noreturn();
 
 static inline void
 MemoryContextCheckSize(MemoryContext context, Size size, int flags)
@@ -147,5 +149,4 @@ MemoryContextCheckSize(MemoryContext context, Size size, int flags)
 	}
 }
 
-
 #endif							/* MEMUTILS_INTERNAL_H */
-- 
2.40.1