I've rebased the 0001 patch and gone over it again and made a few
additional changes besides what I mentioned in my review.
On Wed, 9 Aug 2023 at 20:44, David Rowley <dgrowleyml@gmail.com> wrote:
> Here's a review of v2-0001:
> 2. Why do you need to add the NULL check here?
>
> #ifdef USE_VALGRIND
> - if (method != MCTX_ALIGNED_REDIRECT_ID)
> + if (ret != NULL && method != MCTX_ALIGNED_REDIRECT_ID)
> VALGRIND_MEMPOOL_CHANGE(context, pointer, ret, size);
> #endif
I removed this NULL check as we're calling the realloc function with
no flags, so it shouldn't return NULL as it'll error out from any OOM
errors.
> 3.
>
> /*
> * XXX: Probably no need to check for huge allocations, we only support
> * one size? Which could theoretically be huge, but that'd not make
> * sense...
> */
>
> They can't be huge per Assert(fullChunkSize <= MEMORYCHUNK_MAX_VALUE)
> in SlabContextCreate().
I removed this comment and adjusted the comment just below that which
checks the 'size' matches the expected slab chunk size. i.e.
/*
* Make sure we only allow correct request size. This doubles as the
* MemoryContextCheckSize check.
*/
if (unlikely(size != slab->chunkSize))
> 4. It would be good to see some API documentation in the
> MemoryContextMethods struct. This adds a lot of responsibility onto
> the context implementation without any extra documentation to explain
> what, for example, palloc is responsible for and what the alloc
> function needs to do itself.
I've done that too.
I also added header comments for MemoryContextAllocationFailure and
MemoryContextSizeFailure and added some comments to explain in places
like palloc() to warn people not to add checks after the 'alloc' call.
The rebased patch is 0001 and all of my changes are in 0002. I will
rebase your original 0002 patch later. I think 0001 is much more
important, as evident by the reported benchmarks on this thread.
In absence of anyone else looking at this, I think it's ready to go.
If anyone is following along and wants to review or test it, please do
so soon.
David
