On Mon, 6 Nov 2023 at 19:54, Matthias van de Meent <boekewurm+postg...@gmail.com> wrote: > > On Tue, 11 Jul 2023 at 01:51, David Rowley <dgrowle...@gmail.com> wrote: >> >> On Tue, 27 Jun 2023 at 21:19, David Rowley <dgrowle...@gmail.com> wrote: >>> I've attached the bump allocator patch and also the script I used to >>> gather the performance results in the first 2 tabs in the attached >>> spreadsheet. >> >> I've attached a v2 patch which changes the BumpContext a little to >> remove some of the fields that are not really required. There was no >> need for the "keeper" field as the keeper block always comes at the >> end of the BumpContext as these are allocated in a single malloc(). >> The pointer to the "block" also isn't really needed. This is always >> the same as the head element in the blocks dlist.
>> +++ b/src/backend/utils/mmgr/bump.c >> [...] >> +MemoryContext >> +BumpContextCreate(MemoryContext parent, >> + const char *name, >> + Size minContextSize, >> + Size initBlockSize, >> + Size maxBlockSize) >> [...] >> + /* Determine size of initial block */ >> + allocSize = MAXALIGN(sizeof(BumpContext)) + Bump_BLOCKHDRSZ + >> + if (minContextSize != 0) >> + allocSize = Max(allocSize, minContextSize); >> + else >> + allocSize = Max(allocSize, initBlockSize); Shouldn't this be the following, considering the meaning of "initBlockSize"? + allocSize = MAXALIGN(sizeof(BumpContext)) + Bump_BLOCKHDRSZ + + Bump_CHUNKHDRSZ + initBlockSize; + if (minContextSize != 0) + allocSize = Max(allocSize, minContextSize); >> + * BumpFree >> + * Unsupported. >> [...] >> + * BumpRealloc >> + * Unsupported. Rather than the error, can't we make this a no-op (potentially optionally, or in a different memory context?) What I mean is, I get that it is an easy validator check that the code that uses this context doesn't accidentally leak memory through assumptions about pfree, but this does make this memory context unusable for more generic operations where leaking a little memory is preferred over the overhead of other memory contexts, as MemoryContextReset is quite cheap in the grand scheme of things. E.g. using a bump context in btree descent could speed up queries when we use compare operators that do allocate memory (e.g. numeric, text), because btree operators must not leak memory and thus always have to manually keep track of all allocations, which can be expensive. I understand that allowing pfree/repalloc in bump contexts requires each allocation to have a MemoryChunk prefix in overhead, but I think it's still a valid use case to have a very low overhead allocator with no-op deallocator (except context reset). Do you have performance comparison results between with and without the overhead of MemoryChunk? Kind regards, Matthias van de Meent Neon (https://neon.tech)
