On Sat, 2025-03-22 at 09:39 -0700, Jeff Davis wrote:
> For some reason I'm getting a decline of about 3% in the c.sql test
> that seems to be associated with the accessor functions, even when
> inlined. I'm also not seeing as much benefit from the inlining of the
> MemoryContextMemAllocated(). But these mixed test results are minor
> compared with the memory savings of 35% and the more consistently-
> improved performance of 5% on the larger test (which is also
> integers),
> so I plan to commit it.
Committed, except for v9-0007. (Note that some commits were combined;
they were only separated originally for performance testing.)
I attached a new version of v9-0007, now v10-0001, that uses
recurse=false for two of the memory contexts. I didn't see a major
speedup, but posting here anyway.
Regards,
Jeff Davis
From 33db55a517a413109daf249369a20ae4b8bd2ab9 Mon Sep 17 00:00:00 2001
From: Jeff Davis <j...@j-davis.com>
Date: Tue, 4 Mar 2025 13:21:35 -0800
Subject: [PATCH v10] Inline MemoryContextMemAllocated().
Suggested-by: David Rowley <dgrowle...@gmail.com>
Discussion: https://postgr.es/m/CAApHDvpN4v3t_sdz4dvrv1Fx_ZPw=twsnxuteytryp7lfz5...@mail.gmail.com
---
src/backend/executor/nodeAgg.c | 8 ++--
src/backend/utils/mmgr/mcxt.c | 68 ----------------------------------
src/include/nodes/memnodes.h | 68 ++++++++++++++++++++++++++++++++++
src/include/utils/memutils.h | 1 -
4 files changed, 72 insertions(+), 73 deletions(-)
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index f83fc16c5c8..f2c0e535b8f 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -1868,9 +1868,9 @@ hash_agg_check_limits(AggState *aggstate)
{
uint64 ngroups = aggstate->hash_ngroups_current;
Size meta_mem = MemoryContextMemAllocated(aggstate->hash_metacxt,
- true);
+ false);
Size entry_mem = MemoryContextMemAllocated(aggstate->hash_tablecxt,
- true);
+ false);
Size tval_mem = MemoryContextMemAllocated(aggstate->hashcontext->ecxt_per_tuple_memory,
true);
Size total_mem = meta_mem + entry_mem + tval_mem;
@@ -1957,10 +1957,10 @@ hash_agg_update_metrics(AggState *aggstate, bool from_tape, int npartitions)
return;
/* memory for the hash table itself */
- meta_mem = MemoryContextMemAllocated(aggstate->hash_metacxt, true);
+ meta_mem = MemoryContextMemAllocated(aggstate->hash_metacxt, false);
/* memory for hash entries */
- entry_mem = MemoryContextMemAllocated(aggstate->hash_tablecxt, true);
+ entry_mem = MemoryContextMemAllocated(aggstate->hash_tablecxt, false);
/* memory for byref transition states */
hashkey_mem = MemoryContextMemAllocated(aggstate->hashcontext->ecxt_per_tuple_memory, true);
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 91060de0ab7..ea08bcf8ed2 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -232,50 +232,6 @@ GetMemoryChunkHeader(const void *pointer)
return header;
}
-/*
- * MemoryContextTraverseNext
- * Helper function to traverse all descendants of a memory context
- * without recursion.
- *
- * Recursion could lead to out-of-stack errors with deep context hierarchies,
- * which would be unpleasant in error cleanup code paths.
- *
- * To process 'context' and all its descendants, use a loop like this:
- *
- * <process 'context'>
- * for (MemoryContext curr = context->firstchild;
- * curr != NULL;
- * curr = MemoryContextTraverseNext(curr, context))
- * {
- * <process 'curr'>
- * }
- *
- * This visits all the contexts in pre-order, that is a node is visited
- * before its children.
- */
-static MemoryContext
-MemoryContextTraverseNext(MemoryContext curr, MemoryContext top)
-{
- /* After processing a node, traverse to its first child if any */
- if (curr->firstchild != NULL)
- return curr->firstchild;
-
- /*
- * After processing a childless node, traverse to its next sibling if
- * there is one. If there isn't, traverse back up to the parent (which
- * has already been visited, and now so have all its descendants). We're
- * done if that is "top", otherwise traverse to its next sibling if any,
- * otherwise repeat moving up.
- */
- while (curr->nextchild == NULL)
- {
- curr = curr->parent;
- if (curr == top)
- return NULL;
- }
- return curr->nextchild;
-}
-
/*
* Support routines to trap use of invalid memory context method IDs
* (from calling pfree or the like on a bogus pointer). As a possible
@@ -754,30 +710,6 @@ MemoryContextIsEmpty(MemoryContext context)
return context->methods->is_empty(context);
}
-/*
- * Find the memory allocated to blocks for this memory context. If recurse is
- * true, also include children.
- */
-Size
-MemoryContextMemAllocated(MemoryContext context, bool recurse)
-{
- Size total = context->mem_allocated;
-
- Assert(MemoryContextIsValid(context));
-
- if (recurse)
- {
- for (MemoryContext curr = context->firstchild;
- curr != NULL;
- curr = MemoryContextTraverseNext(curr, context))
- {
- total += curr->mem_allocated;
- }
- }
-
- return total;
-}
-
/*
* Return the memory consumption statistics about the given context and its
* children.
diff --git a/src/include/nodes/memnodes.h b/src/include/nodes/memnodes.h
index 5807ef805bd..969b4d6ef33 100644
--- a/src/include/nodes/memnodes.h
+++ b/src/include/nodes/memnodes.h
@@ -149,4 +149,72 @@ typedef struct MemoryContextData
IsA((context), GenerationContext) || \
IsA((context), BumpContext)))
+/*
+ * MemoryContextTraverseNext
+ * Helper function to traverse all descendants of a memory context
+ * without recursion.
+ *
+ * Recursion could lead to out-of-stack errors with deep context hierarchies,
+ * which would be unpleasant in error cleanup code paths.
+ *
+ * To process 'context' and all its descendants, use a loop like this:
+ *
+ * <process 'context'>
+ * for (MemoryContext curr = context->firstchild;
+ * curr != NULL;
+ * curr = MemoryContextTraverseNext(curr, context))
+ * {
+ * <process 'curr'>
+ * }
+ *
+ * This visits all the contexts in pre-order, that is a node is visited
+ * before its children.
+ */
+static inline MemoryContext
+MemoryContextTraverseNext(MemoryContext curr, MemoryContext top)
+{
+ /* After processing a node, traverse to its first child if any */
+ if (curr->firstchild != NULL)
+ return curr->firstchild;
+
+ /*
+ * After processing a childless node, traverse to its next sibling if
+ * there is one. If there isn't, traverse back up to the parent (which
+ * has already been visited, and now so have all its descendants). We're
+ * done if that is "top", otherwise traverse to its next sibling if any,
+ * otherwise repeat moving up.
+ */
+ while (curr->nextchild == NULL)
+ {
+ curr = curr->parent;
+ if (curr == top)
+ return NULL;
+ }
+ return curr->nextchild;
+}
+
+/*
+ * Find the memory allocated to blocks for this memory context. If recurse is
+ * true, also include children.
+ */
+static inline Size
+MemoryContextMemAllocated(MemoryContext context, bool recurse)
+{
+ Size total = context->mem_allocated;
+
+ Assert(MemoryContextIsValid(context));
+
+ if (recurse)
+ {
+ for (MemoryContext curr = context->firstchild;
+ curr != NULL;
+ curr = MemoryContextTraverseNext(curr, context))
+ {
+ total += curr->mem_allocated;
+ }
+ }
+
+ return total;
+}
+
#endif /* MEMNODES_H */
diff --git a/src/include/utils/memutils.h b/src/include/utils/memutils.h
index 8abc26abce2..4f153d0e067 100644
--- a/src/include/utils/memutils.h
+++ b/src/include/utils/memutils.h
@@ -83,7 +83,6 @@ extern MemoryContext GetMemoryChunkContext(void *pointer);
extern Size GetMemoryChunkSpace(void *pointer);
extern MemoryContext MemoryContextGetParent(MemoryContext context);
extern bool MemoryContextIsEmpty(MemoryContext context);
-extern Size MemoryContextMemAllocated(MemoryContext context, bool recurse);
extern void MemoryContextMemConsumed(MemoryContext context,
MemoryContextCounters *consumed);
extern void MemoryContextStats(MemoryContext context);
--
2.34.1
