On 18/10/16 22:25, Robert Haas wrote: > On Wed, Oct 5, 2016 at 12:22 AM, Tomas Vondra > <tomas.von...@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. >
Are you looking at old version of the patch? I complained about this as well and Tomas has changed that. > 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. You are definitely looking at old version. > > 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. It's used for TXNs which are fixed and some tuples (there is assumption that the decoded tuples have more or less normal distribution). I agree though that the usability beyond the ReoderBuffer is limited because everything that will want to use it for part of allocations will get much more complicated by the fact that it will have to use two different allocators. I was wondering if rather than trying to implement new allocator we should maybe implement palloc_fixed which would use some optimized algorithm for fixed sized objects in our current allocator. The advantage of that would be that we could for example use that for things like ListCell easily (memory management of which I see quite often in profiles). -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
