On Tue, 5 Mar 2024 at 15:42, David Rowley <dgrowle...@gmail.com> wrote:
> The query I ran was:
>
> select chksz,mtype,pg_allocate_memory_test_reset(chksz,
> 1024*1024,1024*1024*1024, mtype) from (values(8),(16),(32),(64))
> sizes(chksz),(values('aset'),('generation'),('slab'),('bump'))
> cxt(mtype) order by mtype,chksz;
Andres and I were discussing this patch offlist in the context of
"should we have bump". Andres wonders if it would be better to have a
function such as palloc_nofree() (we didn't actually discuss the
name), which for aset, would forego rounding up to the next power of 2
and not bother checking the freelist and only have a chunk header for
MEMORY_CONTEXT_CHECKING builds. For non-MEMORY_CONTEXT_CHECKING
builds, the chunk header could be set to some other context type such
as one of the unused ones or perhaps a dedicated new one that does
something along the lines of BogusFree() which raises an ERROR if
anything tries to pfree or repalloc it.
An advantage of having this instead of bump would be that it could be
used for things such as the parser, where we make a possibly large
series of small allocations and never free them again.
Andres ask me to run some benchmarks to mock up AllocSetAlloc() to
have it not check the freelist to see how the performance of it
compares to BumpAlloc(). I did this in the attached 2 patches. The
0001 patch just #ifdefs that part of AllocSetAlloc out, however
properly implementing this is more complex as aset.c currently stores
the freelist index in the MemoryChunk rather than the chunk_size. I
did this because it saved having to call AllocSetFreeIndex() in
AllocSetFree() which made a meaningful performance improvement in
pfree(). The 0002 patch effectively reverses that change out so that
the chunk_size is stored. Again, these patches are only intended to
demonstrate the performance and check how it compares to bump.
I'm yet uncertain why, but I find that the first time I run the query
quoted above, the aset results are quite a bit slower than on
subsequent runs. Other context types don't seem to suffer from this.
The previous results I sent in [1] were of the initial run after
starting up the database.
The attached graph shows the number of seconds it takes to allocate a
total of 1GBs of memory in various chunk sizes, resetting the context
after 1MBs has been allocated, so as to keep the test sized so it fits
in CPU caches.
I'm not drawing any particular conclusion from the results aside from
it's not quite as fast as bump. I also have some reservations about
how easy it would be to actually use something like palloc_nofree().
For example heap_form_minimal_tuple() does palloc0(). What if I
wanted to call ExecCopySlotMinimalTuple() and use palloc0_nofree().
Would we need new versions of various functions to give us control
over this?
David
[1]
https://www.postgresql.org/message-id/CAApHDvr_hGT=kap0yxbksnztbrx+6hukiecwen2bulww1ut...@mail.gmail.com
From 3980d35a11d7f27d18c57586a7f7539d86dafae3 Mon Sep 17 00:00:00 2001
From: David Rowley <dgrow...@gmail.com>
Date: Tue, 26 Mar 2024 00:06:55 +1300
Subject: [PATCH 2/2] Modify aset.c to store the chunk size in the hdr
---
src/backend/utils/mmgr/aset.c | 51 +++++++++++++----------------------
1 file changed, 19 insertions(+), 32 deletions(-)
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index c683e2a928..27006662c4 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -772,7 +772,7 @@ AllocSetAllocLarge(MemoryContext context, Size size, int
flags)
*/
static inline void *
AllocSetAllocChunkFromBlock(MemoryContext context, AllocBlock block,
- Size size, Size
chunk_size, int fidx)
+ Size size, Size
chunk_size)
{
MemoryChunk *chunk;
@@ -785,7 +785,7 @@ AllocSetAllocChunkFromBlock(MemoryContext context,
AllocBlock block,
Assert(block->freeptr <= block->endptr);
/* store the free list index in the value field */
- MemoryChunkSetHdrMask(chunk, block, fidx, MCTX_ASET_ID);
+ MemoryChunkSetHdrMask(chunk, block, chunk_size, MCTX_ASET_ID);
#ifdef MEMORY_CONTEXT_CHECKING
chunk->requested_size = size;
@@ -817,14 +817,13 @@ AllocSetAllocChunkFromBlock(MemoryContext context,
AllocBlock block,
pg_noinline
static void *
AllocSetAllocFromNewBlock(MemoryContext context, Size size, int flags,
- int fidx)
+ Size chunk_size)
{
AllocSet set = (AllocSet) context;
AllocBlock block;
Size availspace;
Size blksize;
Size required_size;
- Size chunk_size;
/* due to the keeper block set->blocks should always be valid */
Assert(set->blocks != NULL);
@@ -869,7 +868,7 @@ AllocSetAllocFromNewBlock(MemoryContext context, Size size,
int flags,
availspace -= (availchunk + ALLOC_CHUNKHDRSZ);
/* store the freelist index in the value field */
- MemoryChunkSetHdrMask(chunk, block, a_fidx, MCTX_ASET_ID);
+ MemoryChunkSetHdrMask(chunk, block, availchunk, MCTX_ASET_ID);
#ifdef MEMORY_CONTEXT_CHECKING
chunk->requested_size = InvalidAllocSize; /* mark it free
*/
#endif
@@ -892,8 +891,6 @@ AllocSetAllocFromNewBlock(MemoryContext context, Size size,
int flags,
if (set->nextBlockSize > set->maxBlockSize)
set->nextBlockSize = set->maxBlockSize;
- /* Choose the actual chunk size to allocate */
- chunk_size = GetChunkSizeFromFreeListIdx(fidx);
Assert(chunk_size >= size);
/*
@@ -938,7 +935,7 @@ AllocSetAllocFromNewBlock(MemoryContext context, Size size,
int flags,
block->next->prev = block;
set->blocks = block;
- return AllocSetAllocChunkFromBlock(context, block, size, chunk_size,
fidx);
+ return AllocSetAllocChunkFromBlock(context, block, size, chunk_size);
}
/*
@@ -969,7 +966,9 @@ AllocSetAlloc(MemoryContext context, Size size, int flags)
AllocSet set = (AllocSet) context;
AllocBlock block;
MemoryChunk *chunk;
+#ifdef ALLOCSET_USE_FREELIST
int fidx;
+#endif
Size chunk_size;
Size availspace;
@@ -985,6 +984,7 @@ AllocSetAlloc(MemoryContext context, Size size, int flags)
if (size > set->allocChunkLimit)
return AllocSetAllocLarge(context, size, flags);
+#ifdef ALLOCSET_USE_FREELIST
/*
* Request is small enough to be treated as a chunk. Look in the
* corresponding free list to see if there is a free chunk we could
reuse.
@@ -998,7 +998,6 @@ AllocSetAlloc(MemoryContext context, Size size, int flags)
* doubling the memory requirements for such allocations.
*/
fidx = AllocSetFreeIndex(size);
-#ifdef ALLOCSET_USE_FREELIST
chunk = set->freelist[fidx];
if (chunk != NULL)
{
@@ -1007,7 +1006,7 @@ AllocSetAlloc(MemoryContext context, Size size, int flags)
/* Allow access to the chunk header. */
VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOC_CHUNKHDRSZ);
- Assert(fidx == MemoryChunkGetValue(chunk));
+ Assert(fidx == AllocSetFreeIndex(MemoryChunkGetValue(chunk)));
/* pop this chunk off the freelist */
VALGRIND_MAKE_MEM_DEFINED(link, sizeof(AllocFreeListLink));
@@ -1039,7 +1038,7 @@ AllocSetAlloc(MemoryContext context, Size size, int flags)
/*
* Choose the actual chunk size to allocate.
*/
- chunk_size = GetChunkSizeFromFreeListIdx(fidx);
+ chunk_size = GetChunkSizeFromFreeListIdx(AllocSetFreeIndex(size));
Assert(chunk_size >= size);
block = set->blocks;
@@ -1050,10 +1049,10 @@ AllocSetAlloc(MemoryContext context, Size size, int
flags)
* chunk into that block. Else must start a new one.
*/
if (unlikely(availspace < (chunk_size + ALLOC_CHUNKHDRSZ)))
- return AllocSetAllocFromNewBlock(context, size, flags, fidx);
+ return AllocSetAllocFromNewBlock(context, size, flags,
chunk_size);
/* There's enough space on the current block, so allocate from that */
- return AllocSetAllocChunkFromBlock(context, block, size, chunk_size,
fidx);
+ return AllocSetAllocChunkFromBlock(context, block, size, chunk_size);
}
/*
@@ -1113,6 +1112,7 @@ AllocSetFree(void *pointer)
AllocBlock block = MemoryChunkGetBlock(chunk);
int fidx;
AllocFreeListLink *link;
+ Size chunk_size;
/*
* In this path, for speed reasons we just Assert that the
referenced
@@ -1123,13 +1123,14 @@ AllocSetFree(void *pointer)
Assert(AllocBlockIsValid(block));
set = block->aset;
- fidx = MemoryChunkGetValue(chunk);
+ chunk_size = MemoryChunkGetValue(chunk);
+ fidx = AllocSetFreeIndex(chunk_size);
Assert(FreeListIdxIsValid(fidx));
link = GetFreeListLink(chunk);
#ifdef MEMORY_CONTEXT_CHECKING
/* Test for someone scribbling on unused space in chunk */
- if (chunk->requested_size < GetChunkSizeFromFreeListIdx(fidx))
+ if (chunk->requested_size < chunk_size)
if (!sentinel_ok(pointer, chunk->requested_size))
elog(WARNING, "detected write past chunk end in
%s %p",
set->header.name, chunk);
@@ -1174,7 +1175,6 @@ AllocSetRealloc(void *pointer, Size size, int flags)
AllocSet set;
MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
Size oldchksize;
- int fidx;
/* Allow access to the chunk header. */
VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOC_CHUNKHDRSZ);
@@ -1313,9 +1313,7 @@ AllocSetRealloc(void *pointer, Size size, int flags)
Assert(AllocBlockIsValid(block));
set = block->aset;
- fidx = MemoryChunkGetValue(chunk);
- Assert(FreeListIdxIsValid(fidx));
- oldchksize = GetChunkSizeFromFreeListIdx(fidx);
+ oldchksize = MemoryChunkGetValue(chunk);
#ifdef MEMORY_CONTEXT_CHECKING
/* Test for someone scribbling on unused space in chunk */
@@ -1464,7 +1462,6 @@ Size
AllocSetGetChunkSpace(void *pointer)
{
MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
- int fidx;
/* Allow access to the chunk header. */
VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOC_CHUNKHDRSZ);
@@ -1481,13 +1478,10 @@ AllocSetGetChunkSpace(void *pointer)
return block->endptr - (char *) chunk;
}
- fidx = MemoryChunkGetValue(chunk);
- Assert(FreeListIdxIsValid(fidx));
-
/* Disallow access to the chunk header. */
VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
- return GetChunkSizeFromFreeListIdx(fidx) + ALLOC_CHUNKHDRSZ;
+ return MemoryChunkGetValue(chunk) + ALLOC_CHUNKHDRSZ;
}
/*
@@ -1554,7 +1548,6 @@ AllocSetStats(MemoryContext context,
/* Allow access to the chunk header. */
VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOC_CHUNKHDRSZ);
- Assert(MemoryChunkGetValue(chunk) == fidx);
VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOC_CHUNKHDRSZ);
freechunks++;
@@ -1665,13 +1658,7 @@ AllocSetCheck(MemoryContext context)
}
else
{
- int fidx =
MemoryChunkGetValue(chunk);
-
- if (!FreeListIdxIsValid(fidx))
- elog(WARNING, "problem in alloc set %s:
bad chunk size for chunk %p in block %p",
- name, chunk, block);
-
- chsize = GetChunkSizeFromFreeListIdx(fidx); /*
aligned chunk size */
+ chsize = MemoryChunkGetValue(chunk); /* aligned
chunk size */
/*
* Check the stored block offset correctly
references this
--
2.40.1.windows.1
From 774edcd23cc68b526bbb971db55f6eb30bf8a763 Mon Sep 17 00:00:00 2001
From: David Rowley <dgrow...@gmail.com>
Date: Mon, 25 Mar 2024 22:04:51 +1300
Subject: [PATCH 1/2] Modify AllocSetAlloc not to look at freelists
---
src/backend/utils/mmgr/aset.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index 751cc3408c..c683e2a928 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -998,6 +998,7 @@ AllocSetAlloc(MemoryContext context, Size size, int flags)
* doubling the memory requirements for such allocations.
*/
fidx = AllocSetFreeIndex(size);
+#ifdef ALLOCSET_USE_FREELIST
chunk = set->freelist[fidx];
if (chunk != NULL)
{
@@ -1033,6 +1034,7 @@ AllocSetAlloc(MemoryContext context, Size size, int flags)
return MemoryChunkGetPointer(chunk);
}
+#endif
/*
* Choose the actual chunk size to allocate.
--
2.40.1.windows.1
