Reducing Memory Consumption (aset and generation)
HI hackers,
I thought it would be better to start a new thread to discuss.
While working with sorting patch, and read others threads,
I have some ideas to reduces memory consumption by aset and generation
memory modules.
I have done basic benchmarks, and it seems to improve performance.
I think it's really worth it, if it really is possible to reduce memory
consumption.
Linux Ubuntu 64 bits
work_mem = 64MB
set max_parallel_workers_per_gather = 0;
create table t (a bigint not null, b bigint not null, c bigint not
null, d bigint not null, e bigint not null, f bigint not null);
insert into t select x,x,x,x,x,x from generate_Series(1,140247142) x; --
10GB!
vacuum freeze t;
select * from t order by a offset 140247142;
HEAD:
postgres=# select * from t order by a offset 140247142;
a | b | c | d | e | f
---+---+---+---+---+---
(0 rows)
work_mem=64MB
Time: 99603,544 ms (01:39,604)
Time: 94000,342 ms (01:34,000)
postgres=# set work_mem="64.2MB";
SET
Time: 0,210 ms
postgres=# select * from t order by a offset 140247142;
a | b | c | d | e | f
---+---+---+---+---+---
(0 rows)
Time: 95306,254 ms (01:35,306)
PATCHED:
postgres=# explain analyze select * from t order by a offset 140247142;
a | b | c | d | e | f
---+---+---+---+---+---
(0 rows)
work_mem=64MB
Time: 90946,482 ms (01:30,946)
postgres=# set work_mem="64.2MB";
SET
Time: 0,210 ms
postgres=# select * from t order by a offset 140247142;
a | b | c | d | e | f
---+---+---+---+---+---
(0 rows)
Time: 91817,533 ms (01:31,818)
There is still room for further improvements, and at this point I need help.
Regarding the patches we have:
1) 001-aset-reduces-memory-consumption.patch
Reduces memory used by struct AllocBlockData by minus 8 bits,
reducing the total size to 32 bits, which leads to "fitting" two structs in
a 64bit cache.
Move some stores to fields struct, for the order of declaration, within the
structures.
Remove tests elog(ERROR, "could not find block containing chunk %p" and
elog(ERROR, "could not find block containing chunk %p", moving them to
MEMORY_CONTEXT_CHECKING context.
Since 8.2 versions, nobody complains about these tests.
But if is not acceptable, have the option (3)
003-aset-reduces-memory-consumption.patch
2) 002-generation-reduces-memory-consumption.patch
Reduces memory used by struct GenerationBlock, by minus 8 bits,
reducing the total size to 32 bits, which leads to "fitting" two structs in
a 64bit cache.
Remove all references to the field *block* used by struct GenerationChunk,
enabling its removal! (not done yet).
What would take the final size to 16 bits, which leads to "fitting" four
structs in a 64bit cache.
Unfortunately, everything works only for the size 24, see the (4).
Move some stores to fields struct, for the order of declaration, within the
structures.
3) 003-aset-reduces-memory-consumption.patch
Same to the (1), but without remove the tests:
elog(ERROR, "could not find block containing chunk %p" and
elog(ERROR, "could not find block containing chunk %p",
But at the cost of removing a one tiny part of the tests.
Since 8.2 versions, nobody complains about these tests.
4) 004-generation-reduces-memory-consumption-BUG.patch
Same to the (2), but with BUG.
It only takes a few tweaks to completely remove the field block.
@@ -117,9 +116,9 @@ struct GenerationChunk
/* this is zero in a free chunk */
Size requested_size;
-#define GENERATIONCHUNK_RAWSIZE (SIZEOF_SIZE_T * 2 + SIZEOF_VOID_P * 2)
+#define GENERATIONCHUNK_RAWSIZE (SIZEOF_SIZE_T * 2 + SIZEOF_VOID_P)
#else
-#define GENERATIONCHUNK_RAWSIZE (SIZEOF_SIZE_T + SIZEOF_VOID_P * 2)
+#define GENERATIONCHUNK_RAWSIZE (SIZEOF_SIZE_T + SIZEOF_VOID_P)
#endif /* MEMORY_CONTEXT_CHECKING */ /* ensure proper alignment by adding padding if needed */
@@ -127,7 +126,6 @@ struct GenerationChunk
char padding[MAXIMUM_ALIGNOF - GENERATIONCHUNK_RAWSIZE % MAXIMUM_ALIGNOF];
#endif
- GenerationBlock *block; /* block owning this chunk */
GenerationContext *context; /* owning context, or NULL if freed chunk */
/* there must not be any padding to reach a MAXALIGN boundary here! */
};
This fails with make check.
I couldn't figure out why it doesn't work with 16 bits (struct
GenerationChunk).
Attachments:
001-aset-reduces-memory-consumption.patchtext/x-patch; charset=US-ASCII; name=001-aset-reduces-memory-consumption.patchDownload
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index ec3e264a73..5e624ff733 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -150,11 +150,13 @@ typedef AllocSetContext *AllocSet;
*/
typedef struct AllocBlockData
{
- AllocSet aset; /* aset that owns this block */
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 */
+#ifdef MEMORY_CONTEXT_CHECKING
+ AllocSet aset; /* aset that owns this block */
+#endif
} AllocBlockData;
/*
@@ -467,7 +469,7 @@ AllocSetContextCreateInternal(MemoryContext parent,
* the context header and its block header follows that.
*/
set = (AllocSet) malloc(firstBlockSize);
- if (set == NULL)
+ if (unlikely(set == NULL))
{
if (TopMemoryContext)
MemoryContextStats(TopMemoryContext);
@@ -485,11 +487,13 @@ AllocSetContextCreateInternal(MemoryContext parent,
/* Fill in the initial block's block header */
block = (AllocBlock) (((char *) set) + MAXALIGN(sizeof(AllocSetContext)));
- block->aset = set;
block->freeptr = ((char *) block) + ALLOC_BLOCKHDRSZ;
block->endptr = ((char *) set) + firstBlockSize;
block->prev = NULL;
block->next = NULL;
+#ifdef MEMORY_CONTEXT_CHECKING
+ block->aset = set;
+#endif
/* Mark unallocated space NOACCESS; leave the block header alone. */
VALGRIND_MAKE_MEM_NOACCESS(block->freeptr, block->endptr - block->freeptr);
@@ -738,17 +742,18 @@ AllocSetAlloc(MemoryContext context, Size size)
chunk_size = MAXALIGN(size);
blksize = chunk_size + ALLOC_BLOCKHDRSZ + ALLOC_CHUNKHDRSZ;
block = (AllocBlock) malloc(blksize);
- if (block == NULL)
+ if (unlikely(block == NULL))
return NULL;
- context->mem_allocated += blksize;
-
- block->aset = set;
block->freeptr = block->endptr = ((char *) block) + blksize;
+#ifdef MEMORY_CONTEXT_CHECKING
+ block->aset = set;
+#endif
chunk = (AllocChunk) (((char *) block) + ALLOC_BLOCKHDRSZ);
- chunk->aset = set;
chunk->size = chunk_size;
+ chunk->aset = set;
+
#ifdef MEMORY_CONTEXT_CHECKING
chunk->requested_size = size;
/* set mark to catch clobber of "unused" space */
@@ -779,6 +784,8 @@ AllocSetAlloc(MemoryContext context, Size size)
set->blocks = block;
}
+ context->mem_allocated += blksize;
+
/* Ensure any padding bytes are marked NOACCESS. */
VALGRIND_MAKE_MEM_NOACCESS((char *) AllocChunkGetPointer(chunk) + size,
chunk_size - size);
@@ -931,14 +938,14 @@ AllocSetAlloc(MemoryContext context, Size size)
block = (AllocBlock) malloc(blksize);
}
- if (block == NULL)
+ if (unlikely(block == NULL))
return NULL;
- context->mem_allocated += blksize;
-
- block->aset = set;
block->freeptr = ((char *) block) + ALLOC_BLOCKHDRSZ;
block->endptr = ((char *) block) + blksize;
+#ifdef MEMORY_CONTEXT_CHECKING
+ block->aset = set;
+#endif
/* Mark unallocated space NOACCESS. */
VALGRIND_MAKE_MEM_NOACCESS(block->freeptr,
@@ -948,7 +955,9 @@ AllocSetAlloc(MemoryContext context, Size size)
block->next = set->blocks;
if (block->next)
block->next->prev = block;
+
set->blocks = block;
+ context->mem_allocated += blksize;
}
/*
@@ -964,6 +973,7 @@ AllocSetAlloc(MemoryContext context, Size size)
chunk->aset = (void *) set;
chunk->size = chunk_size;
+
#ifdef MEMORY_CONTEXT_CHECKING
chunk->requested_size = size;
/* set mark to catch clobber of "unused" space */
@@ -1014,6 +1024,7 @@ AllocSetFree(MemoryContext context, void *pointer)
*/
AllocBlock block = (AllocBlock) (((char *) chunk) - ALLOC_BLOCKHDRSZ);
+#ifdef MEMORY_CONTEXT_CHECKING
/*
* Try to verify that we have a sane block pointer: it should
* reference the correct aset, and freeptr and endptr should point
@@ -1024,6 +1035,7 @@ AllocSetFree(MemoryContext context, void *pointer)
block->freeptr != ((char *) block) +
(chunk->size + ALLOC_BLOCKHDRSZ + ALLOC_CHUNKHDRSZ))
elog(ERROR, "could not find block containing chunk %p", chunk);
+#endif
/* OK, remove block from aset's list and free it */
if (block->prev)
@@ -1103,6 +1115,7 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size size)
Size blksize;
Size oldblksize;
+#ifdef MEMORY_CONTEXT_CHECKING
/*
* Try to verify that we have a sane block pointer: it should
* reference the correct aset, and freeptr and endptr should point
@@ -1113,6 +1126,7 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size size)
block->freeptr != ((char *) block) +
(oldsize + ALLOC_BLOCKHDRSZ + ALLOC_CHUNKHDRSZ))
elog(ERROR, "could not find block containing chunk %p", chunk);
+#endif
/*
* Even if the new request is less than set->allocChunkLimit, we stick
@@ -1128,17 +1142,13 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size size)
oldblksize = block->endptr - ((char *) block);
block = (AllocBlock) realloc(block, blksize);
- if (block == NULL)
+ if (unlikely(block == NULL))
{
/* Disallow external access to private part of chunk header. */
VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOCCHUNK_PRIVATE_LEN);
return NULL;
}
- /* updated separately, not to underflow when (oldblksize > blksize) */
- context->mem_allocated -= oldblksize;
- context->mem_allocated += blksize;
-
block->freeptr = block->endptr = ((char *) block) + blksize;
/* Update pointers since block has likely been moved */
@@ -1186,6 +1196,10 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size size)
VALGRIND_MAKE_MEM_DEFINED(pointer, oldsize);
#endif
+ /* updated separately, not to underflow when (oldblksize > blksize) */
+ context->mem_allocated -= oldblksize;
+ context->mem_allocated += blksize;
+
/* Ensure any padding bytes are marked NOACCESS. */
VALGRIND_MAKE_MEM_NOACCESS((char *) pointer + size, chksize - size);
@@ -1263,7 +1277,7 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size size)
newPointer = AllocSetAlloc((MemoryContext) set, size);
/* leave immediately if request was not completed */
- if (newPointer == NULL)
+ if (unlikely(newPointer == NULL))
{
/* Disallow external access to private part of chunk header. */
VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOCCHUNK_PRIVATE_LEN);
002-generation-reduces-memory-consumption.patchtext/x-patch; charset=US-ASCII; name=002-generation-reduces-memory-consumption.patchDownload
diff --git a/src/backend/utils/mmgr/generation.c b/src/backend/utils/mmgr/generation.c
index e530e272e0..17d768159f 100644
--- a/src/backend/utils/mmgr/generation.c
+++ b/src/backend/utils/mmgr/generation.c
@@ -89,7 +89,6 @@ typedef struct GenerationContext
struct GenerationBlock
{
dlist_node node; /* doubly-linked list of blocks */
- Size blksize; /* allocated size of this block */
int nchunks; /* number of chunks in the block */
int nfree; /* number of free chunks */
char *freeptr; /* start of free space in this block */
@@ -127,7 +126,7 @@ struct GenerationChunk
char padding[MAXIMUM_ALIGNOF - GENERATIONCHUNK_RAWSIZE % MAXIMUM_ALIGNOF];
#endif
- GenerationBlock *block; /* block owning this chunk */
+ GenerationBlock *block; /* TODO: remove me */
GenerationContext *context; /* owning context, or NULL if freed chunk */
/* there must not be any padding to reach a MAXALIGN boundary here! */
};
@@ -258,7 +257,7 @@ GenerationContextCreate(MemoryContext parent,
* starts with the context header and its block header follows that.
*/
set = (GenerationContext *) malloc(allocSize);
- if (set == NULL)
+ if (unlikely(set == NULL))
{
MemoryContextStats(TopMemoryContext);
ereport(ERROR,
@@ -276,12 +275,15 @@ GenerationContextCreate(MemoryContext parent,
/* Fill in the initial block's block header */
block = (GenerationBlock *) (((char *) set) + MAXALIGN(sizeof(GenerationContext)));
+
/* determine the block size and initialize it */
firstBlockSize = allocSize - MAXALIGN(sizeof(GenerationContext));
GenerationBlockInit(block, firstBlockSize);
- /* add it to the doubly-linked list of blocks */
- dlist_push_head(&set->blocks, &block->node);
+ /* Fill in GenerationContext-specific header fields */
+ set->initBlockSize = initBlockSize;
+ set->maxBlockSize = maxBlockSize;
+ set->nextBlockSize = initBlockSize;
/* use it as the current allocation block */
set->block = block;
@@ -292,10 +294,8 @@ GenerationContextCreate(MemoryContext parent,
/* Mark block as not to be released at reset time */
set->keeper = block;
- /* Fill in GenerationContext-specific header fields */
- set->initBlockSize = initBlockSize;
- set->maxBlockSize = maxBlockSize;
- set->nextBlockSize = initBlockSize;
+ /* add it to the doubly-linked list of blocks */
+ dlist_push_head(&set->blocks, &block->node);
/*
* Compute the allocation chunk size limit for this context.
@@ -356,12 +356,12 @@ GenerationReset(MemoryContext context)
GenerationBlockFree(set, block);
}
- /* set it so new allocations to make use of the keeper block */
- set->block = set->keeper;
-
/* Reset block size allocation sequence, too */
set->nextBlockSize = set->initBlockSize;
+ /* set it so new allocations to make use of the keeper block */
+ set->block = set->keeper;
+
/* Ensure there is only 1 item in the dlist */
Assert(!dlist_is_empty(&set->blocks));
Assert(!dlist_has_next(&set->blocks, dlist_head_node(&set->blocks)));
@@ -408,23 +408,22 @@ GenerationAlloc(MemoryContext context, Size size)
Size blksize = required_size + Generation_BLOCKHDRSZ;
block = (GenerationBlock *) malloc(blksize);
- if (block == NULL)
+ if (unlikely(block == NULL))
return NULL;
- context->mem_allocated += blksize;
-
/* block with a single (used) chunk */
- block->blksize = blksize;
block->nchunks = 1;
block->nfree = 0;
/* the block is completely full */
block->freeptr = block->endptr = ((char *) block) + blksize;
+ /* add the block to the list of allocated blocks */
+ dlist_push_head(&set->blocks, &block->node);
+
chunk = (GenerationChunk *) (((char *) block) + Generation_BLOCKHDRSZ);
- chunk->block = block;
- chunk->context = set;
chunk->size = chunk_size;
+ chunk->context = set;
#ifdef MEMORY_CONTEXT_CHECKING
chunk->requested_size = size;
@@ -437,8 +436,7 @@ GenerationAlloc(MemoryContext context, Size size)
randomize_mem((char *) GenerationChunkGetPointer(chunk), size);
#endif
- /* add the block to the list of allocated blocks */
- dlist_push_head(&set->blocks, &block->node);
+ context->mem_allocated += blksize;
/* Ensure any padding bytes are marked NOACCESS. */
VALGRIND_MAKE_MEM_NOACCESS((char *) GenerationChunkGetPointer(chunk) + size,
@@ -470,7 +468,6 @@ GenerationAlloc(MemoryContext context, Size size)
if (block == NULL ||
GenerationBlockFreeBytes(block) < required_size)
{
- Size blksize;
GenerationBlock *freeblock = set->freeblock;
if (freeblock != NULL &&
@@ -492,6 +489,8 @@ GenerationAlloc(MemoryContext context, Size size)
}
else
{
+ Size blksize;
+
/*
* The first such block has size initBlockSize, and we double the
* space in each succeeding block, but not more than maxBlockSize.
@@ -510,17 +509,17 @@ GenerationAlloc(MemoryContext context, Size size)
block = (GenerationBlock *) malloc(blksize);
- if (block == NULL)
+ if (unlikely(block == NULL))
return NULL;
- context->mem_allocated += blksize;
-
/* initialize the new block */
GenerationBlockInit(block, blksize);
/* add it to the doubly-linked list of blocks */
dlist_push_head(&set->blocks, &block->node);
+ context->mem_allocated += blksize;
+
/* Zero out the freeblock in case it's become full */
set->freeblock = NULL;
}
@@ -543,9 +542,8 @@ GenerationAlloc(MemoryContext context, Size size)
Assert(block->freeptr <= block->endptr);
- chunk->block = block;
- chunk->context = set;
chunk->size = chunk_size;
+ chunk->context = set;
#ifdef MEMORY_CONTEXT_CHECKING
chunk->requested_size = size;
@@ -576,7 +574,6 @@ GenerationAlloc(MemoryContext context, Size size)
static inline void
GenerationBlockInit(GenerationBlock *block, Size blksize)
{
- block->blksize = blksize;
block->nchunks = 0;
block->nfree = 0;
@@ -647,10 +644,10 @@ GenerationBlockFree(GenerationContext *set, GenerationBlock *block)
/* release the block from the list of blocks */
dlist_delete(&block->node);
- ((MemoryContext) set)->mem_allocated -= block->blksize;
+ ((MemoryContext) set)->mem_allocated -= block->endptr - ((char *) block);
#ifdef CLOBBER_FREED_MEMORY
- wipe_mem(block, block->blksize);
+ wipe_mem(block, block->endptr - ((char *) block));
#endif
free(block);
@@ -671,8 +668,6 @@ GenerationFree(MemoryContext context, void *pointer)
/* Allow access to private part of chunk header. */
VALGRIND_MAKE_MEM_DEFINED(chunk, GENERATIONCHUNK_PRIVATE_LEN);
- block = chunk->block;
-
#ifdef MEMORY_CONTEXT_CHECKING
/* Test for someone scribbling on unused space in chunk */
if (chunk->requested_size < chunk->size)
@@ -693,6 +688,7 @@ GenerationFree(MemoryContext context, void *pointer)
chunk->requested_size = 0;
#endif
+ block = (GenerationBlock *) (((char *) pointer) - Generation_BLOCKHDRSZ);
block->nfree += 1;
Assert(block->nchunks > 0);
@@ -732,7 +728,7 @@ GenerationFree(MemoryContext context, void *pointer)
*/
dlist_delete(&block->node);
- context->mem_allocated -= block->blksize;
+ context->mem_allocated -= block->endptr - ((char *) block);
free(block);
}
@@ -822,7 +818,7 @@ GenerationRealloc(MemoryContext context, void *pointer, Size size)
newPointer = GenerationAlloc((MemoryContext) set, size);
/* leave immediately if request was not completed */
- if (newPointer == NULL)
+ if (unlikely(newPointer == NULL))
{
/* Disallow external access to private part of chunk header. */
VALGRIND_MAKE_MEM_NOACCESS(chunk, GENERATIONCHUNK_PRIVATE_LEN);
@@ -926,7 +922,7 @@ GenerationStats(MemoryContext context,
nblocks++;
nchunks += block->nchunks;
nfreechunks += block->nfree;
- totalspace += block->blksize;
+ totalspace += block->endptr - ((char *) block);
freespace += (block->endptr - block->freeptr);
}
@@ -977,7 +973,7 @@ GenerationCheck(MemoryContext context)
nchunks;
char *ptr;
- total_allocated += block->blksize;
+ total_allocated += block->endptr - ((char *) block);
/*
* nfree > nchunks is surely wrong. Equality is allowed as the block
@@ -1004,11 +1000,6 @@ GenerationCheck(MemoryContext context)
nchunks += 1;
- /* chunks have both block and context pointers, so check both */
- if (chunk->block != block)
- elog(WARNING, "problem in Generation %s: bogus block link in block %p, chunk %p",
- name, block, chunk);
-
/*
* Check for valid context pointer. Note this is an incomplete
* test, since palloc(0) produces an allocated chunk with
003-aset-reduces-memory-consumption.patchtext/x-patch; charset=US-ASCII; name=003-aset-reduces-memory-consumption.patchDownload
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index ec3e264a73..8d6d804025 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -150,11 +150,13 @@ typedef AllocSetContext *AllocSet;
*/
typedef struct AllocBlockData
{
- AllocSet aset; /* aset that owns this block */
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 */
+#ifdef MEMORY_CONTEXT_CHECKING
+ AllocSet aset; /* aset that owns this block */
+#endif
} AllocBlockData;
/*
@@ -467,7 +469,7 @@ AllocSetContextCreateInternal(MemoryContext parent,
* the context header and its block header follows that.
*/
set = (AllocSet) malloc(firstBlockSize);
- if (set == NULL)
+ if (unlikely(set == NULL))
{
if (TopMemoryContext)
MemoryContextStats(TopMemoryContext);
@@ -485,11 +487,13 @@ AllocSetContextCreateInternal(MemoryContext parent,
/* Fill in the initial block's block header */
block = (AllocBlock) (((char *) set) + MAXALIGN(sizeof(AllocSetContext)));
- block->aset = set;
block->freeptr = ((char *) block) + ALLOC_BLOCKHDRSZ;
block->endptr = ((char *) set) + firstBlockSize;
block->prev = NULL;
block->next = NULL;
+#ifdef MEMORY_CONTEXT_CHECKING
+ block->aset = set;
+#endif
/* Mark unallocated space NOACCESS; leave the block header alone. */
VALGRIND_MAKE_MEM_NOACCESS(block->freeptr, block->endptr - block->freeptr);
@@ -738,17 +742,18 @@ AllocSetAlloc(MemoryContext context, Size size)
chunk_size = MAXALIGN(size);
blksize = chunk_size + ALLOC_BLOCKHDRSZ + ALLOC_CHUNKHDRSZ;
block = (AllocBlock) malloc(blksize);
- if (block == NULL)
+ if (unlikely(block == NULL))
return NULL;
- context->mem_allocated += blksize;
-
- block->aset = set;
block->freeptr = block->endptr = ((char *) block) + blksize;
+#ifdef MEMORY_CONTEXT_CHECKING
+ block->aset = set;
+#endif
chunk = (AllocChunk) (((char *) block) + ALLOC_BLOCKHDRSZ);
- chunk->aset = set;
chunk->size = chunk_size;
+ chunk->aset = set;
+
#ifdef MEMORY_CONTEXT_CHECKING
chunk->requested_size = size;
/* set mark to catch clobber of "unused" space */
@@ -779,6 +784,8 @@ AllocSetAlloc(MemoryContext context, Size size)
set->blocks = block;
}
+ context->mem_allocated += blksize;
+
/* Ensure any padding bytes are marked NOACCESS. */
VALGRIND_MAKE_MEM_NOACCESS((char *) AllocChunkGetPointer(chunk) + size,
chunk_size - size);
@@ -931,14 +938,14 @@ AllocSetAlloc(MemoryContext context, Size size)
block = (AllocBlock) malloc(blksize);
}
- if (block == NULL)
+ if (unlikely(block == NULL))
return NULL;
- context->mem_allocated += blksize;
-
- block->aset = set;
block->freeptr = ((char *) block) + ALLOC_BLOCKHDRSZ;
block->endptr = ((char *) block) + blksize;
+#ifdef MEMORY_CONTEXT_CHECKING
+ block->aset = set;
+#endif
/* Mark unallocated space NOACCESS. */
VALGRIND_MAKE_MEM_NOACCESS(block->freeptr,
@@ -948,7 +955,9 @@ AllocSetAlloc(MemoryContext context, Size size)
block->next = set->blocks;
if (block->next)
block->next->prev = block;
+
set->blocks = block;
+ context->mem_allocated += blksize;
}
/*
@@ -964,6 +973,7 @@ AllocSetAlloc(MemoryContext context, Size size)
chunk->aset = (void *) set;
chunk->size = chunk_size;
+
#ifdef MEMORY_CONTEXT_CHECKING
chunk->requested_size = size;
/* set mark to catch clobber of "unused" space */
@@ -1019,8 +1029,7 @@ AllocSetFree(MemoryContext context, void *pointer)
* reference the correct aset, and freeptr and endptr should point
* just past the chunk.
*/
- if (block->aset != set ||
- block->freeptr != block->endptr ||
+ if (block->freeptr != block->endptr ||
block->freeptr != ((char *) block) +
(chunk->size + ALLOC_BLOCKHDRSZ + ALLOC_CHUNKHDRSZ))
elog(ERROR, "could not find block containing chunk %p", chunk);
@@ -1108,8 +1117,7 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size size)
* reference the correct aset, and freeptr and endptr should point
* just past the chunk.
*/
- if (block->aset != set ||
- block->freeptr != block->endptr ||
+ if (block->freeptr != block->endptr ||
block->freeptr != ((char *) block) +
(oldsize + ALLOC_BLOCKHDRSZ + ALLOC_CHUNKHDRSZ))
elog(ERROR, "could not find block containing chunk %p", chunk);
@@ -1128,17 +1136,13 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size size)
oldblksize = block->endptr - ((char *) block);
block = (AllocBlock) realloc(block, blksize);
- if (block == NULL)
+ if (unlikely(block == NULL))
{
/* Disallow external access to private part of chunk header. */
VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOCCHUNK_PRIVATE_LEN);
return NULL;
}
- /* updated separately, not to underflow when (oldblksize > blksize) */
- context->mem_allocated -= oldblksize;
- context->mem_allocated += blksize;
-
block->freeptr = block->endptr = ((char *) block) + blksize;
/* Update pointers since block has likely been moved */
@@ -1186,6 +1190,10 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size size)
VALGRIND_MAKE_MEM_DEFINED(pointer, oldsize);
#endif
+ /* updated separately, not to underflow when (oldblksize > blksize) */
+ context->mem_allocated -= oldblksize;
+ context->mem_allocated += blksize;
+
/* Ensure any padding bytes are marked NOACCESS. */
VALGRIND_MAKE_MEM_NOACCESS((char *) pointer + size, chksize - size);
@@ -1263,7 +1271,7 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size size)
newPointer = AllocSetAlloc((MemoryContext) set, size);
/* leave immediately if request was not completed */
- if (newPointer == NULL)
+ if (unlikely(newPointer == NULL))
{
/* Disallow external access to private part of chunk header. */
VALGRIND_MAKE_MEM_NOACCESS(chunk, ALLOCCHUNK_PRIVATE_LEN);
004-generation-reduces-memory-consumption_BUG.patchtext/x-patch; charset=US-ASCII; name=004-generation-reduces-memory-consumption_BUG.patchDownload
diff --git a/src/backend/utils/mmgr/generation.c b/src/backend/utils/mmgr/generation.c
index e530e272e0..8ac2144ddd 100644
--- a/src/backend/utils/mmgr/generation.c
+++ b/src/backend/utils/mmgr/generation.c
@@ -89,7 +89,6 @@ typedef struct GenerationContext
struct GenerationBlock
{
dlist_node node; /* doubly-linked list of blocks */
- Size blksize; /* allocated size of this block */
int nchunks; /* number of chunks in the block */
int nfree; /* number of free chunks */
char *freeptr; /* start of free space in this block */
@@ -117,9 +116,9 @@ struct GenerationChunk
/* this is zero in a free chunk */
Size requested_size;
-#define GENERATIONCHUNK_RAWSIZE (SIZEOF_SIZE_T * 2 + SIZEOF_VOID_P * 2)
+#define GENERATIONCHUNK_RAWSIZE (SIZEOF_SIZE_T * 2 + SIZEOF_VOID_P)
#else
-#define GENERATIONCHUNK_RAWSIZE (SIZEOF_SIZE_T + SIZEOF_VOID_P * 2)
+#define GENERATIONCHUNK_RAWSIZE (SIZEOF_SIZE_T + SIZEOF_VOID_P)
#endif /* MEMORY_CONTEXT_CHECKING */
/* ensure proper alignment by adding padding if needed */
@@ -127,7 +126,6 @@ struct GenerationChunk
char padding[MAXIMUM_ALIGNOF - GENERATIONCHUNK_RAWSIZE % MAXIMUM_ALIGNOF];
#endif
- GenerationBlock *block; /* block owning this chunk */
GenerationContext *context; /* owning context, or NULL if freed chunk */
/* there must not be any padding to reach a MAXALIGN boundary here! */
};
@@ -258,7 +256,7 @@ GenerationContextCreate(MemoryContext parent,
* starts with the context header and its block header follows that.
*/
set = (GenerationContext *) malloc(allocSize);
- if (set == NULL)
+ if (unlikely(set == NULL))
{
MemoryContextStats(TopMemoryContext);
ereport(ERROR,
@@ -276,12 +274,15 @@ GenerationContextCreate(MemoryContext parent,
/* Fill in the initial block's block header */
block = (GenerationBlock *) (((char *) set) + MAXALIGN(sizeof(GenerationContext)));
+
/* determine the block size and initialize it */
firstBlockSize = allocSize - MAXALIGN(sizeof(GenerationContext));
GenerationBlockInit(block, firstBlockSize);
- /* add it to the doubly-linked list of blocks */
- dlist_push_head(&set->blocks, &block->node);
+ /* Fill in GenerationContext-specific header fields */
+ set->initBlockSize = initBlockSize;
+ set->maxBlockSize = maxBlockSize;
+ set->nextBlockSize = initBlockSize;
/* use it as the current allocation block */
set->block = block;
@@ -292,10 +293,8 @@ GenerationContextCreate(MemoryContext parent,
/* Mark block as not to be released at reset time */
set->keeper = block;
- /* Fill in GenerationContext-specific header fields */
- set->initBlockSize = initBlockSize;
- set->maxBlockSize = maxBlockSize;
- set->nextBlockSize = initBlockSize;
+ /* add it to the doubly-linked list of blocks */
+ dlist_push_head(&set->blocks, &block->node);
/*
* Compute the allocation chunk size limit for this context.
@@ -356,12 +355,12 @@ GenerationReset(MemoryContext context)
GenerationBlockFree(set, block);
}
- /* set it so new allocations to make use of the keeper block */
- set->block = set->keeper;
-
/* Reset block size allocation sequence, too */
set->nextBlockSize = set->initBlockSize;
+ /* set it so new allocations to make use of the keeper block */
+ set->block = set->keeper;
+
/* Ensure there is only 1 item in the dlist */
Assert(!dlist_is_empty(&set->blocks));
Assert(!dlist_has_next(&set->blocks, dlist_head_node(&set->blocks)));
@@ -408,23 +407,22 @@ GenerationAlloc(MemoryContext context, Size size)
Size blksize = required_size + Generation_BLOCKHDRSZ;
block = (GenerationBlock *) malloc(blksize);
- if (block == NULL)
+ if (unlikely(block == NULL))
return NULL;
- context->mem_allocated += blksize;
-
/* block with a single (used) chunk */
- block->blksize = blksize;
block->nchunks = 1;
block->nfree = 0;
/* the block is completely full */
block->freeptr = block->endptr = ((char *) block) + blksize;
+ /* add the block to the list of allocated blocks */
+ dlist_push_head(&set->blocks, &block->node);
+
chunk = (GenerationChunk *) (((char *) block) + Generation_BLOCKHDRSZ);
- chunk->block = block;
- chunk->context = set;
chunk->size = chunk_size;
+ chunk->context = set;
#ifdef MEMORY_CONTEXT_CHECKING
chunk->requested_size = size;
@@ -437,8 +435,7 @@ GenerationAlloc(MemoryContext context, Size size)
randomize_mem((char *) GenerationChunkGetPointer(chunk), size);
#endif
- /* add the block to the list of allocated blocks */
- dlist_push_head(&set->blocks, &block->node);
+ context->mem_allocated += blksize;
/* Ensure any padding bytes are marked NOACCESS. */
VALGRIND_MAKE_MEM_NOACCESS((char *) GenerationChunkGetPointer(chunk) + size,
@@ -470,7 +467,6 @@ GenerationAlloc(MemoryContext context, Size size)
if (block == NULL ||
GenerationBlockFreeBytes(block) < required_size)
{
- Size blksize;
GenerationBlock *freeblock = set->freeblock;
if (freeblock != NULL &&
@@ -492,6 +488,8 @@ GenerationAlloc(MemoryContext context, Size size)
}
else
{
+ Size blksize;
+
/*
* The first such block has size initBlockSize, and we double the
* space in each succeeding block, but not more than maxBlockSize.
@@ -510,17 +508,17 @@ GenerationAlloc(MemoryContext context, Size size)
block = (GenerationBlock *) malloc(blksize);
- if (block == NULL)
+ if (unlikely(block == NULL))
return NULL;
- context->mem_allocated += blksize;
-
/* initialize the new block */
GenerationBlockInit(block, blksize);
/* add it to the doubly-linked list of blocks */
dlist_push_head(&set->blocks, &block->node);
+ context->mem_allocated += blksize;
+
/* Zero out the freeblock in case it's become full */
set->freeblock = NULL;
}
@@ -543,9 +541,8 @@ GenerationAlloc(MemoryContext context, Size size)
Assert(block->freeptr <= block->endptr);
- chunk->block = block;
- chunk->context = set;
chunk->size = chunk_size;
+ chunk->context = set;
#ifdef MEMORY_CONTEXT_CHECKING
chunk->requested_size = size;
@@ -576,7 +573,6 @@ GenerationAlloc(MemoryContext context, Size size)
static inline void
GenerationBlockInit(GenerationBlock *block, Size blksize)
{
- block->blksize = blksize;
block->nchunks = 0;
block->nfree = 0;
@@ -647,10 +643,10 @@ GenerationBlockFree(GenerationContext *set, GenerationBlock *block)
/* release the block from the list of blocks */
dlist_delete(&block->node);
- ((MemoryContext) set)->mem_allocated -= block->blksize;
+ ((MemoryContext) set)->mem_allocated -= block->endptr - ((char *) block);
#ifdef CLOBBER_FREED_MEMORY
- wipe_mem(block, block->blksize);
+ wipe_mem(block, block->endptr - ((char *) block));
#endif
free(block);
@@ -671,8 +667,6 @@ GenerationFree(MemoryContext context, void *pointer)
/* Allow access to private part of chunk header. */
VALGRIND_MAKE_MEM_DEFINED(chunk, GENERATIONCHUNK_PRIVATE_LEN);
- block = chunk->block;
-
#ifdef MEMORY_CONTEXT_CHECKING
/* Test for someone scribbling on unused space in chunk */
if (chunk->requested_size < chunk->size)
@@ -693,6 +687,7 @@ GenerationFree(MemoryContext context, void *pointer)
chunk->requested_size = 0;
#endif
+ block = (GenerationBlock *) (((char *) pointer) - Generation_BLOCKHDRSZ);
block->nfree += 1;
Assert(block->nchunks > 0);
@@ -732,7 +727,7 @@ GenerationFree(MemoryContext context, void *pointer)
*/
dlist_delete(&block->node);
- context->mem_allocated -= block->blksize;
+ context->mem_allocated -= block->endptr - ((char *) block);
free(block);
}
@@ -822,7 +817,7 @@ GenerationRealloc(MemoryContext context, void *pointer, Size size)
newPointer = GenerationAlloc((MemoryContext) set, size);
/* leave immediately if request was not completed */
- if (newPointer == NULL)
+ if (unlikely(newPointer == NULL))
{
/* Disallow external access to private part of chunk header. */
VALGRIND_MAKE_MEM_NOACCESS(chunk, GENERATIONCHUNK_PRIVATE_LEN);
@@ -926,7 +921,7 @@ GenerationStats(MemoryContext context,
nblocks++;
nchunks += block->nchunks;
nfreechunks += block->nfree;
- totalspace += block->blksize;
+ totalspace += block->endptr - ((char *) block);
freespace += (block->endptr - block->freeptr);
}
@@ -977,7 +972,7 @@ GenerationCheck(MemoryContext context)
nchunks;
char *ptr;
- total_allocated += block->blksize;
+ total_allocated += block->endptr - ((char *) block);
/*
* nfree > nchunks is surely wrong. Equality is allowed as the block
@@ -1004,11 +999,6 @@ GenerationCheck(MemoryContext context)
nchunks += 1;
- /* chunks have both block and context pointers, so check both */
- if (chunk->block != block)
- elog(WARNING, "problem in Generation %s: bogus block link in block %p, chunk %p",
- name, block, chunk);
-
/*
* Check for valid context pointer. Note this is an incomplete
* test, since palloc(0) produces an allocated chunk with
On Mon, 6 Jun 2022 at 07:28, Ranier Vilela <ranier.vf@gmail.com> wrote:
4) 004-generation-reduces-memory-consumption-BUG.patch
Same to the (2), but with BUG.
It only takes a few tweaks to completely remove the field block.
This fails with make check.
I couldn't figure out why it doesn't work with 16 bits (struct GenerationChunk).
I think you're misunderstanding how blocks and chunks work here. A
block can have many chunks. You can't find the block that a chunk is
on by subtracting Generation_BLOCKHDRSZ from the pointer given to
GenerationFree(). That would only work if the chunk happened to be the
first chunk on a block. If it's anything apart from that then you'll
be making adjustments to the memory of some prior chunk on the block.
I imagine this is the reason you can't get the tests to pass.
Can you also explain why you think moving code around randomly or
adding unlikely() macros helps reduce the memory consumption overheads
of generation contexts? I imagine you think that's helping to further
improve performance, but you've not offered any evidence of that
separately from the other changes you've made. If you think those are
useful changes then I recommend you run individual benchmarks and
offer those as proof that those changes are worthwhile.
David
Em seg., 6 de jun. de 2022 às 20:37, David Rowley <dgrowleyml@gmail.com>
escreveu:
On Mon, 6 Jun 2022 at 07:28, Ranier Vilela <ranier.vf@gmail.com> wrote:
4) 004-generation-reduces-memory-consumption-BUG.patch
Same to the (2), but with BUG.
It only takes a few tweaks to completely remove the field block.This fails with make check.
I couldn't figure out why it doesn't work with 16 bits (structGenerationChunk).
Hi David, thanks for taking a look at this.
I think you're misunderstanding how blocks and chunks work here. A
block can have many chunks. You can't find the block that a chunk is
on by subtracting Generation_BLOCKHDRSZ from the pointer given to
GenerationFree(). That would only work if the chunk happened to be the
first chunk on a block. If it's anything apart from that then you'll
be making adjustments to the memory of some prior chunk on the block.
I imagine this is the reason you can't get the tests to pass.
Ok, I am still learning about this.
Can you explain why subtracting Generation_BLOCKHDRSZ from the pointer,
works for sizeof(struct GenerationChunk) = 24 bits,
When all references for the block field have been removed.
This pass check-world.
Can you also explain why you think moving code around randomly or
adding unlikely() macros helps reduce the memory consumption overheads
of generation contexts?
Of course, those changes do not reduce memory consumption.
But, IMO, I think those changes improve the access to memory regions,
because of the locality of the data.
About "unlikely macros", this helps the branchs prediction, when most of
the time,
malloc and related functions, will not fail.
I imagine you think that's helping to further
improve performance, but you've not offered any evidence of that
separately from the other changes you've made. If you think those are
useful changes then I recommend you run individual benchmarks and
offer those as proof that those changes are worthwhile.
Ok, I can understand, are changes unrelated.
regards,
Ranier Vilela
Em seg., 6 de jun. de 2022 às 21:14, Ranier Vilela <ranier.vf@gmail.com>
escreveu:
Em seg., 6 de jun. de 2022 às 20:37, David Rowley <dgrowleyml@gmail.com>
escreveu:On Mon, 6 Jun 2022 at 07:28, Ranier Vilela <ranier.vf@gmail.com> wrote:
4) 004-generation-reduces-memory-consumption-BUG.patch
Same to the (2), but with BUG.
It only takes a few tweaks to completely remove the field block.This fails with make check.
I couldn't figure out why it doesn't work with 16 bits (structGenerationChunk).
Hi David, thanks for taking a look at this.
I think you're misunderstanding how blocks and chunks work here. A
block can have many chunks. You can't find the block that a chunk is
on by subtracting Generation_BLOCKHDRSZ from the pointer given to
GenerationFree(). That would only work if the chunk happened to be the
first chunk on a block. If it's anything apart from that then you'll
be making adjustments to the memory of some prior chunk on the block.
I imagine this is the reason you can't get the tests to pass.Ok, I am still learning about this.
Can you explain why subtracting Generation_BLOCKHDRSZ from the pointer,
works for sizeof(struct GenerationChunk) = 24 bits,
When all references for the block field have been removed.
This pass check-world.Can you also explain why you think moving code around randomly or
adding unlikely() macros helps reduce the memory consumption overheads
of generation contexts?Of course, those changes do not reduce memory consumption.
But, IMO, I think those changes improve the access to memory regions,
because of the locality of the data.About "unlikely macros", this helps the branchs prediction, when most of
the time,
malloc and related functions, will not fail.I imagine you think that's helping to further
improve performance, but you've not offered any evidence of that
separately from the other changes you've made. If you think those are
useful changes then I recommend you run individual benchmarks and
offer those as proof that those changes are worthwhile.Ok, I can understand, are changes unrelated.
Let's restart this, to simplify the review and commit work.
Regarding the patches now, we have:
1) v1-001-aset-reduces-memory-consumption.patch
Reduces memory used by struct AllocBlockData by minus 8 bits,
reducing the total size to 32 bits, which leads to "fitting" two structs in
a 64bit cache.
Remove tests elog(ERROR, "could not find block containing chunk %p" and
elog(ERROR, "could not find block containing chunk %p", moving them to
MEMORY_CONTEXT_CHECKING context.
Since 8.2 versions, nobody complains about these tests.
But if is not acceptable, have the option (3)
v1-003-aset-reduces-memory-consumption.patch
2) v1-002-generation-reduces-memory-consumption.patch
Reduces memory used by struct GenerationBlock, by minus 8 bits,
reducing the total size to 32 bits, which leads to "fitting" two structs in
a 64bit cache.
3) v1-003-aset-reduces-memory-consumption.patch
Same to the (1), but without remove the tests:
elog(ERROR, "could not find block containing chunk %p" and
elog(ERROR, "could not find block containing chunk %p",
But at the cost of removing a one tiny part of the tests and
moving them to MEMORY_CONTEXT_CHECKING context.
Since 8.2 versions, nobody complains about these tests.
regards,
Ranier Vilela
Attachments:
v1-001-aset-reduces-memory-consumption.patchtext/x-patch; charset=US-ASCII; name=v1-001-aset-reduces-memory-consumption.patchDownload
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index ec3e264a73..901a954508 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -150,11 +150,13 @@ typedef AllocSetContext *AllocSet;
*/
typedef struct AllocBlockData
{
- AllocSet aset; /* aset that owns this block */
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 */
+#ifdef MEMORY_CONTEXT_CHECKING
+ AllocSet aset; /* aset that owns this block */
+#endif
} AllocBlockData;
/*
@@ -485,11 +487,13 @@ AllocSetContextCreateInternal(MemoryContext parent,
/* Fill in the initial block's block header */
block = (AllocBlock) (((char *) set) + MAXALIGN(sizeof(AllocSetContext)));
- block->aset = set;
block->freeptr = ((char *) block) + ALLOC_BLOCKHDRSZ;
block->endptr = ((char *) set) + firstBlockSize;
block->prev = NULL;
block->next = NULL;
+#ifdef MEMORY_CONTEXT_CHECKING
+ block->aset = set;
+#endif
/* Mark unallocated space NOACCESS; leave the block header alone. */
VALGRIND_MAKE_MEM_NOACCESS(block->freeptr, block->endptr - block->freeptr);
@@ -743,8 +747,10 @@ AllocSetAlloc(MemoryContext context, Size size)
context->mem_allocated += blksize;
- block->aset = set;
block->freeptr = block->endptr = ((char *) block) + blksize;
+#ifdef MEMORY_CONTEXT_CHECKING
+ block->aset = set;
+#endif
chunk = (AllocChunk) (((char *) block) + ALLOC_BLOCKHDRSZ);
chunk->aset = set;
@@ -936,9 +942,11 @@ AllocSetAlloc(MemoryContext context, Size size)
context->mem_allocated += blksize;
- block->aset = set;
block->freeptr = ((char *) block) + ALLOC_BLOCKHDRSZ;
block->endptr = ((char *) block) + blksize;
+#ifdef MEMORY_CONTEXT_CHECKING
+ block->aset = set;
+#endif
/* Mark unallocated space NOACCESS. */
VALGRIND_MAKE_MEM_NOACCESS(block->freeptr,
@@ -1014,6 +1022,7 @@ AllocSetFree(MemoryContext context, void *pointer)
*/
AllocBlock block = (AllocBlock) (((char *) chunk) - ALLOC_BLOCKHDRSZ);
+#ifdef MEMORY_CONTEXT_CHECKING
/*
* Try to verify that we have a sane block pointer: it should
* reference the correct aset, and freeptr and endptr should point
@@ -1024,6 +1033,7 @@ AllocSetFree(MemoryContext context, void *pointer)
block->freeptr != ((char *) block) +
(chunk->size + ALLOC_BLOCKHDRSZ + ALLOC_CHUNKHDRSZ))
elog(ERROR, "could not find block containing chunk %p", chunk);
+#endif
/* OK, remove block from aset's list and free it */
if (block->prev)
@@ -1103,6 +1113,7 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size size)
Size blksize;
Size oldblksize;
+#ifdef MEMORY_CONTEXT_CHECKING
/*
* Try to verify that we have a sane block pointer: it should
* reference the correct aset, and freeptr and endptr should point
@@ -1113,6 +1124,7 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size size)
block->freeptr != ((char *) block) +
(oldsize + ALLOC_BLOCKHDRSZ + ALLOC_CHUNKHDRSZ))
elog(ERROR, "could not find block containing chunk %p", chunk);
+#endif
/*
* Even if the new request is less than set->allocChunkLimit, we stick
v1-002-generation-reduces-memory-consumption.patchtext/x-patch; charset=US-ASCII; name=v1-002-generation-reduces-memory-consumption.patchDownload
diff --git a/src/backend/utils/mmgr/generation.c b/src/backend/utils/mmgr/generation.c
index e530e272e0..2f4d7ced5f 100644
--- a/src/backend/utils/mmgr/generation.c
+++ b/src/backend/utils/mmgr/generation.c
@@ -89,7 +89,6 @@ typedef struct GenerationContext
struct GenerationBlock
{
dlist_node node; /* doubly-linked list of blocks */
- Size blksize; /* allocated size of this block */
int nchunks; /* number of chunks in the block */
int nfree; /* number of free chunks */
char *freeptr; /* start of free space in this block */
@@ -414,7 +413,6 @@ GenerationAlloc(MemoryContext context, Size size)
context->mem_allocated += blksize;
/* block with a single (used) chunk */
- block->blksize = blksize;
block->nchunks = 1;
block->nfree = 0;
@@ -576,7 +574,6 @@ GenerationAlloc(MemoryContext context, Size size)
static inline void
GenerationBlockInit(GenerationBlock *block, Size blksize)
{
- block->blksize = blksize;
block->nchunks = 0;
block->nfree = 0;
@@ -647,10 +644,10 @@ GenerationBlockFree(GenerationContext *set, GenerationBlock *block)
/* release the block from the list of blocks */
dlist_delete(&block->node);
- ((MemoryContext) set)->mem_allocated -= block->blksize;
+ ((MemoryContext) set)->mem_allocated -= block->endptr - ((char *) block);
#ifdef CLOBBER_FREED_MEMORY
- wipe_mem(block, block->blksize);
+ wipe_mem(block, block->endptr - ((char *) block));
#endif
free(block);
@@ -732,7 +729,8 @@ GenerationFree(MemoryContext context, void *pointer)
*/
dlist_delete(&block->node);
- context->mem_allocated -= block->blksize;
+ context->mem_allocated -= block->endptr - ((char *) block);
+
free(block);
}
@@ -926,7 +924,7 @@ GenerationStats(MemoryContext context,
nblocks++;
nchunks += block->nchunks;
nfreechunks += block->nfree;
- totalspace += block->blksize;
+ totalspace += block->endptr - ((char *) block);
freespace += (block->endptr - block->freeptr);
}
@@ -977,7 +975,7 @@ GenerationCheck(MemoryContext context)
nchunks;
char *ptr;
- total_allocated += block->blksize;
+ total_allocated += block->endptr - ((char *) block);
/*
* nfree > nchunks is surely wrong. Equality is allowed as the block
v1-003-aset-reduces-memory-consumption.patchtext/x-patch; charset=US-ASCII; name=v1-003-aset-reduces-memory-consumption.patchDownload
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index ec3e264a73..fbe33ad87e 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -150,11 +150,13 @@ typedef AllocSetContext *AllocSet;
*/
typedef struct AllocBlockData
{
- AllocSet aset; /* aset that owns this block */
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 */
+#ifdef MEMORY_CONTEXT_CHECKING
+ AllocSet aset; /* aset that owns this block */
+#endif
} AllocBlockData;
/*
@@ -485,11 +487,13 @@ AllocSetContextCreateInternal(MemoryContext parent,
/* Fill in the initial block's block header */
block = (AllocBlock) (((char *) set) + MAXALIGN(sizeof(AllocSetContext)));
- block->aset = set;
block->freeptr = ((char *) block) + ALLOC_BLOCKHDRSZ;
block->endptr = ((char *) set) + firstBlockSize;
block->prev = NULL;
block->next = NULL;
+#ifdef MEMORY_CONTEXT_CHECKING
+ block->aset = set;
+#endif
/* Mark unallocated space NOACCESS; leave the block header alone. */
VALGRIND_MAKE_MEM_NOACCESS(block->freeptr, block->endptr - block->freeptr);
@@ -743,8 +747,10 @@ AllocSetAlloc(MemoryContext context, Size size)
context->mem_allocated += blksize;
- block->aset = set;
block->freeptr = block->endptr = ((char *) block) + blksize;
+#ifdef MEMORY_CONTEXT_CHECKING
+ block->aset = set;
+#endif
chunk = (AllocChunk) (((char *) block) + ALLOC_BLOCKHDRSZ);
chunk->aset = set;
@@ -936,9 +942,11 @@ AllocSetAlloc(MemoryContext context, Size size)
context->mem_allocated += blksize;
- block->aset = set;
block->freeptr = ((char *) block) + ALLOC_BLOCKHDRSZ;
block->endptr = ((char *) block) + blksize;
+#ifdef MEMORY_CONTEXT_CHECKING
+ block->aset = set;
+#endif
/* Mark unallocated space NOACCESS. */
VALGRIND_MAKE_MEM_NOACCESS(block->freeptr,
@@ -1019,8 +1027,10 @@ AllocSetFree(MemoryContext context, void *pointer)
* reference the correct aset, and freeptr and endptr should point
* just past the chunk.
*/
- if (block->aset != set ||
- block->freeptr != block->endptr ||
+ if (block->freeptr != block->endptr ||
+#ifdef MEMORY_CONTEXT_CHECKING
+ block->aset != set ||
+#endif
block->freeptr != ((char *) block) +
(chunk->size + ALLOC_BLOCKHDRSZ + ALLOC_CHUNKHDRSZ))
elog(ERROR, "could not find block containing chunk %p", chunk);
@@ -1108,8 +1118,10 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size size)
* reference the correct aset, and freeptr and endptr should point
* just past the chunk.
*/
- if (block->aset != set ||
- block->freeptr != block->endptr ||
+ if (block->freeptr != block->endptr ||
+#ifdef MEMORY_CONTEXT_CHECKING
+ block->aset != set ||
+#endif
block->freeptr != ((char *) block) +
(oldsize + ALLOC_BLOCKHDRSZ + ALLOC_CHUNKHDRSZ))
elog(ERROR, "could not find block containing chunk %p", chunk);
On Tue, 7 Jun 2022 at 03:09, Ranier Vilela <ranier.vf@gmail.com> wrote:
Let's restart this, to simplify the review and commit work.
The patchset fails to apply. Could you send an updated version that
applies to the current master branch?
Regarding the patches now, we have:
1) v1-001-aset-reduces-memory-consumption.patch
Reduces memory used by struct AllocBlockData by minus 8 bits,
This seems reasonable, considering we don't generally use the field
for anything but validation.
reducing the total size to 32 bits, which leads to "fitting" two structs in a 64bit cache.
By bits, you mean bytes, right?
Regarding fitting 2 structs in 64 bytes, that point is moot, as each
of these structs are stored at the front of each malloc-ed block, so
you will never see more than one of these in the same cache line. Less
space used is nice, but not as critical there IMO.
Remove tests elog(ERROR, "could not find block containing chunk %p" and
elog(ERROR, "could not find block containing chunk %p", moving them to
MEMORY_CONTEXT_CHECKING context.Since 8.2 versions, nobody complains about these tests.
But if is not acceptable, have the option (3) v1-003-aset-reduces-memory-consumption.patch
2) v1-002-generation-reduces-memory-consumption.patch
Reduces memory used by struct GenerationBlock, by minus 8 bits,
That seems fairly straight-forward -- 8 bytes saved on each page isn't
a lot, but it's something.
reducing the total size to 32 bits, which leads to "fitting" two structs in a 64bit cache.
Your size accounting seems wrong. On 64-bit architectures, we have
dlist_node (=16) + Size (=8) + 2*int (=8) + 2 * (char*) (=16) = 48
bytes. Shaving off the Size field reduces that by 8 bytes to 40 bytes.
The argument of fitting 2 of these structures into one cache line is
moot again, because here, too, two of this struct will not share a
cache line (unless somehow we allocate 0-sized blocks, which would be
a bug).
3) v1-003-aset-reduces-memory-consumption.patch
Same to the (1), but without remove the tests:
elog(ERROR, "could not find block containing chunk %p" and
elog(ERROR, "could not find block containing chunk %p",
But at the cost of removing a one tiny part of the tests and
moving them to MEMORY_CONTEXT_CHECKING context.
I like this patch over 001 due to allowing less corruption to occur in
the memory context code. This allows for detecting some issues in 003,
as opposed to none in 001.
Kind regards,
Matthias van de Meent
Hi,
Thanks for take a look.
Em seg., 11 de jul. de 2022 às 05:48, Matthias van de Meent <
boekewurm+postgres@gmail.com> escreveu:
On Tue, 7 Jun 2022 at 03:09, Ranier Vilela <ranier.vf@gmail.com> wrote:
Let's restart this, to simplify the review and commit work.
The patchset fails to apply. Could you send an updated version that
applies to the current master branch?
Sure.
Regarding the patches now, we have:
1) v1-001-aset-reduces-memory-consumption.patch
Reduces memory used by struct AllocBlockData by minus 8 bits,This seems reasonable, considering we don't generally use the field
for anything but validation.reducing the total size to 32 bits, which leads to "fitting" two structs
in a 64bit cache.
By bits, you mean bytes, right?
Correct.
Regarding fitting 2 structs in 64 bytes, that point is moot, as each
of these structs are stored at the front of each malloc-ed block, so
you will never see more than one of these in the same cache line. Less
space used is nice, but not as critical there IMO.Remove tests elog(ERROR, "could not find block containing chunk %p" and
elog(ERROR, "could not find block containing chunk %p", moving them to
MEMORY_CONTEXT_CHECKING context.Since 8.2 versions, nobody complains about these tests.
But if is not acceptable, have the option (3)
v1-003-aset-reduces-memory-consumption.patch
2) v1-002-generation-reduces-memory-consumption.patch
Reduces memory used by struct GenerationBlock, by minus 8 bits,That seems fairly straight-forward -- 8 bytes saved on each page isn't
a lot, but it's something.reducing the total size to 32 bits, which leads to "fitting" two structs
in a 64bit cache.
Your size accounting seems wrong. On 64-bit architectures, we have
dlist_node (=16) + Size (=8) + 2*int (=8) + 2 * (char*) (=16) = 48
bytes. Shaving off the Size field reduces that by 8 bytes to 40 bytes.The argument of fitting 2 of these structures into one cache line is
moot again, because here, too, two of this struct will not share a
cache line (unless somehow we allocate 0-sized blocks, which would be
a bug).
Right. I think I was very tired.
3) v1-003-aset-reduces-memory-consumption.patch
Same to the (1), but without remove the tests:
elog(ERROR, "could not find block containing chunk %p" and
elog(ERROR, "could not find block containing chunk %p",
But at the cost of removing a one tiny part of the tests and
moving them to MEMORY_CONTEXT_CHECKING context.I like this patch over 001 due to allowing less corruption to occur in
the memory context code. This allows for detecting some issues in 003,
as opposed to none in 001.
I understand.
regards,
Ranier Vilela
Em seg., 11 de jul. de 2022 às 09:25, Ranier Vilela <ranier.vf@gmail.com>
escreveu:
Hi,
Thanks for take a look.Em seg., 11 de jul. de 2022 às 05:48, Matthias van de Meent <
boekewurm+postgres@gmail.com> escreveu:On Tue, 7 Jun 2022 at 03:09, Ranier Vilela <ranier.vf@gmail.com> wrote:
Let's restart this, to simplify the review and commit work.
The patchset fails to apply. Could you send an updated version that
applies to the current master branch?Sure.
Here the patchs updated.
regards,
Ranier Vilela
Attachments:
v2-003-aset-reduces-memory-consumption.patchapplication/octet-stream; name=v2-003-aset-reduces-memory-consumption.patchDownload
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index ec3e264a73..fbe33ad87e 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -150,11 +150,13 @@ typedef AllocSetContext *AllocSet;
*/
typedef struct AllocBlockData
{
- AllocSet aset; /* aset that owns this block */
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 */
+#ifdef MEMORY_CONTEXT_CHECKING
+ AllocSet aset; /* aset that owns this block */
+#endif
} AllocBlockData;
/*
@@ -485,11 +487,13 @@ AllocSetContextCreateInternal(MemoryContext parent,
/* Fill in the initial block's block header */
block = (AllocBlock) (((char *) set) + MAXALIGN(sizeof(AllocSetContext)));
- block->aset = set;
block->freeptr = ((char *) block) + ALLOC_BLOCKHDRSZ;
block->endptr = ((char *) set) + firstBlockSize;
block->prev = NULL;
block->next = NULL;
+#ifdef MEMORY_CONTEXT_CHECKING
+ block->aset = set;
+#endif
/* Mark unallocated space NOACCESS; leave the block header alone. */
VALGRIND_MAKE_MEM_NOACCESS(block->freeptr, block->endptr - block->freeptr);
@@ -743,8 +747,10 @@ AllocSetAlloc(MemoryContext context, Size size)
context->mem_allocated += blksize;
- block->aset = set;
block->freeptr = block->endptr = ((char *) block) + blksize;
+#ifdef MEMORY_CONTEXT_CHECKING
+ block->aset = set;
+#endif
chunk = (AllocChunk) (((char *) block) + ALLOC_BLOCKHDRSZ);
chunk->aset = set;
@@ -936,9 +942,11 @@ AllocSetAlloc(MemoryContext context, Size size)
context->mem_allocated += blksize;
- block->aset = set;
block->freeptr = ((char *) block) + ALLOC_BLOCKHDRSZ;
block->endptr = ((char *) block) + blksize;
+#ifdef MEMORY_CONTEXT_CHECKING
+ block->aset = set;
+#endif
/* Mark unallocated space NOACCESS. */
VALGRIND_MAKE_MEM_NOACCESS(block->freeptr,
@@ -1019,8 +1027,10 @@ AllocSetFree(MemoryContext context, void *pointer)
* reference the correct aset, and freeptr and endptr should point
* just past the chunk.
*/
- if (block->aset != set ||
- block->freeptr != block->endptr ||
+ if (block->freeptr != block->endptr ||
+#ifdef MEMORY_CONTEXT_CHECKING
+ block->aset != set ||
+#endif
block->freeptr != ((char *) block) +
(chunk->size + ALLOC_BLOCKHDRSZ + ALLOC_CHUNKHDRSZ))
elog(ERROR, "could not find block containing chunk %p", chunk);
@@ -1108,8 +1118,10 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size size)
* reference the correct aset, and freeptr and endptr should point
* just past the chunk.
*/
- if (block->aset != set ||
- block->freeptr != block->endptr ||
+ if (block->freeptr != block->endptr ||
+#ifdef MEMORY_CONTEXT_CHECKING
+ block->aset != set ||
+#endif
block->freeptr != ((char *) block) +
(oldsize + ALLOC_BLOCKHDRSZ + ALLOC_CHUNKHDRSZ))
elog(ERROR, "could not find block containing chunk %p", chunk);v2-002-generation-reduces-memory-consumption.patchapplication/octet-stream; name=v2-002-generation-reduces-memory-consumption.patchDownload
diff --git a/src/backend/utils/mmgr/generation.c b/src/backend/utils/mmgr/generation.c
index e530e272e0..d7eeefe510 100644
--- a/src/backend/utils/mmgr/generation.c
+++ b/src/backend/utils/mmgr/generation.c
@@ -89,7 +89,6 @@ typedef struct GenerationContext
struct GenerationBlock
{
dlist_node node; /* doubly-linked list of blocks */
- Size blksize; /* allocated size of this block */
int nchunks; /* number of chunks in the block */
int nfree; /* number of free chunks */
char *freeptr; /* start of free space in this block */
@@ -414,7 +413,6 @@ GenerationAlloc(MemoryContext context, Size size)
context->mem_allocated += blksize;
/* block with a single (used) chunk */
- block->blksize = blksize;
block->nchunks = 1;
block->nfree = 0;
@@ -576,7 +574,6 @@ GenerationAlloc(MemoryContext context, Size size)
static inline void
GenerationBlockInit(GenerationBlock *block, Size blksize)
{
- block->blksize = blksize;
block->nchunks = 0;
block->nfree = 0;
@@ -647,10 +644,10 @@ GenerationBlockFree(GenerationContext *set, GenerationBlock *block)
/* release the block from the list of blocks */
dlist_delete(&block->node);
- ((MemoryContext) set)->mem_allocated -= block->blksize;
+ ((MemoryContext) set)->mem_allocated -= block->endptr - ((char *) block);
#ifdef CLOBBER_FREED_MEMORY
- wipe_mem(block, block->blksize);
+ wipe_mem(block, block->endptr - ((char *) block));
#endif
free(block);
@@ -732,7 +729,7 @@ GenerationFree(MemoryContext context, void *pointer)
*/
dlist_delete(&block->node);
- context->mem_allocated -= block->blksize;
+ context->mem_allocated -= block->endptr - ((char *) block);
free(block);
}
@@ -926,7 +923,7 @@ GenerationStats(MemoryContext context,
nblocks++;
nchunks += block->nchunks;
nfreechunks += block->nfree;
- totalspace += block->blksize;
+ totalspace += block->endptr - ((char *) block);
freespace += (block->endptr - block->freeptr);
}
@@ -977,7 +974,7 @@ GenerationCheck(MemoryContext context)
nchunks;
char *ptr;
- total_allocated += block->blksize;
+ total_allocated += block->endptr - ((char *) block);
/*
* nfree > nchunks is surely wrong. Equality is allowed as the blockv2-001-aset-reduces-memory-consumption.diffapplication/octet-stream; name=v2-001-aset-reduces-memory-consumption.diffDownload
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index ec3e264a73..901a954508 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -150,11 +150,13 @@ typedef AllocSetContext *AllocSet;
*/
typedef struct AllocBlockData
{
- AllocSet aset; /* aset that owns this block */
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 */
+#ifdef MEMORY_CONTEXT_CHECKING
+ AllocSet aset; /* aset that owns this block */
+#endif
} AllocBlockData;
/*
@@ -485,11 +487,13 @@ AllocSetContextCreateInternal(MemoryContext parent,
/* Fill in the initial block's block header */
block = (AllocBlock) (((char *) set) + MAXALIGN(sizeof(AllocSetContext)));
- block->aset = set;
block->freeptr = ((char *) block) + ALLOC_BLOCKHDRSZ;
block->endptr = ((char *) set) + firstBlockSize;
block->prev = NULL;
block->next = NULL;
+#ifdef MEMORY_CONTEXT_CHECKING
+ block->aset = set;
+#endif
/* Mark unallocated space NOACCESS; leave the block header alone. */
VALGRIND_MAKE_MEM_NOACCESS(block->freeptr, block->endptr - block->freeptr);
@@ -743,8 +747,10 @@ AllocSetAlloc(MemoryContext context, Size size)
context->mem_allocated += blksize;
- block->aset = set;
block->freeptr = block->endptr = ((char *) block) + blksize;
+#ifdef MEMORY_CONTEXT_CHECKING
+ block->aset = set;
+#endif
chunk = (AllocChunk) (((char *) block) + ALLOC_BLOCKHDRSZ);
chunk->aset = set;
@@ -936,9 +942,11 @@ AllocSetAlloc(MemoryContext context, Size size)
context->mem_allocated += blksize;
- block->aset = set;
block->freeptr = ((char *) block) + ALLOC_BLOCKHDRSZ;
block->endptr = ((char *) block) + blksize;
+#ifdef MEMORY_CONTEXT_CHECKING
+ block->aset = set;
+#endif
/* Mark unallocated space NOACCESS. */
VALGRIND_MAKE_MEM_NOACCESS(block->freeptr,
@@ -1014,6 +1022,7 @@ AllocSetFree(MemoryContext context, void *pointer)
*/
AllocBlock block = (AllocBlock) (((char *) chunk) - ALLOC_BLOCKHDRSZ);
+#ifdef MEMORY_CONTEXT_CHECKING
/*
* Try to verify that we have a sane block pointer: it should
* reference the correct aset, and freeptr and endptr should point
@@ -1024,6 +1033,7 @@ AllocSetFree(MemoryContext context, void *pointer)
block->freeptr != ((char *) block) +
(chunk->size + ALLOC_BLOCKHDRSZ + ALLOC_CHUNKHDRSZ))
elog(ERROR, "could not find block containing chunk %p", chunk);
+#endif
/* OK, remove block from aset's list and free it */
if (block->prev)
@@ -1103,6 +1113,7 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size size)
Size blksize;
Size oldblksize;
+#ifdef MEMORY_CONTEXT_CHECKING
/*
* Try to verify that we have a sane block pointer: it should
* reference the correct aset, and freeptr and endptr should point
@@ -1113,6 +1124,7 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size size)
block->freeptr != ((char *) block) +
(oldsize + ALLOC_BLOCKHDRSZ + ALLOC_CHUNKHDRSZ))
elog(ERROR, "could not find block containing chunk %p", chunk);
+#endif
/*
* Even if the new request is less than set->allocChunkLimit, we stickOn Mon, 11 Jul 2022 at 20:48, Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:
2) v1-002-generation-reduces-memory-consumption.patch
Reduces memory used by struct GenerationBlock, by minus 8 bits,That seems fairly straight-forward -- 8 bytes saved on each page isn't
a lot, but it's something.
I think 002 is likely the only patch here that has some merit.
However, it's hard to imagine any measurable performance gains from
it. I think the smallest generation block we have today is 8192
bytes. Saving 8 bytes in that equates to a saving of 0.1% of memory.
For an 8MB page, it's 1024 times less than that.
I imagine Ranier has been working on this due the performance
regression mentioned in [1]/messages/by-id/CAApHDvqXpLzav6dUeR5vO_RBh_feHrHMLhigVQXw9jHCyKP9PA@mail.gmail.com. I think it'll be much more worthwhile to
aim to reduce the memory chunk overheads rather than the block
overheads, as Ranier is doing here. I posted a patch in [2]/messages/by-id/CAApHDvpjauCRXcgcaL6+e3eqecEHoeRm9D-kcbuvBitgPnW=vw@mail.gmail.com which does
that. To make that work, I need to have the owning context in the
block. The 001 and 003 patch seems to remove those here.
David
[1]: /messages/by-id/CAApHDvqXpLzav6dUeR5vO_RBh_feHrHMLhigVQXw9jHCyKP9PA@mail.gmail.com
[2]: /messages/by-id/CAApHDvpjauCRXcgcaL6+e3eqecEHoeRm9D-kcbuvBitgPnW=vw@mail.gmail.com
Em ter., 12 de jul. de 2022 às 02:35, David Rowley <dgrowleyml@gmail.com>
escreveu:
On Mon, 11 Jul 2022 at 20:48, Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:2) v1-002-generation-reduces-memory-consumption.patch
Reduces memory used by struct GenerationBlock, by minus 8 bits,That seems fairly straight-forward -- 8 bytes saved on each page isn't
a lot, but it's something.I think 002 is likely the only patch here that has some merit.
However, it's hard to imagine any measurable performance gains from
it. I think the smallest generation block we have today is 8192
bytes. Saving 8 bytes in that equates to a saving of 0.1% of memory.
For an 8MB page, it's 1024 times less than that.
I imagine Ranier has been working on this due the performance
regression mentioned in [1]. I think it'll be much more worthwhile to
aim to reduce the memory chunk overheads rather than the block
overheads, as Ranier is doing here. I posted a patch in [2] which does
that. To make that work, I need to have the owning context in the
block. The 001 and 003 patch seems to remove those here.
I saw the numbers at [2], 17% is very impressive.
How you need the context in the block, 001 and 003,
they are more of a hindrance than a help.
So, feel free to incorporate 002 into your patch if you wish.
The best thing to do here is to close and withdraw from commitfest.
regards,
Ranier Vilela