Re: MemSet inline for newNode

From: Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us>
To: PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Subject: Re: MemSet inline for newNode
Date: 2002-11-11 16:06:40
Message-ID: 200211111606.gABG6eS29372@candle.pha.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers pgsql-patches


OK, here is a newer version of the macro. I tested it on
catalog/index.c and saw the optimizer remove the MemSetTest conditional
for the newNode/palloc0 call in that file:

pushl $1 <---------
pushl $108
movl CurrentMemoryContext,%eax
pushl %eax
call MemoryContextAllocZero

I still test the palloc pointer alignment in MemSetLoop because that is
not a constant.

I just ran a test and now see a 7% difference for the following test:

j = 32;
for (i = 0; i < 1000000; i++)
MemSet(buf, 0, j /* choose 32 or j here */);
return 0;

It used to be a 14% difference. However, this looks more like an
optimizer problem than actual coding and with the new macros, MemSet
will be called with a constant third parameter anyway in almost all
cases, including newNode and the merge of palloc/MemSet into palloc0
patch.

---------------------------------------------------------------------------

Bruce Momjian wrote:
> Tom Lane wrote:
> > Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > >> But your patch as committed does NOT inline newNode, in fact it does the
> > >> exact opposite: the MemSet macro is removed from the callsite.
> >
> > > Yes, there were actually two patches; the first inlined newNode by
> > > calling palloc0. Without palloc, there was no way to inline newNode.
> >
> > But you have *not* inlined newNode: the memset is still done at a location
> > that cannot know the size at compile time.
>
> I am sorry. I inlined newNode, but by pushing MemSet into palloc0, the
> MemSet conditional tests can not be optimized away. Is that clearer?
>
> #define newNode(size, tag) \
> ( \
> AssertMacro((size) >= sizeof(Node)), /* need the tag, at least */ \
> \
> newNodeMacroHolder = (Node *) palloc0(size), \
> newNodeMacroHolder->type = (tag), \
> newNodeMacroHolder \
> )
>
> In this case, I needed palloc0 to make newNode inlined, and the old
> function version of newNode couldn't optimize away the conditional test
> either because it was called with various sizes.
>
>
> > > The second was a more general one to merge palloc/MemSet into a single
> > > palloc0 call. That has been backed out while I research it.
> >
> > That part could be sold just on the basis of making the code easier
> > to read and less error-prone; particularly for call sites where the
> > length is a runtime computation anyway. I think that newNode() is the
>
> Yes, that was my thought.
>
> > principal case where the length is knowable at compile time. So my
> > feeling is that the general change to invent a palloc0() call is fine
> > ... but if you want performance then newNode() should *not* be using the
> > generic palloc0() call.
>
> OK. I am still struggling about how to do that.
>
> > > My new idea is to add a third boolean argument to
> > > MemoryContextAllocZero() which will control whether the MemSet
> > > assignment loop is used, or memset().
> >
> > But we are *trying to eliminate any runtime test whatever*. Short
> > of that, you aren't going to get the speedup. Certainly passing a third
> > argument to the alloc subroutine will eat enough cycles to negate any
> > savings from simplifying the runtime test slightly.
>
> My idea is that the conditional tests will be optimized away before the
> palloc0 call is made.
>
> Attached is a patch that implements the MemSet split and allows the size
> tests to be optimized away _before_ at the call location. It isn't done
> yet, but you can get the idea.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Attachment Content-Type Size
unknown_filename text/plain 4.3 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2002-11-11 16:23:44 Re: MemSet inline for newNode
Previous Message Dennis Björklund 2002-11-11 14:16:57 Re: Implicit coercions, choosing types for constants, etc

Browse pgsql-patches by date

  From Date Subject
Next Message Tom Lane 2002-11-11 16:23:44 Re: MemSet inline for newNode
Previous Message Tom Lane 2002-11-11 14:15:45 Re: ON COMMIT temp table handling