Re: Make MemoryContextMemAllocated() more precise

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