On Mon, 20 Mar 2023 at 16:10, Richard Guo <guofenglinux@gmail.com> wrote:
> mstate->mem_used -= EMPTY_ENTRY_MEMORY_BYTES(entry);
> + mstate->mem_used -= sizeof(MemoizeKey) + GetMemoryChunkSpace(key->params);
>
> It seems that the memory used by key is already accounted for in
> EMPTY_ENTRY_MEMORY_BYTES. I wonder if this change is needed.
Yeah, I noticed this earlier when looking at the patch again. I
remembered I'd not taking the cache key memory into account, seems I
should have remembered that I only did that for the planner only. I
just pushed the patch to fix that part.
> Also I'm kinda confused about using MinimalTuple->t_len vs. using
> GetMemoryChunkSpace(MinimalTuple). Why do we choose t_len rather than
> GetMemoryChunkSpace in EMPTY_ENTRY_MEMORY_BYTES and CACHE_TUPLE_BYTES?
It's true that these can be wildly different, but at least slightly
less so than it was in v15 due to c6e0fe1f2a. I understand
GetMemoryChunkSpace is what we generally use in nodeAgg.c. So
probably it would be more accurate to use that. However, it's not
like making that change would make it perfect. Once we start evicting
cache entries and adding new ones, aset.c may malloc() new blocks when
the items we've pfree'd are on a free list that are not useful for new
allocations.
It would be interesting to see if there's any performance hit from
using GetMemoryChunkSpace(). I expect that's slower. It's just a
question of if we can measure it.
David