Re: inline newNode() - Mailing list pgsql-patches

Neil Conway wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > OK, here's a patch for testing.  It needs cleanup because the final
> > version would remove the nodes/nodes.c file.  The net effect of the
> > patch is to make newNode a macro with little code bloat.
>
> Ok, I think this is the better version. The performance seems to be
> about the same as my original patch, and the code bloat is a lot less:
> 12857787 with the patch versus 12845168 without it. The
> implemementation (with a global variable) is pretty ugly, but I don't
> really see a way around that...
>
> One minor quibble with the patch though: MemoryContextAllocC() and
> pallocC() aren't very good function names, IMHO. Perhaps
> MemoryContextAllocZero() and palloc0() would be better?

Sure, I like those names.

> This isn't specific to your patch, but it occurs to me that we could
> save a few bits of code bloat if we removed the '_len' variable
> declaration from the MemSet() macro -- it isn't needed, AFAICT. That
> would mean we we'd evaluate the 'len' argument multiple times, so I'm
> not sure if that's a win overall...

I think that was actually the goal, to not have them evaluated multiple
times, and perhaps bloat if the length itself was a macro.

New patch attached with your suggested renaming.  Now that I look at the
code, there are about 55 places where we call palloc then right after it
MemSet(0), so we could merge those to use palloc0 and reduce existing
MemSet code bloat some more.

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073
Index: src/backend/nodes/nodes.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/backend/nodes/nodes.c,v
retrieving revision 1.15
diff -c -c -r1.15 nodes.c
*** src/backend/nodes/nodes.c    20 Jun 2002 20:29:29 -0000    1.15
--- src/backend/nodes/nodes.c    9 Oct 2002 05:17:13 -0000
***************
*** 28,42 ****
   *      macro makeNode. eg. to create a Resdom node, use makeNode(Resdom)
   *
   */
! Node *
! newNode(Size size, NodeTag tag)
! {
!     Node       *newNode;

-     Assert(size >= sizeof(Node));        /* need the tag, at least */
-
-     newNode = (Node *) palloc(size);
-     MemSet((char *) newNode, 0, size);
-     newNode->type = tag;
-     return newNode;
- }
--- 28,32 ----
   *      macro makeNode. eg. to create a Resdom node, use makeNode(Resdom)
   *
   */
! Node *newNodeMacroHolder;

Index: src/backend/utils/mmgr/mcxt.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/backend/utils/mmgr/mcxt.c,v
retrieving revision 1.32
diff -c -c -r1.32 mcxt.c
*** src/backend/utils/mmgr/mcxt.c    12 Aug 2002 00:36:12 -0000    1.32
--- src/backend/utils/mmgr/mcxt.c    9 Oct 2002 05:17:17 -0000
***************
*** 453,458 ****
--- 453,481 ----
  }

  /*
+  * 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.
+  */
+ void *
+ MemoryContextAllocZero(MemoryContext context, Size size)
+ {
+     void *ret;
+
+     AssertArg(MemoryContextIsValid(context));
+
+     if (!AllocSizeIsValid(size))
+         elog(ERROR, "MemoryContextAllocZero: invalid request size %lu",
+              (unsigned long) size);
+
+     ret = (*context->methods->alloc) (context, size);
+     MemSet(ret, 0, size);
+     return ret;
+ }
+
+ /*
   * pfree
   *        Release an allocated chunk.
   */
Index: src/include/nodes/nodes.h
===================================================================
RCS file: /cvsroot/pgsql-server/src/include/nodes/nodes.h,v
retrieving revision 1.118
diff -c -c -r1.118 nodes.h
*** src/include/nodes/nodes.h    31 Aug 2002 22:10:47 -0000    1.118
--- src/include/nodes/nodes.h    9 Oct 2002 05:17:20 -0000
***************
*** 261,266 ****
--- 261,284 ----

  #define nodeTag(nodeptr)        (((Node*)(nodeptr))->type)

+ /*
+  *    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
+  *    a global variable for this purpose.
+  */
+ extern Node *newNodeMacroHolder;
+
+ #define newNode(size, tag) \
+ ( \
+     AssertMacro((size) >= sizeof(Node)),        /* need the tag, at least */ \
+ \
+     newNodeMacroHolder = (Node *) palloc0(size), \
+     newNodeMacroHolder->type = (tag), \
+     newNodeMacroHolder \
+ )
+
+
  #define makeNode(_type_)        ((_type_ *) newNode(sizeof(_type_),T_##_type_))
  #define NodeSetTag(nodeptr,t)    (((Node*)(nodeptr))->type = (t))

***************
*** 281,291 ****
   *                      extern declarations follow
   * ----------------------------------------------------------------
   */
-
- /*
-  * nodes/nodes.c
-  */
- extern Node *newNode(Size size, NodeTag tag);

  /*
   * nodes/{outfuncs.c,print.c}
--- 299,304 ----
Index: src/include/utils/palloc.h
===================================================================
RCS file: /cvsroot/pgsql-server/src/include/utils/palloc.h,v
retrieving revision 1.19
diff -c -c -r1.19 palloc.h
*** src/include/utils/palloc.h    20 Jun 2002 20:29:53 -0000    1.19
--- src/include/utils/palloc.h    9 Oct 2002 05:17:21 -0000
***************
*** 46,53 ****
--- 46,56 ----
   * Fundamental memory-allocation operations (more are in utils/memutils.h)
   */
  extern void *MemoryContextAlloc(MemoryContext context, Size size);
+ extern void *MemoryContextAllocZero(MemoryContext context, Size size);

  #define palloc(sz)    MemoryContextAlloc(CurrentMemoryContext, (sz))
+
+ #define palloc0(sz)    MemoryContextAllocZero(CurrentMemoryContext, (sz))

  extern void pfree(void *pointer);