From ff97741bc810390db6dd4da0f31ee1e93c8d3abb Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Wed, 8 Mar 2017 12:21:12 -0500
Subject: [PATCH] Use doubly-linked block lists in aset.c to reduce large-chunk
overhead.
Large chunks (those too large for any palloc freelist) are managed as
separate blocks. Formerly, realloc'ing or pfree'ing such a chunk required
O(N) time in a context with N blocks, since we had to traipse down the
singly-linked block list to locate the block's predecessor before we could
fix the list links. This can result in O(N^2) runtime in situations where
large numbers of such chunks are manipulated within one context. Cases
like that were not foreseen in the original design of aset.c, and indeed
didn't arise until fairly recently. But such problems can now occur in
reorderbuffer.c and in hash joining, both of which make repeated large
requests without scaling up their request size as they do so, and which
will free their requests in not-necessarily-LIFO order.
To fix, change the block list from singly-linked to doubly-linked.
This adds another 4 or 8 bytes to ALLOC_BLOCKHDRSZ, but that doesn't
seem like unacceptable overhead, since aset.c's blocks are normally
8K or more, and never less than 1K in current practice.
In passing, get rid of some redundant AllocChunkGetPointer() calls in
AllocSetRealloc (the compiler might be smart enough to optimize these
away anyway, but no need to assume that) and improve AllocSetCheck's
checking of block header fields.
Back-patch to 9.4 where reorderbuffer.c appeared. We could take this
further back, but currently there's no evidence that it would be useful.
Discussion: https://postgr.es/m/CAMkU=1x1hvue1XYrZoWk_omG0Ja5nBvTdvgrOeVkkeqs71CV8g@mail.gmail.com
---
src/backend/utils/mmgr/aset.c | 108 +++++++++++++++++++++-------------
1 file changed, 66 insertions(+), 42 deletions(-)
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index b7904f46829..45d793c1e86 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -148,7 +148,8 @@ typedef AllocSetContext *AllocSet;
typedef struct AllocBlockData
{
AllocSet aset; /* aset that owns this block */
- AllocBlock next; /* next block in aset's blocks list */
+ AllocBlock prev; /* prev block in aset's blocks list, if any */
+ AllocBlock next; /* next block in aset's blocks list, if any */
char *freeptr; /* start of free space in this block */
char *endptr; /* end of space in this block */
} AllocBlockData;
@@ -407,7 +408,10 @@ AllocSetContextCreate(MemoryContext parent,
block->aset = set;
block->freeptr = ((char *) block) + ALLOC_BLOCKHDRSZ;
block->endptr = ((char *) block) + blksize;
+ block->prev = NULL;
block->next = set->blocks;
+ if (block->next)
+ block->next->prev = block;
set->blocks = block;
/* Mark block as not to be released at reset time */
set->keeper = block;
@@ -489,6 +493,7 @@ AllocSetReset(MemoryContext context)
VALGRIND_MAKE_MEM_NOACCESS(datastart, block->freeptr - datastart);
#endif
block->freeptr = datastart;
+ block->prev = NULL;
block->next = NULL;
}
else
@@ -595,16 +600,20 @@ AllocSetAlloc(MemoryContext context, Size size)
#endif
/*
- * Stick the new block underneath the active allocation block, so that
- * we don't lose the use of the space remaining therein.
+ * Stick the new block underneath the active allocation block, if any,
+ * so that we don't lose the use of the space remaining therein.
*/
if (set->blocks != NULL)
{
+ block->prev = set->blocks;
block->next = set->blocks->next;
+ if (block->next)
+ block->next->prev = block;
set->blocks->next = block;
}
else
{
+ block->prev = NULL;
block->next = NULL;
set->blocks = block;
}
@@ -785,7 +794,10 @@ AllocSetAlloc(MemoryContext context, Size size)
VALGRIND_MAKE_MEM_NOACCESS(block->freeptr,
blksize - ALLOC_BLOCKHDRSZ);
+ block->prev = NULL;
block->next = set->blocks;
+ if (block->next)
+ block->next->prev = block;
set->blocks = block;
}
@@ -845,29 +857,28 @@ AllocSetFree(MemoryContext context, void *pointer)
{
/*
* Big chunks are certain to have been allocated as single-chunk
- * blocks. Find the containing block and return it to malloc().
+ * blocks. Just unlink that block and return it to malloc().
*/
- AllocBlock block = set->blocks;
- AllocBlock prevblock = NULL;
+ AllocBlock block = (AllocBlock) (((char *) chunk) - ALLOC_BLOCKHDRSZ);
- while (block != NULL)
- {
- if (chunk == (AllocChunk) (((char *) block) + ALLOC_BLOCKHDRSZ))
- break;
- prevblock = block;
- block = block->next;
- }
- if (block == NULL)
+ /*
+ * Try to verify that we have a sane block pointer: it should
+ * reference the correct aset, and freeptr and endptr should point
+ * just past the chunk.
+ */
+ if (block->aset != set ||
+ block->freeptr != block->endptr ||
+ block->freeptr != ((char *) block) +
+ (chunk->size + ALLOC_BLOCKHDRSZ + ALLOC_CHUNKHDRSZ))
elog(ERROR, "could not find block containing chunk %p", chunk);
- /* let's just make sure chunk is the only one in the block */
- Assert(block->freeptr == ((char *) block) +
- (chunk->size + ALLOC_BLOCKHDRSZ + ALLOC_CHUNKHDRSZ));
/* OK, remove block from aset's list and free it */
- if (prevblock == NULL)
- set->blocks = block->next;
+ if (block->prev)
+ block->prev->next = block->next;
else
- prevblock->next = block->next;
+ set->blocks = block->next;
+ if (block->next)
+ block->next->prev = block->prev;
#ifdef CLOBBER_FREED_MEMORY
wipe_mem(block, block->freeptr - ((char *) block));
#endif
@@ -973,27 +984,24 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size size)
if (oldsize > set->allocChunkLimit)
{
/*
- * The chunk must have been allocated as a single-chunk block. Find
- * the containing block and use realloc() to make it bigger with
- * minimum space wastage.
+ * The chunk must have been allocated as a single-chunk block. Use
+ * realloc() to make the containing block bigger with minimum space
+ * wastage.
*/
- AllocBlock block = set->blocks;
- AllocBlock prevblock = NULL;
+ AllocBlock block = (AllocBlock) (((char *) chunk) - ALLOC_BLOCKHDRSZ);
Size chksize;
Size blksize;
- while (block != NULL)
- {
- if (chunk == (AllocChunk) (((char *) block) + ALLOC_BLOCKHDRSZ))
- break;
- prevblock = block;
- block = block->next;
- }
- if (block == NULL)
+ /*
+ * Try to verify that we have a sane block pointer: it should
+ * reference the correct aset, and freeptr and endptr should point
+ * just past the chunk.
+ */
+ if (block->aset != set ||
+ block->freeptr != block->endptr ||
+ block->freeptr != ((char *) block) +
+ (chunk->size + ALLOC_BLOCKHDRSZ + ALLOC_CHUNKHDRSZ))
elog(ERROR, "could not find block containing chunk %p", chunk);
- /* let's just make sure chunk is the only one in the block */
- Assert(block->freeptr == ((char *) block) +
- (chunk->size + ALLOC_BLOCKHDRSZ + ALLOC_CHUNKHDRSZ));
/* Do the realloc */
chksize = MAXALIGN(size);
@@ -1006,10 +1014,12 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size size)
/* Update pointers since block has likely been moved */
chunk = (AllocChunk) (((char *) block) + ALLOC_BLOCKHDRSZ);
pointer = AllocChunkGetPointer(chunk);
- if (prevblock == NULL)
- set->blocks = block;
+ if (block->prev)
+ block->prev->next = block;
else
- prevblock->next = block;
+ set->blocks = block;
+ if (block->next)
+ block->next->prev = block;
chunk->size = chksize;
#ifdef MEMORY_CONTEXT_CHECKING
@@ -1033,7 +1043,7 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size size)
/* set mark to catch clobber of "unused" space */
if (size < chunk->size)
- set_sentinel(AllocChunkGetPointer(chunk), size);
+ set_sentinel(pointer, size);
#else /* !MEMORY_CONTEXT_CHECKING */
/*
@@ -1046,7 +1056,8 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size size)
/* Make any trailing alignment padding NOACCESS. */
VALGRIND_MAKE_MEM_NOACCESS((char *) pointer + size, chksize - size);
- return AllocChunkGetPointer(chunk);
+
+ return pointer;
}
else
{
@@ -1200,9 +1211,12 @@ AllocSetCheck(MemoryContext context)
{
AllocSet set = (AllocSet) context;
char *name = set->header.name;
+ AllocBlock prevblock;
AllocBlock block;
- for (block = set->blocks; block != NULL; block = block->next)
+ for (prevblock = NULL, block = set->blocks;
+ block != NULL;
+ prevblock = block, block = block->next)
{
char *bpoz = ((char *) block) + ALLOC_BLOCKHDRSZ;
long blk_used = block->freeptr - bpoz;
@@ -1219,6 +1233,16 @@ AllocSetCheck(MemoryContext context)
name, block);
}
+ /*
+ * Check block header fields
+ */
+ if (block->aset != set ||
+ block->prev != prevblock ||
+ block->freeptr < bpoz ||
+ block->freeptr > block->endptr)
+ elog(WARNING, "problem in alloc set %s: corrupt header in block %p",
+ name, block);
+
/*
* Chunk walker
*/
--
GitLab