Re: Change pfree to accept NULL argument

From: David Rowley <dgrowleyml(at)gmail(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Change pfree to accept NULL argument
Date: 2022-08-23 01:17:10
Message-ID: CAApHDvonA=O1WhwBhiqYcFZ3nZqi7wSmpmxLewXpCOBgV92+Rg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Tue, 23 Aug 2022 at 06:30, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> writes:
> > Per discussion in [0], here is a patch set that allows pfree() to accept
> > a NULL argument, like free() does.
>
> So the question is, is this actually a good thing to do?

I think making pfree() accept NULL is a bad idea. The vast majority
of cases the pointer will never be NULL, so we're effectively just
burdening those with the additional overhead of checking for NULL.

We know from [1] that adding branching in the memory management code
can be costly.

I'm measuring about a 2.6% slowdown from the 0002 patch using a
function that I wrote [2] to hammer palloc/pfree.

master
postgres=# select pg_allocate_memory_test(64, 1024*1024,
10::bigint*1024*1024*1024,'aset');
Time: 2007.527 ms (00:02.008)
Time: 1991.574 ms (00:01.992)
Time: 2008.945 ms (00:02.009)
Time: 2011.410 ms (00:02.011)
Time: 2019.317 ms (00:02.019)
Time: 2060.832 ms (00:02.061)
Time: 2003.066 ms (00:02.003)
Time: 2025.039 ms (00:02.025)
Time: 2039.744 ms (00:02.040)
Time: 2090.384 ms (00:02.090)

master + pfree modifed to check for NULLs
postgres=# select pg_allocate_memory_test(64, 1024*1024,
10::bigint*1024*1024*1024,'aset');
Time: 2057.625 ms (00:02.058)
Time: 2074.699 ms (00:02.075)
Time: 2075.629 ms (00:02.076)
Time: 2104.581 ms (00:02.105)
Time: 2072.620 ms (00:02.073)
Time: 2066.916 ms (00:02.067)
Time: 2071.962 ms (00:02.072)
Time: 2097.520 ms (00:02.098)
Time: 2087.421 ms (00:02.087)
Time: 2078.695 ms (00:02.079)

(~2.62% slowdown)

If the aim here is to remove a bunch of ugly if (ptr) pfree(ptr);
code, then why don't we just have a[n inline] function or a macro for
that and only use it when we need to?

David

[1] https://www.postgresql.org/message-id/CAApHDvr6qFw3jLBL9d4zUpo3A2Cb6hoZsUnWD0vF1OGsd67v=w@mail.gmail.com
[2] https://www.postgresql.org/message-id/attachment/136801/pg_allocate_memory_test.patch.txt

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Kyotaro Horiguchi 2022-08-23 01:29:21 Letter case of "admin option"
Previous Message Justin Pryzby 2022-08-23 01:16:59 Re: shadow variables - pg15 edition