Re: PATCH: two slab-like memory allocators - Mailing list pgsql-hackers

On Wed, Oct 5, 2016 at 12:22 AM, Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
> attached is v3 of the patches, with a few minor fixes in Slab, and much
> larger fixes in GenSlab.
>
> Slab (minor fixes)
> ------------------
> - Removed the unnecessary memset() of new blocks in SlabAlloc(), although we
> still need to zero the free bitmap at the end of the block.
> - Renamed minFreeCount to minFreeChunks, added a few comments explaining
> why/how firstFreeChunk and minFreeChunks are maintained.
> - Fixed / improved a bunch of additional comments, based on feedback.

I had a look at 0001 today, but it seems to me that it still needs
work.  It's still got a lot of remnants of where you've
copy-and-pasted aset.c.  I dispute this allegation:

+ * SlabContext is our standard implementation of MemoryContext.

And all of this is just a direct copy-paste; I don't really want two copies:

+ *  When running under Valgrind, we want a NOACCESS memory region both before
+ *  and after the allocation.  The chunk header is tempting as the preceding
+ *  region, but mcxt.c expects to able to examine the standard chunk header
+ *  fields.  Therefore, we use, when available, the requested_size field and
+ *  any subsequent padding.  requested_size is made NOACCESS before returning
+ *  a chunk pointer to a caller.  However, to reduce client request traffic,
+ *  it is kept DEFINED in chunks on the free list.

And then there's this:

+#ifdef HAVE_ALLOCINFO
+#define SlabFreeInfo(_cxt, _chunk) \
+            fprintf(stderr, "AllocFree: %s: %p, %d\n", \
+                (_cxt)->header.name, (_chunk), (_chunk)->size)
+#define SlabAllocInfo(_cxt, _chunk) \
+            fprintf(stderr, "AllocAlloc: %s: %p, %d\n", \
+                (_cxt)->header.name, (_chunk), (_chunk)->size)

Well, it's pretty stupid that AllocSetAlloc is reporting it's name as
AllocAlloc, a think that, as far as I can tell, is not real. But
having this new context type also pretend to be AllocAlloc is even
dumber.

+static void
+randomize_mem(char *ptr, size_t size)
+{
+    static int  save_ctr = 1;
+    size_t      remaining = size;
+    int         ctr;
+
+    ctr = save_ctr;
+    VALGRIND_MAKE_MEM_UNDEFINED(ptr, size);
+    while (remaining-- > 0)
+    {
+        *ptr++ = ctr;
+        if (++ctr > 251)
+            ctr = 1;
+    }
+    VALGRIND_MAKE_MEM_UNDEFINED(ptr - size, size);
+    save_ctr = ctr;
+}
+#endif   /* RANDOMIZE_ALLOCATED_MEMORY */

Another copy of this doesn't seem like a good idea, either.

More broadly, I'm not sure I like this design very much.  The whole
point of a slab context is that all of the objects are the same size.
I wouldn't find it too difficult to support this patch if we were
adding an allocator for fixed-size objects that was then being used to
allocate objects which are fixed size.  However, what we seem to be
doing is creating an allocator for fixed-size objects and then using
it for variable-size tuples.  That's really pretty weird.  Isn't the
root of this problem that aset.c is utterly terrible at handling large
number of allocations?  Maybe we should try to attack that problem
more directly.

On a related note, the autodestruct thing is a weird hack that's only
necessary because of the hijinks already discussed in the previous
paragraph.  The context has no fixed lifetime; we're just trying to
find a way of coping with possibly-shifting tuple sizes over time.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company