Re: Memory Accounting v11

On Tue, 2015-07-07 at 16:49 +1200, David Rowley wrote:

> of performance decrease anywhere. I'm just getting too much variation
> in the test results to get any sort of idea.
>
That was my experience as well. Thank you for taking a look.

> My main question here is: How sure are you that none of your intended
> use cases will need to call MemoryContextMemAllocated with recurse =
> true? I'd imagine if you ever have to
> use MemoryContextMemAllocated(ctx, true) then we'll all be sitting
> around with our stopwatches out to see if the call incurs too much of
> a performance penalty.

I don't expect HashAgg to be using many memory contexts. It did with
array_agg, but that's been changed, and was a bad pattern anyway (one
context per group is quite wasteful).

If it turns out to be slow, we can call it less frequently (e.g.
extrapolate growth rate to determine when to check).

>
> Shouldn't you be setting oldblksize after the "if"? I'd imagine the
> realloc() failure is rare, but it just seems better to do it just
> before it's required and only when required.

I don't think that's safe. Realloc can return an entirely new pointer,
so the endptr would be pointing outside of the allocation. I'd have to
hang on to the original pointer before the realloc, so I'd need an extra
variable anyway.

>
> Here there's a double assignment of "child".

Will fix.

>
> I might be mistaken here, but can't you just set context->mem_allocted
> = 0; after that loop?
> Or maybe it would be an improvement to only do the decrement
> if MEMORY_CONTEXT_CHECKING is defined, then Assert() that
> mem_allocated == 0 after the loop...
>
OK. Why not just always Assert that?
>
> One other thing that I don't remember seeing discussed would be to
> just store a List in each context which would store all of the
> mem_allocated's that need to be updated during each allocation and
> deallocation of memory. The list would be setup once when the context
> is created. When a child context is setup we'd just loop up the parent
> context chain and list_concat() all of the parent's lists onto the
> child's lists. Would this method add too much overhead when a context
> is deleted? Has this been explored already?
>
That's a good idea, as it would be faster than recursing. Also, the
number of parents can't change, so we can just make an array, and that
would be quite fast to update. Unless I'm missing something, this sounds
like a nice solution. It would require more space in MemoryContextData,
but that might not be a problem.

Regards,
Jeff Davis