Re: Make MemoryContextMemAllocated() more precise - Mailing list pgsql-hackers

Hi,

On 2020-03-27 17:21:10 -0700, Jeff Davis wrote:
> Attached refactoring patch. There's enough in here that warrants
> discussion that I don't think this makes sense for v13 and I'm adding
> it to the July commitfest.

IDK, adding a commit to v13 that we know we should do architecturally
differently in v14, when the difference in complexity between the two
patches isn't actually *that* big...

I'd like to see others jump in here...


> I still think we should do something for v13, such as the originally-
> proposed patch[1]. It's not critical, but it simply reports a better
> number for memory consumption. Currently, the memory usage appears to
> jump, often right past work mem (by a reasonable but noticable amount),
> which could be confusing.

Is that really a significant issue for most work mem sizes? Shouldn't
the way we increase sizes lead to the max difference between the
measurements to be somewhat limited?


>   * there's a new MemoryContextCount() that simply calculates the
>     statistics without printing anything, and returns a struct
>     - it supports flags to indicate which stats should be
>       calculated, so that some callers can avoid walking through
>       blocks/freelists
>   * it adds a new statistic for "new space" (i.e. untouched)
>   * it eliminates specialization of the memory context printing
>     - the only specialization was for generation.c to output the
>       number of chunks, which can be done easily enough for the
>       other types, too

That sounds like a good direction.



> +    if (flags & MCXT_STAT_NBLOCKS)
> +        counters.nblocks = nblocks;
> +    if (flags & MCXT_STAT_NCHUNKS)
> +        counters.nchunks = set->nChunks;
> +    if (flags & MCXT_STAT_FREECHUNKS)
> +        counters.freechunks = freechunks;
> +    if (flags & MCXT_STAT_TOTALSPACE)
> +        counters.totalspace = set->memAllocated;
> +    if (flags & MCXT_STAT_FREESPACE)
> +        counters.freespace = freespace;
> +    if (flags & MCXT_STAT_NEWSPACE)
> +        counters.newspace = set->blocks->endptr - set->blocks->freeptr;

I'd spec it so that context implementations are allowed to
unconditionally fill fields, even when the flag isn't specified. The
branches quoted don't buy us anyting...



> diff --git a/src/include/nodes/memnodes.h b/src/include/nodes/memnodes.h
> index c9f2bbcb367..cc545852968 100644
> --- a/src/include/nodes/memnodes.h
> +++ b/src/include/nodes/memnodes.h
> @@ -29,11 +29,21 @@
>  typedef struct MemoryContextCounters
>  {
>      Size        nblocks;        /* Total number of malloc blocks */
> +    Size        nchunks;        /* Total number of chunks (used+free) */
>      Size        freechunks;        /* Total number of free chunks */
>      Size        totalspace;        /* Total bytes requested from malloc */
>      Size        freespace;        /* The unused portion of totalspace */
> +    Size        newspace;        /* Allocated but never held any chunks */

I'd add some reasoning as to why this is useful.


>  } MemoryContextCounters;
>  
> +#define MCXT_STAT_NBLOCKS        (1 << 0)
> +#define MCXT_STAT_NCHUNKS        (1 << 1)
> +#define MCXT_STAT_FREECHUNKS    (1 << 2)
> +#define MCXT_STAT_TOTALSPACE    (1 << 3)
> +#define MCXT_STAT_FREESPACE        (1 << 4)
> +#define MCXT_STAT_NEWSPACE        (1 << 5)

s/MCXT_STAT/MCXT_STAT_NEED/?


> +#define MCXT_STAT_ALL            ((1 << 6) - 1)

Hm, why not go for ~0 or such?


Greetings,

Andres Freund