MemoryContext reset/delete callbacks
We discussed this idea a couple weeks ago. The core of it is that when a
memory context is being deleted, you might want something extra to happen
beyond just pfree'ing everything in the context. I'm thinking in
particular that this might provide a nice solution to the problem we
discussed earlier today of managing cached information about a domain's
CHECK constraints: a function could make a reference-counted link to some
data in its FmgrInfo cache, and use a memory context callback on the
context containing the FmgrInfo to ensure that the reference count gets
decremented when needed and not before.
Attached is a draft patch for this. I've not really tested it more than
seeing that it compiles, but it's so simple that there are unlikely to be
bugs as such in it. There are some design decisions that could be
questioned though:
1. I used ilists for the linked list of callback requests. This creates a
dependency from memory contexts to lib/ilist. That's all right at the
moment since lib/ilist does no memory allocation, but it might be
logically problematic. We could just use explicit "struct foo *" links
with little if any notational penalty, so I wonder if that would be
better.
2. I specified that callers have to allocate the storage for the callback
structs. This saves a palloc cycle in just about every use-case I've
thought of, since callers generally will need to palloc some larger struct
of their own and they can just embed the MemoryContextCallback struct in
that. It does seem like this offers a larger surface for screwups, in
particular putting the callback struct in the wrong memory context --- but
there's plenty of surface for that type of mistake anyway, if you put
whatever the "void *arg" is pointing at in the wrong context.
So I'm OK with this but could also accept a design in which
MemoryContextRegisterResetCallback takes just a function pointer and a
"void *arg" and palloc's the callback struct for itself in the target
context. I'm unsure whether that adds enough safety to justify a separate
palloc.
3. Is the "void *arg" API for the callback func sufficient? I considered
also passing it the MemoryContext, but couldn't really find a use-case
to justify that.
4. I intentionally didn't provide a MemoryContextDeregisterResetCallback
API. Since it's just a singly-linked list, that could be an expensive
operation and so I'd rather discourage it. In the use cases I've thought
of, you'd want the callback to remain active for the life of the context
anyway, so there would be no need. (And, of course, there is slist_delete
for the truly determined ...)
Comments?
regards, tom lane
Attachments:
memory-context-callbacks-1.0.patchtext/x-diff; charset=us-ascii; name=memory-context-callbacks-1.0.patchDownload+82-2
Hi,
On 2015-02-26 19:28:50 -0500, Tom Lane wrote:
We discussed this idea a couple weeks ago.
Hm, didn't follow that discussion.
The core of it is that when a memory context is being deleted, you
might want something extra to happen beyond just pfree'ing everything
in the context.
I've certainly wished for this a couple times...
Attached is a draft patch for this. I've not really tested it more than
seeing that it compiles, but it's so simple that there are unlikely to be
bugs as such in it. There are some design decisions that could be
questioned though:1. I used ilists for the linked list of callback requests. This creates a
dependency from memory contexts to lib/ilist. That's all right at the
moment since lib/ilist does no memory allocation, but it might be
logically problematic. We could just use explicit "struct foo *" links
with little if any notational penalty, so I wonder if that would be
better.
Maybe I'm partial here, but I don't see a problem. Actually the reason I
started the ilist stuff was that I wrote a different memory context
implementation ;). Wish I'd time/energy to go back to that...
2. I specified that callers have to allocate the storage for the callback
structs. This saves a palloc cycle in just about every use-case I've
thought of, since callers generally will need to palloc some larger struct
of their own and they can just embed the MemoryContextCallback struct in
that. It does seem like this offers a larger surface for screwups, in
particular putting the callback struct in the wrong memory context --- but
there's plenty of surface for that type of mistake anyway, if you put
whatever the "void *arg" is pointing at in the wrong context.
So I'm OK with this but could also accept a design in which
MemoryContextRegisterResetCallback takes just a function pointer and a
"void *arg" and palloc's the callback struct for itself in the target
context. I'm unsure whether that adds enough safety to justify a separate
palloc.
I'm unworried about this. Yes, one might be confused for a short while,
but compared to the complexity of using any such facility sanely it
seems barely relevant.
3. Is the "void *arg" API for the callback func sufficient? I considered
also passing it the MemoryContext, but couldn't really find a use-case
to justify that.
Hm, seems sufficient to me.
4. I intentionally didn't provide a MemoryContextDeregisterResetCallback
API. Since it's just a singly-linked list, that could be an expensive
operation and so I'd rather discourage it. In the use cases I've thought
of, you'd want the callback to remain active for the life of the context
anyway, so there would be no need. (And, of course, there is slist_delete
for the truly determined ...)
Yea, that seems fine. If you don't want the callback to do anything
anymore, it's easy enough to just set a flag somewhere.
/*
*************** typedef struct MemoryContextData
*** 59,72 ****
MemoryContext firstchild; /* head of linked list of children */
MemoryContext nextchild; /* next child of same parent */
char *name; /* context name (just for debugging) */
bool isReset; /* T = no space alloced since last reset */
#ifdef USE_ASSERT_CHECKING
! bool allowInCritSection; /* allow palloc in critical section */
#endif
} MemoryContextData;
It's a bit sad to push AllocSetContextData onto four cachelines from the
current three... That stuff is hot. But I don't really see a way around
it right now. And it seems like it'd give us more amunition to improve
things than the small loss of speed it implies.
I guess it might needa a warning about being careful what you directly
free if you use this. Might also better fit in a layer above this...
Greetings,
Andres Freund
--
Andres Freund 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
On 2015-02-27 01:54:27 +0100, Andres Freund wrote:
On 2015-02-26 19:28:50 -0500, Tom Lane wrote:
/*
*************** typedef struct MemoryContextData
*** 59,72 ****
MemoryContext firstchild; /* head of linked list of children */
MemoryContext nextchild; /* next child of same parent */
char *name; /* context name (just for debugging) */
bool isReset; /* T = no space alloced since last reset */
#ifdef USE_ASSERT_CHECKING
! bool allowInCritSection; /* allow palloc in critical section */
#endif
} MemoryContextData;It's a bit sad to push AllocSetContextData onto four cachelines from the
current three... That stuff is hot. But I don't really see a way around
it right now. And it seems like it'd give us more amunition to improve
things than the small loss of speed it implies.
Actually:
struct MemoryContextData {
NodeTag type; /* 0 4 */
/* XXX 4 bytes hole, try to pack */
MemoryContextMethods * methods; /* 8 8 */
MemoryContext parent; /* 16 8 */
MemoryContext firstchild; /* 24 8 */
MemoryContext nextchild; /* 32 8 */
char * name; /* 40 8 */
bool isReset; /* 48 1 */
bool allowInCritSection; /* 49 1 */
/* size: 56, cachelines: 1, members: 8 */
/* sum members: 46, holes: 1, sum holes: 4 */
/* padding: 6 */
/* last cacheline: 56 bytes */
};
If we move isReset and allowInCritSection after type, we'd stay at the
same size...
Andres Freund
--
Andres Freund 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
Andres Freund <andres@2ndquadrant.com> writes:
It's a bit sad to push AllocSetContextData onto four cachelines from the
current three... That stuff is hot. But I don't really see a way around
it right now. And it seems like it'd give us more amunition to improve
things than the small loss of speed it implies.
Meh. I doubt it would make any difference, especially seeing that the
struct isn't going to be aligned on any special boundary.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Feb 26, 2015 at 9:34 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andres Freund <andres@2ndquadrant.com> writes:
It's a bit sad to push AllocSetContextData onto four cachelines from the
current three... That stuff is hot. But I don't really see a way around
it right now. And it seems like it'd give us more amunition to improve
things than the small loss of speed it implies.Meh. I doubt it would make any difference, especially seeing that the
struct isn't going to be aligned on any special boundary.
It might not make much difference, but I think avoiding unnecessary
padding space inside frequently-used data structures is probably a
smart idea.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@2ndquadrant.com> writes:
On 2015-02-26 19:28:50 -0500, Tom Lane wrote:
1. I used ilists for the linked list of callback requests. This creates a
dependency from memory contexts to lib/ilist. That's all right at the
moment since lib/ilist does no memory allocation, but it might be
logically problematic. We could just use explicit "struct foo *" links
with little if any notational penalty, so I wonder if that would be
better.
Maybe I'm partial here, but I don't see a problem. Actually the reason I
started the ilist stuff was that I wrote a different memory context
implementation ;). Wish I'd time/energy to go back to that...
After further reflection, I concluded that it was better to go with the
low-tech "struct foo *next" approach. Aside from the question of whether
we really want mcxt.c to have any more dependencies than it already does,
there's the stylistic point that mcxt.c is already managing lists (of
child contexts) that way; it would be odd to have two different list
technologies in use in the one data structure.
You could of course argue that both of these should be changed to slists,
but that would be a matter for a separate patch. (And frankly, I'm not
so in love with the slist notation that I'd think it an improvement.)
I also rearranged the bool fields as you suggested to avoid wasted padding
space. I'm still not exactly convinced that it's worth the ugliness, but
it's not worth arguing about.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers