Thread: pgsql: Pull up isReset flag from AllocSetContext to MemoryContext struc
pgsql: Pull up isReset flag from AllocSetContext to MemoryContext struc
From
Heikki Linnakangas
Date:
Pull up isReset flag from AllocSetContext to MemoryContext struct. This avoids the overhead of one function call when calling MemoryContextReset(), and it seems like the isReset optimization would be applicable to any new memory context we might invent in the future anyway. This buys back the overhead I just added in previous patch to always call MemoryContextReset() in ExecScan, even when there's no quals or projections. Branch ------ master Details ------- http://git.postgresql.org/pg/commitdiff/30e98a7e6e4869a7d6b3748ac9770bb8d69a8b26 Modified Files -------------- src/backend/utils/mmgr/aset.c | 25 ++----------------------- src/backend/utils/mmgr/mcxt.c | 18 ++++++++++++++++-- src/include/nodes/memnodes.h | 1 + 3 files changed, 19 insertions(+), 25 deletions(-)
Heikki Linnakangas <heikki.linnakangas@iki.fi> writes:
> Pull up isReset flag from AllocSetContext to MemoryContext struct. This
> avoids the overhead of one function call when calling MemoryContextReset(),
> and it seems like the isReset optimization would be applicable to any new
> memory context we might invent in the future anyway.
> This buys back the overhead I just added in previous patch to always call
> MemoryContextReset() in ExecScan, even when there's no quals or projections.
Do you actually have any measurements that prove that? This seems like
a rather ugly destruction of a modularity boundary in return for a
hypothetical performance gain. I'm also concerned that you've probably
added cycles on net to MemoryContextAlloc (where it's no longer possible
to tail-call AllocSetAlloc), which could very easily cost more cycles on
most workloads than could ever be saved by making MemoryContextReset a
shade faster.
regards, tom lane
Re: pgsql: Pull up isReset flag from AllocSetContext to MemoryContext struc
From
Heikki Linnakangas
Date:
On 22.05.2011 21:18, Tom Lane wrote: > Heikki Linnakangas<heikki.linnakangas@iki.fi> writes: >> Pull up isReset flag from AllocSetContext to MemoryContext struct. This >> avoids the overhead of one function call when calling MemoryContextReset(), >> and it seems like the isReset optimization would be applicable to any new >> memory context we might invent in the future anyway. > >> This buys back the overhead I just added in previous patch to always call >> MemoryContextReset() in ExecScan, even when there's no quals or projections. > > Do you actually have any measurements that prove that? Yes, I repeated the test I posted earlier in this thread with this patch: > CREATE TABLE foo AS SELECT generate_series(1,10000000); > > I ran "SELECT COUNT(*) FROM foo" many times with \timing on, and took the smallest time with and without the patch. I got: > > 1230 ms with the patch > 1186 ms without the patch "with the patch" above means with the patch to just add the MemoryContextReset call, not the pulling up of isReset flag. After the pull-up-isReset-flag patch I got the time down to 1174 ms, so it does buy back the slowdown of adding the MemoryContextReset call. > This seems like > a rather ugly destruction of a modularity boundary in return for a > hypothetical performance gain. It doesn't seem bad from a modularity point of view to me. The optimization to not do anything if nothing has been allocated since last reset seems valid across all conceivable memory context implementations. > I'm also concerned that you've probably > added cycles on net to MemoryContextAlloc (where it's no longer possible > to tail-call AllocSetAlloc), which could very easily cost more cycles on > most workloads than could ever be saved by making MemoryContextReset a > shade faster. Good point. That would be solved by clearing the flag before the AllocSetAlloc() call. I don't see any harm in clearing the flag before actually doing the allocation. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> On 22.05.2011 21:18, Tom Lane wrote:
>> I'm also concerned that you've probably
>> added cycles on net to MemoryContextAlloc (where it's no longer possible
>> to tail-call AllocSetAlloc), which could very easily cost more cycles on
>> most workloads than could ever be saved by making MemoryContextReset a
>> shade faster.
> Good point. That would be solved by clearing the flag before the
> AllocSetAlloc() call. I don't see any harm in clearing the flag before
> actually doing the allocation.
Done that way.
regards, tom lane
Re: pgsql: Pull up isReset flag from AllocSetContext to MemoryContext struc
From
Heikki Linnakangas
Date:
On 25.05.2011 00:58, Tom Lane wrote: > Heikki Linnakangas<heikki.linnakangas@enterprisedb.com> writes: >> On 22.05.2011 21:18, Tom Lane wrote: >>> I'm also concerned that you've probably >>> added cycles on net to MemoryContextAlloc (where it's no longer possible >>> to tail-call AllocSetAlloc), which could very easily cost more cycles on >>> most workloads than could ever be saved by making MemoryContextReset a >>> shade faster. > >> Good point. That would be solved by clearing the flag before the >> AllocSetAlloc() call. I don't see any harm in clearing the flag before >> actually doing the allocation. > > Done that way. Ok, thanks. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com