Skip site navigation (1) Skip section navigation (2)

Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)

From: Andres Freund <andres(at)2ndquadrant(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Heikki Linnakangas <hlinnakangas(at)vmware(dot)com>,pgsql-hackers(at)postgresql(dot)org
Subject: Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
Date: 2013-01-09 23:05:07
Message-ID: 20130109230507.GA17428@awork2.anarazel.de (view raw or flat)
Thread:
Lists: pgsql-hackers
On 2013-01-09 17:28:35 -0500, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > On 2013-01-09 15:43:19 -0500, Tom Lane wrote:
> >> I had thought of proposing that we code
> >> palloc() like this:
> >>
> >> void *
> >> palloc(Size size)
> >> {
> >>     MemoryContext context = CurrentMemoryContext;
> >>
> >>     AssertArg(MemoryContextIsValid(context));
> >>
> >>     if (!AllocSizeIsValid(size))
> >>         elog(ERROR, "invalid memory alloc request size %lu",
> >>              (unsigned long) size);
> >>
> >>     context->isReset = false;
> >>
> >>     return (*context->methods->alloc) (context, size);
> >> }
> >>
> >> but at least on this specific hardware and compiler that would evidently
> >> be a net loss compared to direct use of CurrentMemoryContext.  I would
> >> not put a lot of faith in that result holding up on other machines
> >> though.
>
> > Thats not optimized to the same? ISTM the compiler should produce
> > exactly the same code for both.
>
> No, that's exactly the point here, you can't just assume that you get
> the same code; this is tense enough that a few instructions matter.
> Remember we're considering non-assert builds, so the AssertArg vanishes.
> So in the form where CurrentMemoryContext is only read after the elog
> call, the compiler can see that it requires but one fetch - it can't
> change within the two lines where it's used.  In the form given above,
> the compiler is required to fetch CurrentMemoryContext into a local
> variable and keep it across the elog call.

I had thought that it could simply store the address in a callee
saved register inside palloc, totally forgetting that palloc would need
to save that register itself in that case.

> (We know this doesn't
> matter, but gcc doesn't; this version of gcc apparently isn't doing much
> with the knowledge that elog won't return.)

Afaics one reason for that is that we don't have any such annotation for
elog(), just for ereport. And I don't immediately see how it could be
added to elog without relying on variadic macros. Bit of a shame, there
probably a good number of callsites that could actually benefit from
that knowledge.
Is it worth making that annotation conditional on variadic macro
support?

Greetings,

Andres Freund

--
 Andres Freund	                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


In response to

Responses

pgsql-hackers by date

Next:From: Bruce MomjianDate: 2013-01-09 23:07:31
Subject: 9.2.1 & index-only scans : abnormal heap fetches after VACUUM FULL
Previous:From: Simon RiggsDate: 2013-01-09 22:47:15
Subject: Re: Index build temp files

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group