Changing types of block and chunk sizes in memory contexts
Hi hackers,
In memory contexts, block and chunk sizes are likely to be limited by
some upper bounds. Some examples of those bounds can be
MEMORYCHUNK_MAX_BLOCKOFFSET and MEMORYCHUNK_MAX_VALUE. Both values are
only 1 less than 1GB.
This makes memory contexts to have blocks/chunks with sizes less than
1GB. Such sizes can be stored in 32-bits. Currently, "Size" type,
which is 64-bit, is used, but 32-bit integers should be enough to
store any value less than 1GB.
Attached patch is an attempt to change the types of some fields to
uint32 from Size in aset, slab and generation memory contexts.
I tried to find most of the places that needed to be changed to
uint32, but I probably missed some. I can add more places if you feel
like it.
I would appreciate any feedback.
Thanks,
--
Melih Mutlu
Microsoft
Attachments:
0001-Change-memory-context-fields-to-uint32.patchapplication/octet-stream; name=0001-Change-memory-context-fields-to-uint32.patchDownload
From a11795b222b98ea51afc6c09b4ad8859d6b4284e Mon Sep 17 00:00:00 2001
From: Melih Mutlu <m.melihmutlu@gmail.com>
Date: Mon, 12 Jun 2023 08:50:28 +0300
Subject: [PATCH] Change memory context fields to uint32
Block sizes and chunk sizes can be upper bounded by
MEMORYCHUNK_MAX_BLOCKOFFSET and MEMORYCHUNK_MAX_VALUE respectively.
Values of both bounds correspond to 1 less than 1GB. This allows us
to store the sizes of any block or chunk size limited by those bounds
in 32 bits.
This patch changes types of such fields that represents block or chunk sizes
from 64-bit Size to 32-bit unsigned integers.
---
src/backend/utils/mmgr/aset.c | 16 ++++++++--------
src/backend/utils/mmgr/generation.c | 18 +++++++++---------
src/backend/utils/mmgr/slab.c | 8 ++++----
src/include/utils/memutils.h | 14 +++++++-------
4 files changed, 28 insertions(+), 28 deletions(-)
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index 0bbbf93672..62fb7e1f90 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -156,10 +156,10 @@ typedef struct AllocSetContext
AllocBlock blocks; /* head of list of blocks in this set */
MemoryChunk *freelist[ALLOCSET_NUM_FREELISTS]; /* free chunk lists */
/* Allocation parameters for this context: */
- Size initBlockSize; /* initial block size */
- Size maxBlockSize; /* maximum block size */
- Size nextBlockSize; /* next block size to allocate */
- Size allocChunkLimit; /* effective chunk size limit */
+ uint32 initBlockSize; /* initial block size */
+ uint32 maxBlockSize; /* maximum block size */
+ uint32 nextBlockSize; /* next block size to allocate */
+ uint32 allocChunkLimit; /* effective chunk size limit */
AllocBlock keeper; /* keep this block over resets */
/* freelist this context could be put in, or -1 if not a candidate: */
int freeListIndex; /* index in context_freelists[], or -1 */
@@ -340,12 +340,12 @@ AllocSetFreeIndex(Size size)
MemoryContext
AllocSetContextCreateInternal(MemoryContext parent,
const char *name,
- Size minContextSize,
- Size initBlockSize,
- Size maxBlockSize)
+ uint32 minContextSize,
+ uint32 initBlockSize,
+ uint32 maxBlockSize)
{
int freeListIndex;
- Size firstBlockSize;
+ uint32 firstBlockSize;
AllocSet set;
AllocBlock block;
diff --git a/src/backend/utils/mmgr/generation.c b/src/backend/utils/mmgr/generation.c
index 4fb8663cd6..a27d113af8 100644
--- a/src/backend/utils/mmgr/generation.c
+++ b/src/backend/utils/mmgr/generation.c
@@ -61,10 +61,10 @@ typedef struct GenerationContext
MemoryContextData header; /* Standard memory-context fields */
/* Generational context parameters */
- Size initBlockSize; /* initial block size */
- Size maxBlockSize; /* maximum block size */
- Size nextBlockSize; /* next block size to allocate */
- Size allocChunkLimit; /* effective chunk size limit */
+ uint32 initBlockSize; /* initial block size */
+ uint32 maxBlockSize; /* maximum block size */
+ uint32 nextBlockSize; /* next block size to allocate */
+ uint32 allocChunkLimit; /* effective chunk size limit */
GenerationBlock *block; /* current (most recently allocated) block, or
* NULL if we've just freed the most recent
@@ -149,12 +149,12 @@ static inline void GenerationBlockFree(GenerationContext *set,
MemoryContext
GenerationContextCreate(MemoryContext parent,
const char *name,
- Size minContextSize,
- Size initBlockSize,
- Size maxBlockSize)
+ uint32 minContextSize,
+ uint32 initBlockSize,
+ uint32 maxBlockSize)
{
- Size firstBlockSize;
- Size allocSize;
+ uint32 firstBlockSize;
+ uint32 allocSize;
GenerationContext *set;
GenerationBlock *block;
diff --git a/src/backend/utils/mmgr/slab.c b/src/backend/utils/mmgr/slab.c
index 718dd2ba03..43e01a480f 100644
--- a/src/backend/utils/mmgr/slab.c
+++ b/src/backend/utils/mmgr/slab.c
@@ -104,7 +104,7 @@ typedef struct SlabContext
{
MemoryContextData header; /* Standard memory-context fields */
/* Allocation parameters for this context: */
- Size chunkSize; /* the requested (non-aligned) chunk size */
+ uint32 chunkSize; /* the requested (non-aligned) chunk size */
Size fullChunkSize; /* chunk size with chunk header and alignment */
Size blockSize; /* the size to make each block of chunks */
int32 chunksPerBlock; /* number of chunks that fit in 1 block */
@@ -320,7 +320,7 @@ MemoryContext
SlabContextCreate(MemoryContext parent,
const char *name,
Size blockSize,
- Size chunkSize)
+ uint32 chunkSize)
{
int chunksPerBlock;
Size fullChunkSize;
@@ -352,7 +352,7 @@ SlabContextCreate(MemoryContext parent,
/* Make sure the block can store at least one chunk. */
if (chunksPerBlock == 0)
- elog(ERROR, "block size %zu for slab is too small for %zu-byte chunks",
+ elog(ERROR, "block size %zu for slab is too small for %u-byte chunks",
blockSize, chunkSize);
@@ -506,7 +506,7 @@ SlabAlloc(MemoryContext context, Size size)
/* make sure we only allow correct request size */
if (unlikely(size != slab->chunkSize))
- elog(ERROR, "unexpected alloc chunk size %zu (expected %zu)",
+ elog(ERROR, "unexpected alloc chunk size %zu (expected %u)",
size, slab->chunkSize);
/*
diff --git a/src/include/utils/memutils.h b/src/include/utils/memutils.h
index 21640d62a6..c0c3f102a5 100644
--- a/src/include/utils/memutils.h
+++ b/src/include/utils/memutils.h
@@ -111,9 +111,9 @@ extern void ProcessLogMemoryContextInterrupt(void);
/* aset.c */
extern MemoryContext AllocSetContextCreateInternal(MemoryContext parent,
const char *name,
- Size minContextSize,
- Size initBlockSize,
- Size maxBlockSize);
+ uint32 minContextSize,
+ uint32 initBlockSize,
+ uint32 maxBlockSize);
/*
* This wrapper macro exists to check for non-constant strings used as context
@@ -134,14 +134,14 @@ extern MemoryContext AllocSetContextCreateInternal(MemoryContext parent,
extern MemoryContext SlabContextCreate(MemoryContext parent,
const char *name,
Size blockSize,
- Size chunkSize);
+ uint32 chunkSize);
/* generation.c */
extern MemoryContext GenerationContextCreate(MemoryContext parent,
const char *name,
- Size minContextSize,
- Size initBlockSize,
- Size maxBlockSize);
+ uint32 minContextSize,
+ uint32 initBlockSize,
+ uint32 maxBlockSize);
/*
* Recommended default alloc parameters, suitable for "ordinary" contexts
--
2.25.1
In memory contexts, block and chunk sizes are likely to be limited by
some upper bounds. Some examples of those bounds can be
MEMORYCHUNK_MAX_BLOCKOFFSET and MEMORYCHUNK_MAX_VALUE. Both values are
only 1 less than 1GB.
This makes memory contexts to have blocks/chunks with sizes less than
1GB. Such sizes can be stored in 32-bits. Currently, "Size" type,
which is 64-bit, is used, but 32-bit integers should be enough to
store any value less than 1GB.
size_t (= Size) is the correct type in C to store the size of an object
in memory. This is partially a self-documentation issue: If I see
size_t in a function signature, I know what is intended; if I see
uint32, I have to wonder what the intent was.
You could make an argument that using shorter types would save space for
some internal structs, but then you'd have to show some more information
where and why that would be beneficial. (But again, self-documentation:
If one were to do that, I would argue for introducing a custom type like
pg_short_size_t.)
Absent any strong performance argument, I don't see the benefit of this
change. People might well want to experiment with MEMORYCHUNK_...
settings larger than 1GB.
On Wed, 28 Jun 2023 at 20:13, Peter Eisentraut <peter@eisentraut.org> wrote:
size_t (= Size) is the correct type in C to store the size of an object
in memory. This is partially a self-documentation issue: If I see
size_t in a function signature, I know what is intended; if I see
uint32, I have to wonder what the intent was.
Perhaps it's ok to leave the context creation functions with Size
typed parameters and then just Assert the passed-in sizes are not
larger than 1GB within the context creation function. That way we
could keep this change self contained in the .c file for the given
memory context. That would mean there's no less readability. If we
ever wanted to lift the 1GB limit on block sizes then we'd not need to
switch the function signature again. There's documentation where the
struct's field is declared, so having a smaller type in the struct
itself does not seem like a reduction of documentation quality.
You could make an argument that using shorter types would save space for
some internal structs, but then you'd have to show some more information
where and why that would be beneficial.
I think there's not much need to go proving this speeds something up.
There's just simply no point in the struct fields being changed in
Melih's patch to be bigger than 32 bits as we never need to store more
than 1GB in them. Reducing these down means we may have to touch
fewer cache lines and we'll also have more space on the keeper blocks
to store allocations. Memory allocation performance is fairly
fundamental to Postgres's performance. In my view, we shouldn't have
fields that are twice as large as they need to be in code as hot as
this.
Absent any strong performance argument, I don't see the benefit of this
change. People might well want to experiment with MEMORYCHUNK_...
settings larger than 1GB.
Anyone doing so will be editing C code anyway. They can adjust these
fields then.
David
David Rowley <dgrowleyml@gmail.com> writes:
Perhaps it's ok to leave the context creation functions with Size
typed parameters and then just Assert the passed-in sizes are not
larger than 1GB within the context creation function.
Yes, I'm strongly opposed to not using Size/size_t in the mmgr APIs.
If we go that road, we're going to have a problem when someone
inevitably wants to pass a larger-than-GB value for some context
type.
What happens in semi-private structs is a different matter, although
I'm a little dubious that shaving a couple of bytes from context
headers is a useful activity. The self-documentation argument
still has some force there, so I agree with Peter that some positive
benefit has to be shown.
regards, tom lane
On 6/28/23 12:59, Tom Lane wrote:
David Rowley <dgrowleyml@gmail.com> writes:
Perhaps it's ok to leave the context creation functions with Size
typed parameters and then just Assert the passed-in sizes are not
larger than 1GB within the context creation function.Yes, I'm strongly opposed to not using Size/size_t in the mmgr APIs.
If we go that road, we're going to have a problem when someone
inevitably wants to pass a larger-than-GB value for some context
type.
+1
What happens in semi-private structs is a different matter, although
I'm a little dubious that shaving a couple of bytes from context
headers is a useful activity. The self-documentation argument
still has some force there, so I agree with Peter that some positive
benefit has to be shown.
Yeah. FWIW I was interested what the patch does in practice, so I
checked what pahole says about impact on struct sizes:
AllocSetContext 224B -> 208B (4 cachelines)
GenerationContext 152B -> 136B (3 cachelines)
SlabContext 200B -> 200B (no change, adds 4B hole)
Nothing else changes, AFAICS. I find it hard to believe this could have
any sort of positive benefit - I doubt we ever have enough contexts for
this to matter.
When I first saw the patch I was thinking it's probably changing how we
store the per-chunk requested_size. Maybe that'd make a difference,
although 4B is tiny compared to what we waste due to the doubling.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
... 4B is tiny compared to what we waste due to the doubling.
Yeah. I've occasionally wondered if we should rethink aset.c's
"only power-of-2 chunk sizes" rule. Haven't had the bandwidth
to pursue the idea though.
regards, tom lane
Hi,
On 2023-06-28 23:26:00 +0200, Tomas Vondra wrote:
Yeah. FWIW I was interested what the patch does in practice, so I
checked what pahole says about impact on struct sizes:AllocSetContext 224B -> 208B (4 cachelines)
GenerationContext 152B -> 136B (3 cachelines)
SlabContext 200B -> 200B (no change, adds 4B hole)Nothing else changes, AFAICS. I find it hard to believe this could have
any sort of positive benefit - I doubt we ever have enough contexts for
this to matter.
I don't think it's that hard to believe. We create a lot of memory contexts
that we never or just barely use. Just reducing the number of cachelines
touched for that can't hurt. This does't quite get us to reducing the size to
a lower number of cachelines, but it's a good step.
There are a few other fields that we can get rid of.
- Afaics AllocSet->keeper is unnecessary these days, as it is always allocated
together with the context itself. Saves 8 bytes.
- The set of memory context types isn't runtime extensible. We could replace
MemoryContextData->methods with a small integer index into mcxt_methods. I
think that might actually end up being as-cheap or even cheaper than the
current approach. Saves 8 bytes.
Tthat's sufficient for going to 3 cachelines.
- We could store the power of 2 for initBlockSize, nextBlockSize,
maxBlockSize, instead of the "raw" value. That'd make them one byte
each. Which also would get rid of the concerns around needing a
"mini_size_t" type.
- freeListIndex could be a single byte as well (saving 7 bytes, as right now
we loose 4 trailing bytes due to padding).
That would save another 12 bytes, if I calculate correctly. 25% shrinkage
together ain't bad.
Greetings,
Andres Freund
Hi,
On 2023-06-28 17:56:55 -0400, Tom Lane wrote:
Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
... 4B is tiny compared to what we waste due to the doubling.
Yeah. I've occasionally wondered if we should rethink aset.c's
"only power-of-2 chunk sizes" rule. Haven't had the bandwidth
to pursue the idea though.
Me too. It'd not be trivial to do without also incurring performance overhead.
A somewhat easier thing we could try is to carve the "rounding up" space into
smaller chunks, similar to what we do for full blocks. It wouldn't make sense
to do that for the smaller size classes, but above 64-256 bytes or such, I
think the wins might be big enough to outweight the costs?
Of course that doesn't guarantee that that memory in those smaller size
classes is going to be used...
Greetings,
Andres Freund
On Thu, 29 Jun 2023 at 09:26, Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
AllocSetContext 224B -> 208B (4 cachelines)
GenerationContext 152B -> 136B (3 cachelines)
SlabContext 200B -> 200B (no change, adds 4B hole)Nothing else changes, AFAICS.
I don't think a lack of a reduction in the number of cache lines is
the important part. Allowing more space on the keeper block, which is
at the end of the context struct seems more useful. I understand that
the proposal is just to shave off 12 bytes and that's not exactly huge
when it's just once per context, but we do create quite a large number
of contexts with ALLOCSET_SMALL_SIZES which have a 1KB initial block
size. 12 bytes in 1024 is not terrible.
It's not exactly an invasive change. It does not add any complexity
to the code and as far as I can see, about zero risk of it slowing
anything down.
David
On 6/29/23 01:34, Andres Freund wrote:
Hi,
On 2023-06-28 23:26:00 +0200, Tomas Vondra wrote:
Yeah. FWIW I was interested what the patch does in practice, so I
checked what pahole says about impact on struct sizes:AllocSetContext 224B -> 208B (4 cachelines)
GenerationContext 152B -> 136B (3 cachelines)
SlabContext 200B -> 200B (no change, adds 4B hole)Nothing else changes, AFAICS. I find it hard to believe this could have
any sort of positive benefit - I doubt we ever have enough contexts for
this to matter.I don't think it's that hard to believe. We create a lot of memory contexts
that we never or just barely use. Just reducing the number of cachelines
touched for that can't hurt. This does't quite get us to reducing the size to
a lower number of cachelines, but it's a good step.There are a few other fields that we can get rid of.
- Afaics AllocSet->keeper is unnecessary these days, as it is always allocated
together with the context itself. Saves 8 bytes.- The set of memory context types isn't runtime extensible. We could replace
MemoryContextData->methods with a small integer index into mcxt_methods. I
think that might actually end up being as-cheap or even cheaper than the
current approach. Saves 8 bytes.Tthat's sufficient for going to 3 cachelines.
- We could store the power of 2 for initBlockSize, nextBlockSize,
maxBlockSize, instead of the "raw" value. That'd make them one byte
each. Which also would get rid of the concerns around needing a
"mini_size_t" type.- freeListIndex could be a single byte as well (saving 7 bytes, as right now
we loose 4 trailing bytes due to padding).That would save another 12 bytes, if I calculate correctly. 25% shrinkage
together ain't bad.
I don't oppose these changes, but I still don't quite believe it'll make
a measurable difference (even if we manage to save a cacheline or two).
I'd definitely like to see some measurements demonstrating it's worth
the extra complexity.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Hi,
On 2023-06-29 11:58:27 +0200, Tomas Vondra wrote:
On 6/29/23 01:34, Andres Freund wrote:
On 2023-06-28 23:26:00 +0200, Tomas Vondra wrote:
Yeah. FWIW I was interested what the patch does in practice, so I
checked what pahole says about impact on struct sizes:AllocSetContext 224B -> 208B (4 cachelines)
GenerationContext 152B -> 136B (3 cachelines)
SlabContext 200B -> 200B (no change, adds 4B hole)...
That would save another 12 bytes, if I calculate correctly. 25% shrinkage
together ain't bad.I don't oppose these changes, but I still don't quite believe it'll make
a measurable difference (even if we manage to save a cacheline or two).
I'd definitely like to see some measurements demonstrating it's worth
the extra complexity.
I hacked (emphasis on that) a version together that shrinks AllocSetContext
down to 176 bytes.
There seem to be some minor performance gains, and some not too shabby memory
savings.
E.g. a backend after running readonly pgbench goes from (results repeat
precisely across runs):
pgbench: Grand total: 1361528 bytes in 289 blocks; 367480 free (206 chunks); 994048 used
to:
pgbench: Grand total: 1339000 bytes in 278 blocks; 352352 free (188 chunks); 986648 used
Running a total over all connections in the main regression tests gives less
of a win (best of three):
backends grand blocks free chunks used
690 1046956664 111373 370680728 291436 676275936
to:
backends grand blocks free chunks used
690 1045226056 111099 372972120 297969 672253936
the latter is produced with this beauty:
ninja && m test --suite setup --no-rebuild && m test --no-rebuild --print-errorlogs regress/regress -v && grep "Grand total" testrun/regress/regress/log/postmaster.log|sed -E -e 's/.*Grand total: (.*) bytes in (.*) blocks; (.*) free \((.*) chunks\); (.*) used/\1\t\2\t\3\t\4\t\5/'|awk '{backends += 1; grand += $1; blocks += $2; free += $3; chunks += $4; used += $5} END{print backends, grand, blocks, free, chunks, used}'
There's more to get. The overhead of AllocSetBlock also plays into this. Both
due to the keeper block and obviously separate blocks getting allocated
subsequently. We e.g. don't need AllocBlockData->next,prev as 8 byte pointers
(some trickiness would be required for external blocks, but they could combine
both).
Greetings,
Andres Freund
Hi,
Thanks for your comments.
Tom Lane <tgl@sss.pgh.pa.us>, 28 Haz 2023 Çar, 13:59 tarihinde şunu yazdı:
David Rowley <dgrowleyml@gmail.com> writes:
Perhaps it's ok to leave the context creation functions with Size
typed parameters and then just Assert the passed-in sizes are not
larger than 1GB within the context creation function.Yes, I'm strongly opposed to not using Size/size_t in the mmgr APIs.
If we go that road, we're going to have a problem when someone
inevitably wants to pass a larger-than-GB value for some context
type.
I reverted changes in the context creation functions and only changed
the types in structs.
I believe there are already lines to assert whether the sizes are less
than 1GB, so we should be safe there.
Andres Freund <andres@anarazel.de>, 29 Haz 2023 Per, 02:34 tarihinde şunu yazdı:
There are a few other fields that we can get rid of.
- Afaics AllocSet->keeper is unnecessary these days, as it is always allocated
together with the context itself. Saves 8 bytes.
This seemed like a safe change and removed the keeper field in
AllocSet and Generation contexts. It saves an additional 8 bytes.
Best,
--
Melih Mutlu
Microsoft
Attachments:
v2-0001-Change-memory-context-fields-to-uint32.patchapplication/octet-stream; name=v2-0001-Change-memory-context-fields-to-uint32.patchDownload
From b9669a832576ca7fb941c727e6ff46adb3fdcdbf Mon Sep 17 00:00:00 2001
From: Melih Mutlu <m.melihmutlu@gmail.com>
Date: Mon, 12 Jun 2023 08:50:28 +0300
Subject: [PATCH v2] Change memory context fields to uint32
Block sizes and chunk sizes can be upper bounded by
MEMORYCHUNK_MAX_BLOCKOFFSET and MEMORYCHUNK_MAX_VALUE respectively.
Values of both bounds correspond to 1 less than 1GB. This allows us
to store the sizes of any block or chunk size limited by those bounds
in 32 bits.
This patch changes types of such fields that represents block or chunk sizes
from 64-bit Size to 32-bit unsigned integers.
Also; keeper fields from AllocSet and Generation structs are removed and
added AllocSetKeeper/GenerationKeeper macros where needed.
---
src/backend/utils/mmgr/aset.c | 52 +++++++++++++++++------------
src/backend/utils/mmgr/generation.c | 51 ++++++++++++++++------------
src/backend/utils/mmgr/slab.c | 6 ++--
3 files changed, 62 insertions(+), 47 deletions(-)
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index 0bbbf93672..797abef1b5 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -156,11 +156,10 @@ typedef struct AllocSetContext
AllocBlock blocks; /* head of list of blocks in this set */
MemoryChunk *freelist[ALLOCSET_NUM_FREELISTS]; /* free chunk lists */
/* Allocation parameters for this context: */
- Size initBlockSize; /* initial block size */
- Size maxBlockSize; /* maximum block size */
- Size nextBlockSize; /* next block size to allocate */
- Size allocChunkLimit; /* effective chunk size limit */
- AllocBlock keeper; /* keep this block over resets */
+ uint32 initBlockSize; /* initial block size */
+ uint32 maxBlockSize; /* maximum block size */
+ uint32 nextBlockSize; /* next block size to allocate */
+ uint32 allocChunkLimit; /* effective chunk size limit */
/* freelist this context could be put in, or -1 if not a candidate: */
int freeListIndex; /* index in context_freelists[], or -1 */
} AllocSetContext;
@@ -241,6 +240,14 @@ typedef struct AllocBlockData
*/
#define MAX_FREE_CONTEXTS 100 /* arbitrary limit on freelist length */
+/* Obtain the keeper block for an allocation set */
+#define AllocSetKeeper(set) \
+ ((AllocBlock) (((char *) set) + MAXALIGN(sizeof(AllocSetContext))))
+
+/* Check if the block is the keeper block of the given allocation set */
+#define IsKeeperBlock(set, block) \
+ ((block) == (AllocSetKeeper(set)))
+
typedef struct AllocSetFreeList
{
int num_free; /* current list length */
@@ -345,7 +352,7 @@ AllocSetContextCreateInternal(MemoryContext parent,
Size maxBlockSize)
{
int freeListIndex;
- Size firstBlockSize;
+ uint32 firstBlockSize;
AllocSet set;
AllocBlock block;
@@ -417,7 +424,7 @@ AllocSetContextCreateInternal(MemoryContext parent,
name);
((MemoryContext) set)->mem_allocated =
- set->keeper->endptr - ((char *) set);
+ AllocSetKeeper(set)->endptr - ((char *) set);
return (MemoryContext) set;
}
@@ -452,8 +459,11 @@ AllocSetContextCreateInternal(MemoryContext parent,
* we'd leak the header/initial block if we ereport in this stretch.
*/
- /* Fill in the initial block's block header */
- block = (AllocBlock) (((char *) set) + MAXALIGN(sizeof(AllocSetContext)));
+ /*
+ * Fill in the initial block's block header. The initial block is not to be
+ * released at reset time, considered as keeper block.
+ */
+ block = AllocSetKeeper(set);
block->aset = set;
block->freeptr = ((char *) block) + ALLOC_BLOCKHDRSZ;
block->endptr = ((char *) set) + firstBlockSize;
@@ -465,15 +475,13 @@ AllocSetContextCreateInternal(MemoryContext parent,
/* Remember block as part of block list */
set->blocks = block;
- /* Mark block as not to be released at reset time */
- set->keeper = block;
/* Finish filling in aset-specific parts of the context header */
MemSetAligned(set->freelist, 0, sizeof(set->freelist));
- set->initBlockSize = initBlockSize;
- set->maxBlockSize = maxBlockSize;
- set->nextBlockSize = initBlockSize;
+ set->initBlockSize = (Size) initBlockSize;
+ set->maxBlockSize = (Size) maxBlockSize;
+ set->nextBlockSize = (Size) initBlockSize;
set->freeListIndex = freeListIndex;
/*
@@ -544,7 +552,7 @@ AllocSetReset(MemoryContext context)
#endif
/* Remember keeper block size for Assert below */
- keepersize = set->keeper->endptr - ((char *) set);
+ keepersize = AllocSetKeeper(set)->endptr - ((char *) set);
/* Clear chunk freelists */
MemSetAligned(set->freelist, 0, sizeof(set->freelist));
@@ -552,13 +560,13 @@ AllocSetReset(MemoryContext context)
block = set->blocks;
/* New blocks list will be just the keeper block */
- set->blocks = set->keeper;
+ set->blocks = AllocSetKeeper(set);
while (block != NULL)
{
AllocBlock next = block->next;
- if (block == set->keeper)
+ if (IsKeeperBlock(set, block))
{
/* Reset the block, but don't return it to malloc */
char *datastart = ((char *) block) + ALLOC_BLOCKHDRSZ;
@@ -614,7 +622,7 @@ AllocSetDelete(MemoryContext context)
#endif
/* Remember keeper block size for Assert below */
- keepersize = set->keeper->endptr - ((char *) set);
+ keepersize = AllocSetKeeper(set)->endptr - ((char *) set);
/*
* If the context is a candidate for a freelist, put it into that freelist
@@ -663,14 +671,14 @@ AllocSetDelete(MemoryContext context)
{
AllocBlock next = block->next;
- if (block != set->keeper)
+ if (!IsKeeperBlock(set, block))
context->mem_allocated -= block->endptr - ((char *) block);
#ifdef CLOBBER_FREED_MEMORY
wipe_mem(block, block->freeptr - ((char *) block));
#endif
- if (block != set->keeper)
+ if (!IsKeeperBlock(set, block))
free(block);
block = next;
@@ -1547,7 +1555,7 @@ AllocSetCheck(MemoryContext context)
long nchunks = 0;
bool has_external_chunk = false;
- if (set->keeper == block)
+ if (IsKeeperBlock(set, block))
total_allocated += block->endptr - ((char *) set);
else
total_allocated += block->endptr - ((char *) block);
@@ -1557,7 +1565,7 @@ AllocSetCheck(MemoryContext context)
*/
if (!blk_used)
{
- if (set->keeper != block)
+ if (!IsKeeperBlock(set, block))
elog(WARNING, "problem in alloc set %s: empty block %p",
name, block);
}
diff --git a/src/backend/utils/mmgr/generation.c b/src/backend/utils/mmgr/generation.c
index 4fb8663cd6..d6a0811a72 100644
--- a/src/backend/utils/mmgr/generation.c
+++ b/src/backend/utils/mmgr/generation.c
@@ -61,17 +61,16 @@ typedef struct GenerationContext
MemoryContextData header; /* Standard memory-context fields */
/* Generational context parameters */
- Size initBlockSize; /* initial block size */
- Size maxBlockSize; /* maximum block size */
- Size nextBlockSize; /* next block size to allocate */
- Size allocChunkLimit; /* effective chunk size limit */
+ uint32 initBlockSize; /* initial block size */
+ uint32 maxBlockSize; /* maximum block size */
+ uint32 nextBlockSize; /* next block size to allocate */
+ uint32 allocChunkLimit; /* effective chunk size limit */
GenerationBlock *block; /* current (most recently allocated) block, or
* NULL if we've just freed the most recent
* block */
GenerationBlock *freeblock; /* pointer to a block that's being recycled,
* or NULL if there's no such block. */
- GenerationBlock *keeper; /* keep this block over resets */
dlist_head blocks; /* list of blocks */
} GenerationContext;
@@ -120,6 +119,14 @@ struct GenerationBlock
#define ExternalChunkGetBlock(chunk) \
(GenerationBlock *) ((char *) chunk - Generation_BLOCKHDRSZ)
+/* Obtain the keeper block for a generation context */
+#define GenerationKeeper(set) \
+ ((GenerationBlock *) (((char *) set) + MAXALIGN(sizeof(GenerationContext))))
+
+/* Check if the block is the keeper block of the given generation context */
+#define IsKeeperBlock(set, block) \
+ ((block) == (GenerationKeeper(set)))
+
/* Inlined helper functions */
static inline void GenerationBlockInit(GenerationContext *context,
GenerationBlock *block,
@@ -153,8 +160,8 @@ GenerationContextCreate(MemoryContext parent,
Size initBlockSize,
Size maxBlockSize)
{
- Size firstBlockSize;
- Size allocSize;
+ uint32 firstBlockSize;
+ uint32 allocSize;
GenerationContext *set;
GenerationBlock *block;
@@ -213,8 +220,11 @@ GenerationContextCreate(MemoryContext parent,
*/
dlist_init(&set->blocks);
- /* Fill in the initial block's block header */
- block = (GenerationBlock *) (((char *) set) + MAXALIGN(sizeof(GenerationContext)));
+ /*
+ * Fill in the initial block's block header. The initial block is not to be
+ * released at reset time, considered as keeper block.
+ */
+ block = GenerationKeeper(set);
/* determine the block size and initialize it */
firstBlockSize = allocSize - MAXALIGN(sizeof(GenerationContext));
GenerationBlockInit(set, block, firstBlockSize);
@@ -228,13 +238,10 @@ GenerationContextCreate(MemoryContext parent,
/* No free block, yet */
set->freeblock = NULL;
- /* 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;
+ set->initBlockSize = (Size) initBlockSize;
+ set->maxBlockSize = (Size) maxBlockSize;
+ set->nextBlockSize = (Size) initBlockSize;
/*
* Compute the allocation chunk size limit for this context.
@@ -294,14 +301,14 @@ GenerationReset(MemoryContext context)
{
GenerationBlock *block = dlist_container(GenerationBlock, node, miter.cur);
- if (block == set->keeper)
+ if (IsKeeperBlock(set, block))
GenerationBlockMarkEmpty(block);
else
GenerationBlockFree(set, block);
}
/* set it so new allocations to make use of the keeper block */
- set->block = set->keeper;
+ set->block = GenerationKeeper(set);
/* Reset block size allocation sequence, too */
set->nextBlockSize = set->initBlockSize;
@@ -440,10 +447,10 @@ GenerationAlloc(MemoryContext context, Size size)
*/
set->freeblock = NULL;
}
- else if (GenerationBlockIsEmpty(set->keeper) &&
- GenerationBlockFreeBytes(set->keeper) >= required_size)
+ else if (GenerationBlockIsEmpty(GenerationKeeper(set)) &&
+ GenerationBlockFreeBytes(GenerationKeeper(set)) >= required_size)
{
- block = set->keeper;
+ block = GenerationKeeper(set);
}
else
{
@@ -594,7 +601,7 @@ static inline void
GenerationBlockFree(GenerationContext *set, GenerationBlock *block)
{
/* Make sure nobody tries to free the keeper block */
- Assert(block != set->keeper);
+ Assert(!IsKeeperBlock(set, block));
/* We shouldn't be freeing the freeblock either */
Assert(block != set->freeblock);
@@ -691,7 +698,7 @@ GenerationFree(void *pointer)
set = block->context;
/* Don't try to free the keeper block, just mark it empty */
- if (block == set->keeper)
+ if (IsKeeperBlock(set, block))
{
GenerationBlockMarkEmpty(block);
return;
diff --git a/src/backend/utils/mmgr/slab.c b/src/backend/utils/mmgr/slab.c
index 718dd2ba03..d5e0edbede 100644
--- a/src/backend/utils/mmgr/slab.c
+++ b/src/backend/utils/mmgr/slab.c
@@ -104,7 +104,7 @@ typedef struct SlabContext
{
MemoryContextData header; /* Standard memory-context fields */
/* Allocation parameters for this context: */
- Size chunkSize; /* the requested (non-aligned) chunk size */
+ uint32 chunkSize; /* the requested (non-aligned) chunk size */
Size fullChunkSize; /* chunk size with chunk header and alignment */
Size blockSize; /* the size to make each block of chunks */
int32 chunksPerBlock; /* number of chunks that fit in 1 block */
@@ -374,7 +374,7 @@ SlabContextCreate(MemoryContext parent,
*/
/* Fill in SlabContext-specific header fields */
- slab->chunkSize = chunkSize;
+ slab->chunkSize = (Size) chunkSize;
slab->fullChunkSize = fullChunkSize;
slab->blockSize = blockSize;
slab->chunksPerBlock = chunksPerBlock;
@@ -506,7 +506,7 @@ SlabAlloc(MemoryContext context, Size size)
/* make sure we only allow correct request size */
if (unlikely(size != slab->chunkSize))
- elog(ERROR, "unexpected alloc chunk size %zu (expected %zu)",
+ elog(ERROR, "unexpected alloc chunk size %zu (expected %u)",
size, slab->chunkSize);
/*
--
2.25.1
On Tue, 11 Jul 2023 at 02:41, Melih Mutlu <m.melihmutlu@gmail.com> wrote:
- Afaics AllocSet->keeper is unnecessary these days, as it is always allocated
together with the context itself. Saves 8 bytes.This seemed like a safe change and removed the keeper field in
AllocSet and Generation contexts. It saves an additional 8 bytes.
Seems like a good idea for an additional 8-bytes.
I looked at your v2 patch. The only thing that really looked wrong
were the (Size) casts in the context creation functions. These should
have been casts to uint32 rather than Size. Basically, all the casts
do is say to the compiler "Yes, I know this could cause truncation due
to assigning to a size smaller than the source type's size". Some
compilers will likely warn without that and the cast will stop them.
We know there can't be any truncation due to the Asserts. There's also
the fundamental limitation that MemoryChunk can't store block offsets
larger than 1GBs anyway, so things will go bad if we tried to have
blocks bigger than 1GB.
Aside from that, I thought that a couple of other slab.c fields could
be shrunken to uint32 as the v2 patch just reduces the size of 1 field
which just creates a 4-byte hole in SlabContext. The fullChunkSize
field is just the MAXALIGN(chunkSize) + sizeof(MemoryChunk). We
should never be using slab contexts for any chunks anywhere near that
size. aset.c would be a better context for that, so it seems fine to
me to further restrict the maximum supported chunk size by another 8
bytes.
I've attached your patch again along with a small delta of what I adjusted.
My thoughts on these changes are that it's senseless to have Size
typed fields for storing a value that's never larger than 2^30.
Getting rid of the keeper pointer seems like a cleanup as it's pretty
much a redundant field. For small sized contexts like the ones used
for storing index relcache entries, I think it makes sense to save 20
more bytes. Each backend can have many thousand of those and there
could be many hundred backends. If we can fit more allocations on that
initial 1 kilobyte keeper block without having to allocate any
additional blocks, then that's great.
I feel that Andres's results showing several hundred fewer block
allocations shows this working. Albeit, his patch reduced the size of
the structs even further than what v3 does. I think v3 is enough for
now as the additional changes Andres mentioned require some more
invasive code changes to make work.
If nobody objects or has other ideas about this, modulo commit
message, I plan to push the attached on Monday.
David
Attachments:
v3-0001-Change-memory-context-fields-to-uint32.patchtext/plain; charset=US-ASCII; name=v3-0001-Change-memory-context-fields-to-uint32.patchDownload
From e5e7da6a9880b0cbc7fa96d26cdd73d8bb02c88f Mon Sep 17 00:00:00 2001
From: Melih Mutlu <m.melihmutlu@gmail.com>
Date: Mon, 12 Jun 2023 08:50:28 +0300
Subject: [PATCH v3 1/2] Change memory context fields to uint32
Block sizes and chunk sizes can be upper bounded by
MEMORYCHUNK_MAX_BLOCKOFFSET and MEMORYCHUNK_MAX_VALUE respectively.
Values of both bounds correspond to 1 less than 1GB. This allows us
to store the sizes of any block or chunk size limited by those bounds
in 32 bits.
This patch changes types of such fields that represents block or chunk sizes
from 64-bit Size to 32-bit unsigned integers.
Also; keeper fields from AllocSet and Generation structs are removed and
added AllocSetKeeper/GenerationKeeper macros where needed.
---
src/backend/utils/mmgr/aset.c | 52 +++++++++++++++++------------
src/backend/utils/mmgr/generation.c | 51 ++++++++++++++++------------
src/backend/utils/mmgr/slab.c | 6 ++--
3 files changed, 62 insertions(+), 47 deletions(-)
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index 0bbbf93672..797abef1b5 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -156,11 +156,10 @@ typedef struct AllocSetContext
AllocBlock blocks; /* head of list of blocks in this set */
MemoryChunk *freelist[ALLOCSET_NUM_FREELISTS]; /* free chunk lists */
/* Allocation parameters for this context: */
- Size initBlockSize; /* initial block size */
- Size maxBlockSize; /* maximum block size */
- Size nextBlockSize; /* next block size to allocate */
- Size allocChunkLimit; /* effective chunk size limit */
- AllocBlock keeper; /* keep this block over resets */
+ uint32 initBlockSize; /* initial block size */
+ uint32 maxBlockSize; /* maximum block size */
+ uint32 nextBlockSize; /* next block size to allocate */
+ uint32 allocChunkLimit; /* effective chunk size limit */
/* freelist this context could be put in, or -1 if not a candidate: */
int freeListIndex; /* index in context_freelists[], or -1 */
} AllocSetContext;
@@ -241,6 +240,14 @@ typedef struct AllocBlockData
*/
#define MAX_FREE_CONTEXTS 100 /* arbitrary limit on freelist length */
+/* Obtain the keeper block for an allocation set */
+#define AllocSetKeeper(set) \
+ ((AllocBlock) (((char *) set) + MAXALIGN(sizeof(AllocSetContext))))
+
+/* Check if the block is the keeper block of the given allocation set */
+#define IsKeeperBlock(set, block) \
+ ((block) == (AllocSetKeeper(set)))
+
typedef struct AllocSetFreeList
{
int num_free; /* current list length */
@@ -345,7 +352,7 @@ AllocSetContextCreateInternal(MemoryContext parent,
Size maxBlockSize)
{
int freeListIndex;
- Size firstBlockSize;
+ uint32 firstBlockSize;
AllocSet set;
AllocBlock block;
@@ -417,7 +424,7 @@ AllocSetContextCreateInternal(MemoryContext parent,
name);
((MemoryContext) set)->mem_allocated =
- set->keeper->endptr - ((char *) set);
+ AllocSetKeeper(set)->endptr - ((char *) set);
return (MemoryContext) set;
}
@@ -452,8 +459,11 @@ AllocSetContextCreateInternal(MemoryContext parent,
* we'd leak the header/initial block if we ereport in this stretch.
*/
- /* Fill in the initial block's block header */
- block = (AllocBlock) (((char *) set) + MAXALIGN(sizeof(AllocSetContext)));
+ /*
+ * Fill in the initial block's block header. The initial block is not to be
+ * released at reset time, considered as keeper block.
+ */
+ block = AllocSetKeeper(set);
block->aset = set;
block->freeptr = ((char *) block) + ALLOC_BLOCKHDRSZ;
block->endptr = ((char *) set) + firstBlockSize;
@@ -465,15 +475,13 @@ AllocSetContextCreateInternal(MemoryContext parent,
/* Remember block as part of block list */
set->blocks = block;
- /* Mark block as not to be released at reset time */
- set->keeper = block;
/* Finish filling in aset-specific parts of the context header */
MemSetAligned(set->freelist, 0, sizeof(set->freelist));
- set->initBlockSize = initBlockSize;
- set->maxBlockSize = maxBlockSize;
- set->nextBlockSize = initBlockSize;
+ set->initBlockSize = (Size) initBlockSize;
+ set->maxBlockSize = (Size) maxBlockSize;
+ set->nextBlockSize = (Size) initBlockSize;
set->freeListIndex = freeListIndex;
/*
@@ -544,7 +552,7 @@ AllocSetReset(MemoryContext context)
#endif
/* Remember keeper block size for Assert below */
- keepersize = set->keeper->endptr - ((char *) set);
+ keepersize = AllocSetKeeper(set)->endptr - ((char *) set);
/* Clear chunk freelists */
MemSetAligned(set->freelist, 0, sizeof(set->freelist));
@@ -552,13 +560,13 @@ AllocSetReset(MemoryContext context)
block = set->blocks;
/* New blocks list will be just the keeper block */
- set->blocks = set->keeper;
+ set->blocks = AllocSetKeeper(set);
while (block != NULL)
{
AllocBlock next = block->next;
- if (block == set->keeper)
+ if (IsKeeperBlock(set, block))
{
/* Reset the block, but don't return it to malloc */
char *datastart = ((char *) block) + ALLOC_BLOCKHDRSZ;
@@ -614,7 +622,7 @@ AllocSetDelete(MemoryContext context)
#endif
/* Remember keeper block size for Assert below */
- keepersize = set->keeper->endptr - ((char *) set);
+ keepersize = AllocSetKeeper(set)->endptr - ((char *) set);
/*
* If the context is a candidate for a freelist, put it into that freelist
@@ -663,14 +671,14 @@ AllocSetDelete(MemoryContext context)
{
AllocBlock next = block->next;
- if (block != set->keeper)
+ if (!IsKeeperBlock(set, block))
context->mem_allocated -= block->endptr - ((char *) block);
#ifdef CLOBBER_FREED_MEMORY
wipe_mem(block, block->freeptr - ((char *) block));
#endif
- if (block != set->keeper)
+ if (!IsKeeperBlock(set, block))
free(block);
block = next;
@@ -1547,7 +1555,7 @@ AllocSetCheck(MemoryContext context)
long nchunks = 0;
bool has_external_chunk = false;
- if (set->keeper == block)
+ if (IsKeeperBlock(set, block))
total_allocated += block->endptr - ((char *) set);
else
total_allocated += block->endptr - ((char *) block);
@@ -1557,7 +1565,7 @@ AllocSetCheck(MemoryContext context)
*/
if (!blk_used)
{
- if (set->keeper != block)
+ if (!IsKeeperBlock(set, block))
elog(WARNING, "problem in alloc set %s: empty block %p",
name, block);
}
diff --git a/src/backend/utils/mmgr/generation.c b/src/backend/utils/mmgr/generation.c
index 4fb8663cd6..d6a0811a72 100644
--- a/src/backend/utils/mmgr/generation.c
+++ b/src/backend/utils/mmgr/generation.c
@@ -61,17 +61,16 @@ typedef struct GenerationContext
MemoryContextData header; /* Standard memory-context fields */
/* Generational context parameters */
- Size initBlockSize; /* initial block size */
- Size maxBlockSize; /* maximum block size */
- Size nextBlockSize; /* next block size to allocate */
- Size allocChunkLimit; /* effective chunk size limit */
+ uint32 initBlockSize; /* initial block size */
+ uint32 maxBlockSize; /* maximum block size */
+ uint32 nextBlockSize; /* next block size to allocate */
+ uint32 allocChunkLimit; /* effective chunk size limit */
GenerationBlock *block; /* current (most recently allocated) block, or
* NULL if we've just freed the most recent
* block */
GenerationBlock *freeblock; /* pointer to a block that's being recycled,
* or NULL if there's no such block. */
- GenerationBlock *keeper; /* keep this block over resets */
dlist_head blocks; /* list of blocks */
} GenerationContext;
@@ -120,6 +119,14 @@ struct GenerationBlock
#define ExternalChunkGetBlock(chunk) \
(GenerationBlock *) ((char *) chunk - Generation_BLOCKHDRSZ)
+/* Obtain the keeper block for a generation context */
+#define GenerationKeeper(set) \
+ ((GenerationBlock *) (((char *) set) + MAXALIGN(sizeof(GenerationContext))))
+
+/* Check if the block is the keeper block of the given generation context */
+#define IsKeeperBlock(set, block) \
+ ((block) == (GenerationKeeper(set)))
+
/* Inlined helper functions */
static inline void GenerationBlockInit(GenerationContext *context,
GenerationBlock *block,
@@ -153,8 +160,8 @@ GenerationContextCreate(MemoryContext parent,
Size initBlockSize,
Size maxBlockSize)
{
- Size firstBlockSize;
- Size allocSize;
+ uint32 firstBlockSize;
+ uint32 allocSize;
GenerationContext *set;
GenerationBlock *block;
@@ -213,8 +220,11 @@ GenerationContextCreate(MemoryContext parent,
*/
dlist_init(&set->blocks);
- /* Fill in the initial block's block header */
- block = (GenerationBlock *) (((char *) set) + MAXALIGN(sizeof(GenerationContext)));
+ /*
+ * Fill in the initial block's block header. The initial block is not to be
+ * released at reset time, considered as keeper block.
+ */
+ block = GenerationKeeper(set);
/* determine the block size and initialize it */
firstBlockSize = allocSize - MAXALIGN(sizeof(GenerationContext));
GenerationBlockInit(set, block, firstBlockSize);
@@ -228,13 +238,10 @@ GenerationContextCreate(MemoryContext parent,
/* No free block, yet */
set->freeblock = NULL;
- /* 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;
+ set->initBlockSize = (Size) initBlockSize;
+ set->maxBlockSize = (Size) maxBlockSize;
+ set->nextBlockSize = (Size) initBlockSize;
/*
* Compute the allocation chunk size limit for this context.
@@ -294,14 +301,14 @@ GenerationReset(MemoryContext context)
{
GenerationBlock *block = dlist_container(GenerationBlock, node, miter.cur);
- if (block == set->keeper)
+ if (IsKeeperBlock(set, block))
GenerationBlockMarkEmpty(block);
else
GenerationBlockFree(set, block);
}
/* set it so new allocations to make use of the keeper block */
- set->block = set->keeper;
+ set->block = GenerationKeeper(set);
/* Reset block size allocation sequence, too */
set->nextBlockSize = set->initBlockSize;
@@ -440,10 +447,10 @@ GenerationAlloc(MemoryContext context, Size size)
*/
set->freeblock = NULL;
}
- else if (GenerationBlockIsEmpty(set->keeper) &&
- GenerationBlockFreeBytes(set->keeper) >= required_size)
+ else if (GenerationBlockIsEmpty(GenerationKeeper(set)) &&
+ GenerationBlockFreeBytes(GenerationKeeper(set)) >= required_size)
{
- block = set->keeper;
+ block = GenerationKeeper(set);
}
else
{
@@ -594,7 +601,7 @@ static inline void
GenerationBlockFree(GenerationContext *set, GenerationBlock *block)
{
/* Make sure nobody tries to free the keeper block */
- Assert(block != set->keeper);
+ Assert(!IsKeeperBlock(set, block));
/* We shouldn't be freeing the freeblock either */
Assert(block != set->freeblock);
@@ -691,7 +698,7 @@ GenerationFree(void *pointer)
set = block->context;
/* Don't try to free the keeper block, just mark it empty */
- if (block == set->keeper)
+ if (IsKeeperBlock(set, block))
{
GenerationBlockMarkEmpty(block);
return;
diff --git a/src/backend/utils/mmgr/slab.c b/src/backend/utils/mmgr/slab.c
index 718dd2ba03..d5e0edbede 100644
--- a/src/backend/utils/mmgr/slab.c
+++ b/src/backend/utils/mmgr/slab.c
@@ -104,7 +104,7 @@ typedef struct SlabContext
{
MemoryContextData header; /* Standard memory-context fields */
/* Allocation parameters for this context: */
- Size chunkSize; /* the requested (non-aligned) chunk size */
+ uint32 chunkSize; /* the requested (non-aligned) chunk size */
Size fullChunkSize; /* chunk size with chunk header and alignment */
Size blockSize; /* the size to make each block of chunks */
int32 chunksPerBlock; /* number of chunks that fit in 1 block */
@@ -374,7 +374,7 @@ SlabContextCreate(MemoryContext parent,
*/
/* Fill in SlabContext-specific header fields */
- slab->chunkSize = chunkSize;
+ slab->chunkSize = (Size) chunkSize;
slab->fullChunkSize = fullChunkSize;
slab->blockSize = blockSize;
slab->chunksPerBlock = chunksPerBlock;
@@ -506,7 +506,7 @@ SlabAlloc(MemoryContext context, Size size)
/* make sure we only allow correct request size */
if (unlikely(size != slab->chunkSize))
- elog(ERROR, "unexpected alloc chunk size %zu (expected %zu)",
+ elog(ERROR, "unexpected alloc chunk size %zu (expected %u)",
size, slab->chunkSize);
/*
--
2.39.2
v3-0002-fixup-Change-memory-context-fields-to-uint32.patchtext/plain; charset=US-ASCII; name=v3-0002-fixup-Change-memory-context-fields-to-uint32.patchDownload
From 81825c3d3f7d12b5f5d68f3e132e5284f823e393 Mon Sep 17 00:00:00 2001
From: David Rowley <dgrowley@gmail.com>
Date: Thu, 13 Jul 2023 16:41:29 +1200
Subject: [PATCH v3 2/2] fixup! Change memory context fields to uint32
---
src/backend/utils/mmgr/aset.c | 25 ++++++++++++-------------
src/backend/utils/mmgr/generation.c | 29 ++++++++++++++---------------
src/backend/utils/mmgr/slab.c | 18 +++++++++++-------
3 files changed, 37 insertions(+), 35 deletions(-)
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index 797abef1b5..9b556396ee 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -241,12 +241,11 @@ typedef struct AllocBlockData
#define MAX_FREE_CONTEXTS 100 /* arbitrary limit on freelist length */
/* Obtain the keeper block for an allocation set */
-#define AllocSetKeeper(set) \
+#define KeeperBlock(set) \
((AllocBlock) (((char *) set) + MAXALIGN(sizeof(AllocSetContext))))
/* Check if the block is the keeper block of the given allocation set */
-#define IsKeeperBlock(set, block) \
- ((block) == (AllocSetKeeper(set)))
+#define IsKeeperBlock(set, block) ((block) == (KeeperBlock(set)))
typedef struct AllocSetFreeList
{
@@ -424,7 +423,7 @@ AllocSetContextCreateInternal(MemoryContext parent,
name);
((MemoryContext) set)->mem_allocated =
- AllocSetKeeper(set)->endptr - ((char *) set);
+ KeeperBlock(set)->endptr - ((char *) set);
return (MemoryContext) set;
}
@@ -460,10 +459,10 @@ AllocSetContextCreateInternal(MemoryContext parent,
*/
/*
- * Fill in the initial block's block header. The initial block is not to be
- * released at reset time, considered as keeper block.
+ * Fill in the initial block's block header. The initial block is not to
+ * be released at reset time, considered as keeper block.
*/
- block = AllocSetKeeper(set);
+ block = KeeperBlock(set);
block->aset = set;
block->freeptr = ((char *) block) + ALLOC_BLOCKHDRSZ;
block->endptr = ((char *) set) + firstBlockSize;
@@ -479,9 +478,9 @@ AllocSetContextCreateInternal(MemoryContext parent,
/* Finish filling in aset-specific parts of the context header */
MemSetAligned(set->freelist, 0, sizeof(set->freelist));
- set->initBlockSize = (Size) initBlockSize;
- set->maxBlockSize = (Size) maxBlockSize;
- set->nextBlockSize = (Size) initBlockSize;
+ set->initBlockSize = (uint32) initBlockSize;
+ set->maxBlockSize = (uint32) maxBlockSize;
+ set->nextBlockSize = (uint32) initBlockSize;
set->freeListIndex = freeListIndex;
/*
@@ -552,7 +551,7 @@ AllocSetReset(MemoryContext context)
#endif
/* Remember keeper block size for Assert below */
- keepersize = AllocSetKeeper(set)->endptr - ((char *) set);
+ keepersize = KeeperBlock(set)->endptr - ((char *) set);
/* Clear chunk freelists */
MemSetAligned(set->freelist, 0, sizeof(set->freelist));
@@ -560,7 +559,7 @@ AllocSetReset(MemoryContext context)
block = set->blocks;
/* New blocks list will be just the keeper block */
- set->blocks = AllocSetKeeper(set);
+ set->blocks = KeeperBlock(set);
while (block != NULL)
{
@@ -622,7 +621,7 @@ AllocSetDelete(MemoryContext context)
#endif
/* Remember keeper block size for Assert below */
- keepersize = AllocSetKeeper(set)->endptr - ((char *) set);
+ keepersize = KeeperBlock(set)->endptr - ((char *) set);
/*
* If the context is a candidate for a freelist, put it into that freelist
diff --git a/src/backend/utils/mmgr/generation.c b/src/backend/utils/mmgr/generation.c
index d6a0811a72..a152fbf0c5 100644
--- a/src/backend/utils/mmgr/generation.c
+++ b/src/backend/utils/mmgr/generation.c
@@ -120,12 +120,11 @@ struct GenerationBlock
(GenerationBlock *) ((char *) chunk - Generation_BLOCKHDRSZ)
/* Obtain the keeper block for a generation context */
-#define GenerationKeeper(set) \
+#define KeeperBlock(set) \
((GenerationBlock *) (((char *) set) + MAXALIGN(sizeof(GenerationContext))))
/* Check if the block is the keeper block of the given generation context */
-#define IsKeeperBlock(set, block) \
- ((block) == (GenerationKeeper(set)))
+#define IsKeeperBlock(set, block) ((block) == (KeeperBlock(set)))
/* Inlined helper functions */
static inline void GenerationBlockInit(GenerationContext *context,
@@ -160,8 +159,8 @@ GenerationContextCreate(MemoryContext parent,
Size initBlockSize,
Size maxBlockSize)
{
- uint32 firstBlockSize;
- uint32 allocSize;
+ Size firstBlockSize;
+ Size allocSize;
GenerationContext *set;
GenerationBlock *block;
@@ -221,10 +220,10 @@ GenerationContextCreate(MemoryContext parent,
dlist_init(&set->blocks);
/*
- * Fill in the initial block's block header. The initial block is not to be
- * released at reset time, considered as keeper block.
+ * Fill in the initial block's block header. The initial block is not to
+ * be released at reset time, considered as keeper block.
*/
- block = GenerationKeeper(set);
+ block = KeeperBlock(set);
/* determine the block size and initialize it */
firstBlockSize = allocSize - MAXALIGN(sizeof(GenerationContext));
GenerationBlockInit(set, block, firstBlockSize);
@@ -239,9 +238,9 @@ GenerationContextCreate(MemoryContext parent,
set->freeblock = NULL;
/* Fill in GenerationContext-specific header fields */
- set->initBlockSize = (Size) initBlockSize;
- set->maxBlockSize = (Size) maxBlockSize;
- set->nextBlockSize = (Size) initBlockSize;
+ set->initBlockSize = (uint32) initBlockSize;
+ set->maxBlockSize = (uint32) maxBlockSize;
+ set->nextBlockSize = (uint32) initBlockSize;
/*
* Compute the allocation chunk size limit for this context.
@@ -308,7 +307,7 @@ GenerationReset(MemoryContext context)
}
/* set it so new allocations to make use of the keeper block */
- set->block = GenerationKeeper(set);
+ set->block = KeeperBlock(set);
/* Reset block size allocation sequence, too */
set->nextBlockSize = set->initBlockSize;
@@ -447,10 +446,10 @@ GenerationAlloc(MemoryContext context, Size size)
*/
set->freeblock = NULL;
}
- else if (GenerationBlockIsEmpty(GenerationKeeper(set)) &&
- GenerationBlockFreeBytes(GenerationKeeper(set)) >= required_size)
+ else if (GenerationBlockIsEmpty(KeeperBlock(set)) &&
+ GenerationBlockFreeBytes(KeeperBlock(set)) >= required_size)
{
- block = GenerationKeeper(set);
+ block = KeeperBlock(set);
}
else
{
diff --git a/src/backend/utils/mmgr/slab.c b/src/backend/utils/mmgr/slab.c
index d5e0edbede..40c1d401c4 100644
--- a/src/backend/utils/mmgr/slab.c
+++ b/src/backend/utils/mmgr/slab.c
@@ -105,8 +105,8 @@ typedef struct SlabContext
MemoryContextData header; /* Standard memory-context fields */
/* Allocation parameters for this context: */
uint32 chunkSize; /* the requested (non-aligned) chunk size */
- Size fullChunkSize; /* chunk size with chunk header and alignment */
- Size blockSize; /* the size to make each block of chunks */
+ uint32 fullChunkSize; /* chunk size with chunk header and alignment */
+ uint32 blockSize; /* the size to make each block of chunks */
int32 chunksPerBlock; /* number of chunks that fit in 1 block */
int32 curBlocklistIndex; /* index into the blocklist[] element
* containing the fullest, blocks */
@@ -314,7 +314,9 @@ SlabGetNextFreeChunk(SlabContext *slab, SlabBlock *block)
* blockSize: allocation block size
* chunkSize: allocation chunk size
*
- * The MAXALIGN(chunkSize) may not exceed MEMORYCHUNK_MAX_VALUE
+ * The Slab_CHUNKHDRSZ + MAXALIGN(chunkSize + 1) may not exceed
+ * MEMORYCHUNK_MAX_VALUE.
+ * 'blockSize' may not exceed MEMORYCHUNK_MAX_BLOCKOFFSET.
*/
MemoryContext
SlabContextCreate(MemoryContext parent,
@@ -330,7 +332,7 @@ SlabContextCreate(MemoryContext parent,
/* ensure MemoryChunk's size is properly maxaligned */
StaticAssertDecl(Slab_CHUNKHDRSZ == MAXALIGN(Slab_CHUNKHDRSZ),
"sizeof(MemoryChunk) is not maxaligned");
- Assert(MAXALIGN(chunkSize) <= MEMORYCHUNK_MAX_VALUE);
+ Assert(blockSize <= MEMORYCHUNK_MAX_BLOCKOFFSET);
/*
* Ensure there's enough space to store the pointer to the next free chunk
@@ -347,6 +349,8 @@ SlabContextCreate(MemoryContext parent,
fullChunkSize = Slab_CHUNKHDRSZ + MAXALIGN(chunkSize);
#endif
+ Assert(fullChunkSize <= MEMORYCHUNK_MAX_VALUE);
+
/* compute the number of chunks that will fit on each block */
chunksPerBlock = (blockSize - Slab_BLOCKHDRSZ) / fullChunkSize;
@@ -374,9 +378,9 @@ SlabContextCreate(MemoryContext parent,
*/
/* Fill in SlabContext-specific header fields */
- slab->chunkSize = (Size) chunkSize;
- slab->fullChunkSize = fullChunkSize;
- slab->blockSize = blockSize;
+ slab->chunkSize = (uint32) chunkSize;
+ slab->fullChunkSize = (uint32) fullChunkSize;
+ slab->blockSize = (uint32) blockSize;
slab->chunksPerBlock = chunksPerBlock;
slab->curBlocklistIndex = 0;
--
2.39.2
Hi David,
David Rowley <dgrowleyml@gmail.com>, 13 Tem 2023 Per, 08:04 tarihinde şunu
yazdı:
I looked at your v2 patch. The only thing that really looked wrong
were the (Size) casts in the context creation functions. These should
have been casts to uint32 rather than Size. Basically, all the casts
do is say to the compiler "Yes, I know this could cause truncation due
to assigning to a size smaller than the source type's size". Some
compilers will likely warn without that and the cast will stop them.
We know there can't be any truncation due to the Asserts. There's also
the fundamental limitation that MemoryChunk can't store block offsets
larger than 1GBs anyway, so things will go bad if we tried to have
blocks bigger than 1GB.
Right! I don't know why I cast them to Size. Thanks for the fix.
Best,
--
Melih Mutlu
Microsoft
On Fri, 14 Jul 2023 at 18:53, Melih Mutlu <m.melihmutlu@gmail.com> wrote:
David Rowley <dgrowleyml@gmail.com>, 13 Tem 2023 Per, 08:04 tarihinde şunu yazdı:
I looked at your v2 patch. The only thing that really looked wrong
were the (Size) casts in the context creation functions. These should
have been casts to uint32 rather than Size. Basically, all the casts
do is say to the compiler "Yes, I know this could cause truncation due
to assigning to a size smaller than the source type's size". Some
compilers will likely warn without that and the cast will stop them.
We know there can't be any truncation due to the Asserts. There's also
the fundamental limitation that MemoryChunk can't store block offsets
larger than 1GBs anyway, so things will go bad if we tried to have
blocks bigger than 1GB.Right! I don't know why I cast them to Size. Thanks for the fix.
Pushed.
David