From 88177f77b17ef478da1dbca9acb5e3a61b346613 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 16 Dec 2002 16:22:46 +0000 Subject: [PATCH] Code review for palloc0 patch --- avoid dangerous and unnecessary practice of evaluating MemSet's arguments multiple times, except for the special case of newNode(), where we can assume the argument is a constant sizeof() operator. Also, add GetMemoryChunkContext() to mcxt.c's API, in preparation for fixing recent GEQO breakage. --- src/backend/nodes/nodes.c | 13 ++----- src/backend/utils/mmgr/mcxt.c | 66 ++++++++++++++++++++++++++++--- src/include/c.h | 90 +++++++++++++++++++++++++++++++------------ src/include/nodes/nodes.h | 16 +++++--- src/include/utils/memutils.h | 3 +- src/include/utils/palloc.h | 24 ++++++++---- 6 files changed, 160 insertions(+), 52 deletions(-) diff --git a/src/backend/nodes/nodes.c b/src/backend/nodes/nodes.c index 6d1deadb0b..f71bd020ce 100644 --- a/src/backend/nodes/nodes.c +++ b/src/backend/nodes/nodes.c @@ -9,7 +9,7 @@ * * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/nodes/nodes.c,v 1.18 2002/11/10 02:17:25 momjian Exp $ + * $Header: /cvsroot/pgsql/src/backend/nodes/nodes.c,v 1.19 2002/12/16 16:22:46 tgl Exp $ * * HISTORY * Andrew Yu Oct 20, 1994 file creation @@ -17,16 +17,11 @@ *------------------------------------------------------------------------- */ #include "postgres.h" + #include "nodes/nodes.h" /* - * newNode - - * create a new node of the specified size and tag the node with the - * specified tag. - * - * !WARNING!: Avoid using newNode directly. You should be using the - * macro makeNode. eg. to create a Resdom node, use makeNode(Resdom) - * + * Support for newNode() macro */ -Node *newNodeMacroHolder; +Node *newNodeMacroHolder; diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c index 9ce0c4ca72..48545b1512 100644 --- a/src/backend/utils/mmgr/mcxt.c +++ b/src/backend/utils/mmgr/mcxt.c @@ -14,7 +14,7 @@ * * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/utils/mmgr/mcxt.c,v 1.37 2002/12/15 21:01:34 tgl Exp $ + * $Header: /cvsroot/pgsql/src/backend/utils/mmgr/mcxt.c,v 1.38 2002/12/16 16:22:46 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -258,6 +258,35 @@ GetMemoryChunkSpace(void *pointer) } /* + * GetMemoryChunkContext + * Given a currently-allocated chunk, determine the context + * it belongs to. + */ +MemoryContext +GetMemoryChunkContext(void *pointer) +{ + StandardChunkHeader *header; + + /* + * Try to detect bogus pointers handed to us, poorly though we can. + * Presumably, a pointer that isn't MAXALIGNED isn't pointing at an + * allocated chunk. + */ + Assert(pointer != NULL); + Assert(pointer == (void *) MAXALIGN(pointer)); + + /* + * OK, it's probably safe to look at the chunk header. + */ + header = (StandardChunkHeader *) + ((char *) pointer - STANDARDCHUNKHEADERSIZE); + + AssertArg(MemoryContextIsValid(header->context)); + + return header->context; +} + +/* * MemoryContextStats * Print statistics about the named context and all its descendants. * @@ -453,25 +482,52 @@ MemoryContextAlloc(MemoryContext context, Size size) } /* - * MemoryContextAllocPalloc0 + * MemoryContextAllocZero * Like MemoryContextAlloc, but clears allocated memory * * We could just call MemoryContextAlloc then clear the memory, but this - * function is called too many times, so we have a separate version. + * is a very common combination, so we provide the combined operation. */ void * -MemoryContextAllocPalloc0(MemoryContext context, Size size) +MemoryContextAllocZero(MemoryContext context, Size size) { void *ret; AssertArg(MemoryContextIsValid(context)); if (!AllocSizeIsValid(size)) - elog(ERROR, "MemoryContextAllocZero: invalid request size %lu", + elog(ERROR, "MemoryContextAlloc: invalid request size %lu", (unsigned long) size); ret = (*context->methods->alloc) (context, size); + + MemSetAligned(ret, 0, size); + + return ret; +} + +/* + * MemoryContextAllocZeroAligned + * MemoryContextAllocZero where length is suitable for MemSetLoop + * + * This might seem overly specialized, but it's not because newNode() + * is so often called with compile-time-constant sizes. + */ +void * +MemoryContextAllocZeroAligned(MemoryContext context, Size size) +{ + void *ret; + + AssertArg(MemoryContextIsValid(context)); + + if (!AllocSizeIsValid(size)) + elog(ERROR, "MemoryContextAlloc: invalid request size %lu", + (unsigned long) size); + + ret = (*context->methods->alloc) (context, size); + MemSetLoop(ret, 0, size); + return ret; } diff --git a/src/include/c.h b/src/include/c.h index e5d7d64161..040a211a8f 100644 --- a/src/include/c.h +++ b/src/include/c.h @@ -12,7 +12,7 @@ * Portions Copyright (c) 1996-2002, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * $Id: c.h,v 1.133 2002/12/05 04:04:48 momjian Exp $ + * $Id: c.h,v 1.134 2002/12/16 16:22:46 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -579,38 +579,78 @@ typedef NameData *Name; * memset() functions. More research needs to be done, perhaps with * platform-specific MEMSET_LOOP_LIMIT values or tests in configure. * - * MemSet has been split into two parts so MemSetTest can be optimized - * away for constant 'val' and 'len'. This is used by palloc0(). - * - * Note, arguments are evaluated more than once. - * * bjm 2002-10-08 */ +#define MemSet(start, val, len) \ + do \ + { \ + int32 * _start = (int32 *) (start); \ + int _val = (val); \ + Size _len = (len); \ +\ + if ((((long) _start) & INT_ALIGN_MASK) == 0 && \ + (_len & INT_ALIGN_MASK) == 0 && \ + _val == 0 && \ + _len <= MEMSET_LOOP_LIMIT) \ + { \ + int32 * _stop = (int32 *) ((char *) _start + _len); \ + while (_start < _stop) \ + *_start++ = 0; \ + } \ + else \ + memset((char *) _start, _val, _len); \ + } while (0) + +#define MEMSET_LOOP_LIMIT 1024 + +/* + * MemSetAligned is the same as MemSet except it omits the test to see if + * "start" is word-aligned. This is okay to use if the caller knows a-priori + * that the pointer is suitably aligned (typically, because he just got it + * from palloc(), which always delivers a max-aligned pointer). + */ +#define MemSetAligned(start, val, len) \ + do \ + { \ + int32 * _start = (int32 *) (start); \ + int _val = (val); \ + Size _len = (len); \ +\ + if ((_len & INT_ALIGN_MASK) == 0 && \ + _val == 0 && \ + _len <= MEMSET_LOOP_LIMIT) \ + { \ + int32 * _stop = (int32 *) ((char *) _start + _len); \ + while (_start < _stop) \ + *_start++ = 0; \ + } \ + else \ + memset((char *) _start, _val, _len); \ + } while (0) + + +/* + * MemSetTest/MemSetLoop are a variant version that allow all the tests in + * MemSet to be done at compile time in cases where "val" and "len" are + * constants *and* we know the "start" pointer must be word-aligned. + * If MemSetTest succeeds, then it is okay to use MemSetLoop, otherwise use + * MemSetAligned. Beware of multiple evaluations of the arguments when using + * this approach. + */ #define MemSetTest(val, len) \ ( ((len) & INT_ALIGN_MASK) == 0 && \ (len) <= MEMSET_LOOP_LIMIT && \ (val) == 0 ) #define MemSetLoop(start, val, len) \ -do \ -{ \ - int32 * _start = (int32 *) (start); \ - int32 * _stop = (int32 *) ((char *) _start + (len)); \ -\ - while (_start < _stop) \ - *_start++ = 0; \ -} while (0) - -#define MemSet(start, val, len) \ -do \ -{ \ - if (MemSetTest(val, len) && ((long)(start) & INT_ALIGN_MASK) == 0 ) \ - MemSetLoop(start, val, len); \ - else \ - memset((char *)(start), (val), (len)); \ -} while (0) - -#define MEMSET_LOOP_LIMIT 1024 + do \ + { \ + int32 * _start = (int32 *) (start); \ + int32 * _stop = (int32 *) ((char *) _start + (Size) (len)); \ + \ + while (_start < _stop) \ + *_start++ = 0; \ + } while (0) /* ---------------------------------------------------------------- diff --git a/src/include/nodes/nodes.h b/src/include/nodes/nodes.h index 5f62816873..f119b5111d 100644 --- a/src/include/nodes/nodes.h +++ b/src/include/nodes/nodes.h @@ -7,7 +7,7 @@ * Portions Copyright (c) 1996-2002, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * $Id: nodes.h,v 1.133 2002/12/14 00:17:59 tgl Exp $ + * $Id: nodes.h,v 1.134 2002/12/16 16:22:46 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -293,9 +293,16 @@ typedef struct Node #define nodeTag(nodeptr) (((Node*)(nodeptr))->type) /* + * newNode - + * create a new node of the specified size and tag the node with the + * specified tag. + * + * !WARNING!: Avoid using newNode directly. You should be using the + * macro makeNode. eg. to create a Query node, use makeNode(Query) + * * There is no way to dereference the palloc'ed pointer to assign the - * tag, and return the pointer itself, so we need a holder variable. - * Fortunately, this function isn't recursive so we just define + * tag, and also return the pointer itself, so we need a holder variable. + * Fortunately, this macro isn't recursive so we just define * a global variable for this purpose. */ extern Node *newNodeMacroHolder; @@ -303,8 +310,7 @@ extern Node *newNodeMacroHolder; #define newNode(size, tag) \ ( \ AssertMacro((size) >= sizeof(Node)), /* need the tag, at least */ \ -\ - newNodeMacroHolder = (Node *) palloc0(size), \ + newNodeMacroHolder = (Node *) palloc0fast(size), \ newNodeMacroHolder->type = (tag), \ newNodeMacroHolder \ ) diff --git a/src/include/utils/memutils.h b/src/include/utils/memutils.h index 94578b8fb6..09198d1c6e 100644 --- a/src/include/utils/memutils.h +++ b/src/include/utils/memutils.h @@ -10,7 +10,7 @@ * Portions Copyright (c) 1996-2002, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * $Id: memutils.h,v 1.49 2002/12/15 21:01:34 tgl Exp $ + * $Id: memutils.h,v 1.50 2002/12/16 16:22:46 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -83,6 +83,7 @@ extern void MemoryContextResetChildren(MemoryContext context); extern void MemoryContextDeleteChildren(MemoryContext context); extern void MemoryContextResetAndDeleteChildren(MemoryContext context); extern Size GetMemoryChunkSpace(void *pointer); +extern MemoryContext GetMemoryChunkContext(void *pointer); extern void MemoryContextStats(MemoryContext context); #ifdef MEMORY_CONTEXT_CHECKING diff --git a/src/include/utils/palloc.h b/src/include/utils/palloc.h index 093764d5eb..7dabc52f31 100644 --- a/src/include/utils/palloc.h +++ b/src/include/utils/palloc.h @@ -21,7 +21,7 @@ * Portions Copyright (c) 1996-2002, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * $Id: palloc.h,v 1.23 2002/11/13 00:37:06 momjian Exp $ + * $Id: palloc.h,v 1.24 2002/12/16 16:22:46 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -46,15 +46,25 @@ extern DLLIMPORT MemoryContext CurrentMemoryContext; * Fundamental memory-allocation operations (more are in utils/memutils.h) */ extern void *MemoryContextAlloc(MemoryContext context, Size size); -extern void *MemoryContextAllocPalloc0(MemoryContext context, Size size); +extern void *MemoryContextAllocZero(MemoryContext context, Size size); +extern void *MemoryContextAllocZeroAligned(MemoryContext context, Size size); #define palloc(sz) MemoryContextAlloc(CurrentMemoryContext, (sz)) -/* We assume palloc() is already int-aligned */ -#define palloc0(sz) \ - ( MemSetTest(0, (sz)) ? \ - MemoryContextAllocPalloc0(CurrentMemoryContext, (sz)) : \ - memset(palloc(sz), 0, (sz))) +#define palloc0(sz) MemoryContextAllocZero(CurrentMemoryContext, (sz)) + +/* + * The result of palloc() is always word-aligned, so we can skip testing + * alignment of the pointer when deciding which MemSet variant to use. + * Note that this variant does not offer any advantage, and should not be + * used, unless its "sz" argument is a compile-time constant; therefore, the + * issue that it evaluates the argument multiple times isn't a problem in + * practice. + */ +#define palloc0fast(sz) \ + ( MemSetTest(0, sz) ? \ + MemoryContextAllocZeroAligned(CurrentMemoryContext, sz) : \ + MemoryContextAllocZero(CurrentMemoryContext, sz) ) extern void pfree(void *pointer); -- 2.11.0