Em seg., 6 de jun. de 2022 às 21:14, Ranier Vilela <[email protected]>
escreveu:
> Em seg., 6 de jun. de 2022 às 20:37, David Rowley <[email protected]>
> escreveu:
>
>> On Mon, 6 Jun 2022 at 07:28, Ranier Vilela <[email protected]> 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).
>>
>> 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
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
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
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);
