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