Thread: Oversight in slab.c SlabContextCreate(), initial memory allocation size is not populated to context->mem_allocated
Oversight in slab.c SlabContextCreate(), initial memory allocation size is not populated to context->mem_allocated
From
Reid Thompson
Date:
Hi,
Both aset.c and generation.c populate mem_allocated in
AllocSetContextCreateInternal(), GenerationContextCreate()
respectively.
aset.c
/* Finally, do the type-independent part of context creation */
MemoryContextCreate((MemoryContext) set,
T_AllocSetContext,
&AllocSetMethods,
parent,
name);
((MemoryContext) set)->mem_allocated = firstBlockSize;
return (MemoryContext) set;
}
generation.c
/* Finally, do the type-independent part of context creation */
MemoryContextCreate((MemoryContext) set,
T_GenerationContext,
&GenerationMethods,
parent,
name);
((MemoryContext) set)->mem_allocated = firstBlockSize;
return (MemoryContext) set;
}
slab.c
does not in SlabContextCreate(). Is this intentional, it seems to be an
oversight to me.
/* Finally, do the type-independent part of context creation */
MemoryContextCreate((MemoryContext) slab,
T_SlabContext,
&SlabMethods,
parent,
name);
return (MemoryContext) slab;
}
--
Reid Thompson
Senior Software Engineer
Crunchy Data, Inc.
reid.thompson@crunchydata.com
www.crunchydata.com
Re: Oversight in slab.c SlabContextCreate(), initial memory allocation size is not populated to context->mem_allocated
From
Nathan Bossart
Date:
On Fri, Jul 29, 2022 at 12:43:45PM -0400, Reid Thompson wrote:
> slab.c
> does not in SlabContextCreate(). Is this intentional, it seems to be an
> oversight to me.
>
> /* Finally, do the type-independent part of context creation */
> MemoryContextCreate((MemoryContext) slab,
> T_SlabContext,
> &SlabMethods,
> parent,
> name);
>
> return (MemoryContext) slab;
> }
IIUC this is because the header is tracked separately from the first
regular block, unlike aset.c. See the following comment:
/*
* Allocate the context header. Unlike aset.c, we never try to combine
* this with the first regular block; not worth the extra complication.
*/
You'll also notice that the "reset" and "free" functions in aset.c and
generation.c have special logic for "keeper" blocks. Here is a relevant
comment from AllocSetReset():
* Actually, this routine has some discretion about what to do.
* It should mark all allocated chunks freed, but it need not necessarily
* give back all the resources the set owns. Our actual implementation is
* that we give back all but the "keeper" block (which we must keep, since
* it shares a malloc chunk with the context header). In this way, we don't
* thrash malloc() when a context is repeatedly reset after small allocations,
* which is typical behavior for per-tuple contexts.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Re: Oversight in slab.c SlabContextCreate(), initial memory allocation size is not populated to context->mem_allocated
From
Tom Lane
Date:
Nathan Bossart <nathandbossart@gmail.com> writes:
> On Fri, Jul 29, 2022 at 12:43:45PM -0400, Reid Thompson wrote:
>> slab.c
>> does not in SlabContextCreate(). Is this intentional, it seems to be an
>> oversight to me.
> IIUC this is because the header is tracked separately from the first
> regular block, unlike aset.c.
That doesn't make it not an oversight, though. It looks like aset.c
thinks that mem_allocated includes all the context's overhead, whereas
this implementation doesn't seem to have that result. The comments
associated with mem_allocated are sufficiently vague that it's impossible
to tell which implementation is correct. Maybe we don't really care,
but ...
regards, tom lane
Re: Oversight in slab.c SlabContextCreate(), initial memory allocation size is not populated to context->mem_allocated
From
Nathan Bossart
Date:
On Fri, Jul 29, 2022 at 01:55:10PM -0400, Tom Lane wrote: > Nathan Bossart <nathandbossart@gmail.com> writes: >> On Fri, Jul 29, 2022 at 12:43:45PM -0400, Reid Thompson wrote: >>> slab.c >>> does not in SlabContextCreate(). Is this intentional, it seems to be an >>> oversight to me. > >> IIUC this is because the header is tracked separately from the first >> regular block, unlike aset.c. > > That doesn't make it not an oversight, though. It looks like aset.c > thinks that mem_allocated includes all the context's overhead, whereas > this implementation doesn't seem to have that result. The comments > associated with mem_allocated are sufficiently vague that it's impossible > to tell which implementation is correct. Maybe we don't really care, > but ... Hm. mmgr/README indicates the following note about mem_allocated: * inquire about the total amount of memory allocated to the context (the raw memory from which the context allocates chunks; not the chunks themselves) AFAICT MemoryContextMemAllocated() is only used for determining when to spill to disk for hash aggegations at the moment. I don't know whether I'd classify this as an oversight or if it even makes any meaningful difference, but consistency among the different implementations is probably desirable either way. So, I guess I'm +1 for including the memory context header in mem_allocated in this case. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: Oversight in slab.c SlabContextCreate(), initial memory allocation size is not populated to context->mem_allocated
From
Tomas Vondra
Date:
On 7/29/22 20:23, Nathan Bossart wrote: > On Fri, Jul 29, 2022 at 01:55:10PM -0400, Tom Lane wrote: >> Nathan Bossart <nathandbossart@gmail.com> writes: >>> On Fri, Jul 29, 2022 at 12:43:45PM -0400, Reid Thompson wrote: >>>> slab.c >>>> does not in SlabContextCreate(). Is this intentional, it seems to be an >>>> oversight to me. >> >>> IIUC this is because the header is tracked separately from the first >>> regular block, unlike aset.c. >> >> That doesn't make it not an oversight, though. It looks like aset.c >> thinks that mem_allocated includes all the context's overhead, whereas >> this implementation doesn't seem to have that result. The comments >> associated with mem_allocated are sufficiently vague that it's impossible >> to tell which implementation is correct. Maybe we don't really care, >> but ... > > Hm. mmgr/README indicates the following note about mem_allocated: > > * inquire about the total amount of memory allocated to the context > (the raw memory from which the context allocates chunks; not the > chunks themselves) > > AFAICT MemoryContextMemAllocated() is only used for determining when to > spill to disk for hash aggegations at the moment. I don't know whether I'd > classify this as an oversight or if it even makes any meaningful > difference, but consistency among the different implementations is probably > desirable either way. So, I guess I'm +1 for including the memory context > header in mem_allocated in this case. > I don't think this can make meaningful difference - as you mention, we only really use this to decide when to spill to disk etc. So maybe you'll spill a bit sooner, but the work_mem is pretty crude threshold anyway, people don't tune it to an exact byte value (which would be pretty futile anyway). OTOH it does seem like an oversight, or at least an inconsistency with the two other contexts, so if anyone feels like tweaking it ... regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Oversight in slab.c SlabContextCreate(), initial memory allocation size is not populated to context->mem_allocated
From
Kyotaro Horiguchi
Date:
At Fri, 29 Jul 2022 21:16:51 +0200, Tomas Vondra <tomas.vondra@enterprisedb.com> wrote in > I don't think this can make meaningful difference - as you mention, we > only really use this to decide when to spill to disk etc. So maybe > you'll spill a bit sooner, but the work_mem is pretty crude threshold > anyway, people don't tune it to an exact byte value (which would be > pretty futile anyway). From another perspective.. SlabStats includes the header size into its total size. So it reports a different total size from MemoryContextMemAllocated() (For example, 594 bytes vs 0). Since this is an inconsistency within slab.c, no users will notice that difference in the field. > OTOH it does seem like an oversight, or at least an inconsistency with > the two other contexts, so if anyone feels like tweaking it ... regards. -- Kyotaro Horiguchi NTT Open Source Software Center