You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Greg Stein <gs...@lyra.org> on 2001/09/21 02:48:45 UTC
Re: [PATCH] fix cleanups in cleanups
On Thu, Sep 20, 2001 at 11:18:55AM -0700, Aaron Bannert wrote:
>...
> Does this fix it for you? All testmem tests passed for me and your code
> above properly flushes "Cleanup" to the file.
>
> (Someone needs to check my work on run_child_cleanups() and make sure
> that the popping is necessary. It looked to be in the same situation.)
Calling pop_cleanup() on every iteration is a bit much. Consider the
following patch:
while ((c = p->cleanups) != NULL) {
p->cleanups = NULL;
run_cleanups(c);
}
You don't even have to change run_cleanups or run_child_cleanups.
Basically, the above code processes the cleanups in batches. Everything that
was initially registered is processed, then everything registerd during the
first cleanup round, etc.
It does not maintain the LIFO behavior where cleanup A registers cleanup B
and expects B to run *just after* cleanup A finishes. If A wanted that, then
it could just calll B. But the most important part: all cleanups *do* get
run.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: [PATCH] fix cleanups in cleanups
Posted by Ryan Bloom <rb...@covalent.net>.
On Thursday 20 September 2001 08:12 pm, Aaron Bannert wrote:
> On Thu, Sep 20, 2001 at 05:48:45PM -0700, Greg Stein wrote:
> > On Thu, Sep 20, 2001 at 11:18:55AM -0700, Aaron Bannert wrote:
> > Basically, the above code processes the cleanups in batches. Everything
> > that was initially registered is processed, then everything registerd
> > during the first cleanup round, etc.
>
> That does encourage deeper recursion, would this be a potential problem
> for OSs like Netware that have a rather small and limited stack size?
> I don't really know how much the pool_clear() routines consume stack space.
>
> > It does not maintain the LIFO behavior where cleanup A registers cleanup
> > B and expects B to run *just after* cleanup A finishes. If A wanted that,
> > then it could just calll B. But the most important part: all cleanups
> > *do* get run.
>
> Correct me if I'm wrong, it is a LIFO and what Ryan wants is a FIFO.
>
> LIFO == cleanup registers a cleanup, it gets run after the cleanups
> FIFO == cleanup registers a cleanup, it gets run as soon as this one
> returns
>
> Am I missing something?
Well, I wouldn't say what I want, rather the way cleanups have always worked.
As for the FIFO vs LIFO, I think you have it backwards.
LIFO == cleanup registers a cleanup, it gets run as soon as this one returns
FIFO == cleanup registers a cleanup, it gets run after the the current batch
of cleanups.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: [PATCH] fix cleanups in cleanups
Posted by Ryan Bloom <rb...@covalent.net>.
On Thursday 20 September 2001 08:12 pm, Aaron Bannert wrote:
> On Thu, Sep 20, 2001 at 05:48:45PM -0700, Greg Stein wrote:
> > On Thu, Sep 20, 2001 at 11:18:55AM -0700, Aaron Bannert wrote:
> > Basically, the above code processes the cleanups in batches. Everything
> > that was initially registered is processed, then everything registerd
> > during the first cleanup round, etc.
>
> That does encourage deeper recursion, would this be a potential problem
> for OSs like Netware that have a rather small and limited stack size?
> I don't really know how much the pool_clear() routines consume stack space.
>
> > It does not maintain the LIFO behavior where cleanup A registers cleanup
> > B and expects B to run *just after* cleanup A finishes. If A wanted that,
> > then it could just calll B. But the most important part: all cleanups
> > *do* get run.
>
> Correct me if I'm wrong, it is a LIFO and what Ryan wants is a FIFO.
>
> LIFO == cleanup registers a cleanup, it gets run after the cleanups
> FIFO == cleanup registers a cleanup, it gets run as soon as this one
> returns
>
> Am I missing something?
Well, I wouldn't say what I want, rather the way cleanups have always worked.
As for the FIFO vs LIFO, I think you have it backwards.
LIFO == cleanup registers a cleanup, it gets run as soon as this one returns
FIFO == cleanup registers a cleanup, it gets run after the the current batch
of cleanups.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: [PATCH] fix cleanups in cleanups
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
From: "Sander Striker" <st...@apache.org>
Sent: Friday, September 21, 2001 2:51 AM
> > From: Greg Stein [mailto:gstein@lyra.org]
> > Sent: 21 September 2001 09:35
> > On Thu, Sep 20, 2001 at 07:54:22PM -0700, Ryan Bloom wrote:
> > > On Thursday 20 September 2001 05:48 pm, Greg Stein wrote:
> > ....
> > > > Calling pop_cleanup() on every iteration is a bit much. Consider the
> > > > following patch:
> > >
> > > Why is it a bit much? I just took a quick look at it, it is an
> > if, and three assignments.
> >
> > Heh. It is a bit much when you consider you consider a thread in October
> > 1999 discussing using NULL rather than ap_null_cleanup:
> >
> > http://www.humanfactor.com/cgi-bin/cgi-delegate/apache-ML/nh/1999/
> > Oct/0189.html
> >
> > In that case, an "if" was considered too much :-)
>
> I always wondered about that. It seems so silly to have to pass in
> apr_pool_cleanup_null allmost all the time as second argument. It isn't
> very clear for first time users and certainly not intuitive. Oh well,
> Manoj pointed this out aswell in that thread and still we have
> apr_pool_cleanup_null...
Not only is it non-intuitive (to a first time apr_pools coder) but it
is plain wrong. The cycles to set up the stack and call directly to
a return stub is _far_ slower than testing if (int_val). And compiler
and cpu instruction discussions are always appropriate in this forum.
There are several options here that satisfy everyone, except the few who
seem to insist on forcing broken cleanup behavior to keep coder folks
'on their toes'.
First, there is no reason to push a cleanup at all today of a null value.
We have a single stack of two lists (a prepare fork and a general cleanup.)
There is no reason I can see for a single stack. Two stacks would be more
effective to implement what we want.
Greg's pointed out that the registering a cleanup for child fork (which
we do _often_ due to our set/unset inheritance toggle) breaks the ordering
of sub-object creation. And cleaning up for a child fork is even worse;
even if we 'fix' the broken reordering of pool cleanups, the child cleanups
are still misordered.
That is a _bug_. If we _cannot_ preserve LIFO, our model is wrong,
and it will return to bite us. As one who's spent hours tracking down
dynamic lib load/unload bugs of all sorts, from MS COM+ to Perl XS setup
and teardown, it's much more important to get this right.
Aaron's patch doesn't solve all these problems, but it's a start. Unless
someone has a valid technical veto, let's please commit and begin digging
deeper.
Bill
Re: [PATCH] fix cleanups in cleanups
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
From: "Sander Striker" <st...@apache.org>
Sent: Friday, September 21, 2001 2:51 AM
> > From: Greg Stein [mailto:gstein@lyra.org]
> > Sent: 21 September 2001 09:35
> > On Thu, Sep 20, 2001 at 07:54:22PM -0700, Ryan Bloom wrote:
> > > On Thursday 20 September 2001 05:48 pm, Greg Stein wrote:
> > ....
> > > > Calling pop_cleanup() on every iteration is a bit much. Consider the
> > > > following patch:
> > >
> > > Why is it a bit much? I just took a quick look at it, it is an
> > if, and three assignments.
> >
> > Heh. It is a bit much when you consider you consider a thread in October
> > 1999 discussing using NULL rather than ap_null_cleanup:
> >
> > http://www.humanfactor.com/cgi-bin/cgi-delegate/apache-ML/nh/1999/
> > Oct/0189.html
> >
> > In that case, an "if" was considered too much :-)
>
> I always wondered about that. It seems so silly to have to pass in
> apr_pool_cleanup_null allmost all the time as second argument. It isn't
> very clear for first time users and certainly not intuitive. Oh well,
> Manoj pointed this out aswell in that thread and still we have
> apr_pool_cleanup_null...
Not only is it non-intuitive (to a first time apr_pools coder) but it
is plain wrong. The cycles to set up the stack and call directly to
a return stub is _far_ slower than testing if (int_val). And compiler
and cpu instruction discussions are always appropriate in this forum.
There are several options here that satisfy everyone, except the few who
seem to insist on forcing broken cleanup behavior to keep coder folks
'on their toes'.
First, there is no reason to push a cleanup at all today of a null value.
We have a single stack of two lists (a prepare fork and a general cleanup.)
There is no reason I can see for a single stack. Two stacks would be more
effective to implement what we want.
Greg's pointed out that the registering a cleanup for child fork (which
we do _often_ due to our set/unset inheritance toggle) breaks the ordering
of sub-object creation. And cleaning up for a child fork is even worse;
even if we 'fix' the broken reordering of pool cleanups, the child cleanups
are still misordered.
That is a _bug_. If we _cannot_ preserve LIFO, our model is wrong,
and it will return to bite us. As one who's spent hours tracking down
dynamic lib load/unload bugs of all sorts, from MS COM+ to Perl XS setup
and teardown, it's much more important to get this right.
Aaron's patch doesn't solve all these problems, but it's a start. Unless
someone has a valid technical veto, let's please commit and begin digging
deeper.
Bill
RE: [PATCH] fix cleanups in cleanups
Posted by Sander Striker <st...@apache.org>.
> From: Greg Stein [mailto:gstein@lyra.org]
> Sent: 21 September 2001 09:35
> On Thu, Sep 20, 2001 at 07:54:22PM -0700, Ryan Bloom wrote:
> > On Thursday 20 September 2001 05:48 pm, Greg Stein wrote:
> ....
> > > Calling pop_cleanup() on every iteration is a bit much. Consider the
> > > following patch:
> >
> > Why is it a bit much? I just took a quick look at it, it is an
> if, and three assignments.
>
> Heh. It is a bit much when you consider you consider a thread in October
> 1999 discussing using NULL rather than ap_null_cleanup:
>
> http://www.humanfactor.com/cgi-bin/cgi-delegate/apache-ML/nh/1999/
> Oct/0189.html
>
> In that case, an "if" was considered too much :-)
I always wondered about that. It seems so silly to have to pass in
apr_pool_cleanup_null allmost all the time as second argument. It isn't
very clear for first time users and certainly not intuitive. Oh well,
Manoj pointed this out aswell in that thread and still we have
apr_pool_cleanup_null...
Sander
RE: [PATCH] fix cleanups in cleanups
Posted by Sander Striker <st...@apache.org>.
> From: Greg Stein [mailto:gstein@lyra.org]
> Sent: 21 September 2001 09:35
> On Thu, Sep 20, 2001 at 07:54:22PM -0700, Ryan Bloom wrote:
> > On Thursday 20 September 2001 05:48 pm, Greg Stein wrote:
> ....
> > > Calling pop_cleanup() on every iteration is a bit much. Consider the
> > > following patch:
> >
> > Why is it a bit much? I just took a quick look at it, it is an
> if, and three assignments.
>
> Heh. It is a bit much when you consider you consider a thread in October
> 1999 discussing using NULL rather than ap_null_cleanup:
>
> http://www.humanfactor.com/cgi-bin/cgi-delegate/apache-ML/nh/1999/
> Oct/0189.html
>
> In that case, an "if" was considered too much :-)
I always wondered about that. It seems so silly to have to pass in
apr_pool_cleanup_null allmost all the time as second argument. It isn't
very clear for first time users and certainly not intuitive. Oh well,
Manoj pointed this out aswell in that thread and still we have
apr_pool_cleanup_null...
Sander
Re: [PATCH] fix cleanups in cleanups
Posted by Greg Stein <gs...@lyra.org>.
On Thu, Sep 20, 2001 at 07:54:22PM -0700, Ryan Bloom wrote:
> On Thursday 20 September 2001 05:48 pm, Greg Stein wrote:
...
> > Calling pop_cleanup() on every iteration is a bit much. Consider the
> > following patch:
>
> Why is it a bit much? I just took a quick look at it, it is an if, and three assignments.
Heh. It is a bit much when you consider you consider a thread in October
1999 discussing using NULL rather than ap_null_cleanup:
http://www.humanfactor.com/cgi-bin/cgi-delegate/apache-ML/nh/1999/Oct/0189.html
In that case, an "if" was considered too much :-)
> I would assume that any compiler worth it's salt would inline this function as well.
Very bad assumption. But this isn't the place to discuss compilers.
> This patch also keeps the LIFO behavior, which is important, because it means
> that it is much less likely that an item allocated out of the pool when the cleanup
> was registered will no longer be there when the cleanup is run.
I am telling you that dependency upon items in the pool is bogus. You cannot
do that. Thus, LIFO means diddly squat.
Consider a case where A inserts its cleanup, expecting to have B present. At
some point between then and the pool cleanup, B goes and removes its
cleanup, then inserts some other cleanup. This new one, B', will now run
*before* A's cleanup.
Gee. Guess what? Your oh-so-needed LIFO behavior has been screwed. B is
*not* present for A to use.
The *only* guarantees you have are between parent and child pools. Within a
pool, you've got nothing.
> > while ((c = p->cleanups) != NULL) {
> > p->cleanups = NULL;
> > run_cleanups(c);
> > }
>
> Which function is this in? I have looked, but the only place that I can find to
> put this code would be in apr_pool_clear, around the run_cleanups code.
Yes, it would go in apr_pool_clear. And a similar one for the child cleanup
case.
> > Basically, the above code processes the cleanups in batches. Everything
> > that was initially registered is processed, then everything registerd
> > during the first cleanup round, etc.
>
> This makes it more more likely that a variable in the same pool that was available
> when the cleanup was registered would not be available when your cleanup
> ran. I would really want to see a performance analysis before we broke that
> behavior.
Broke what behavior? Registering a cleanup in a cleanup doesn't work now.
The new behavior is, "if you register it, I'll run it after I'm done running
these other cleanups."
Stop. Take a breath. Think about it. LIFO doesn't matter.
-g
--
Greg Stein, http://www.lyra.org/
Re: [PATCH] fix cleanups in cleanups
Posted by Greg Stein <gs...@lyra.org>.
On Thu, Sep 20, 2001 at 07:54:22PM -0700, Ryan Bloom wrote:
> On Thursday 20 September 2001 05:48 pm, Greg Stein wrote:
...
> > Calling pop_cleanup() on every iteration is a bit much. Consider the
> > following patch:
>
> Why is it a bit much? I just took a quick look at it, it is an if, and three assignments.
Heh. It is a bit much when you consider you consider a thread in October
1999 discussing using NULL rather than ap_null_cleanup:
http://www.humanfactor.com/cgi-bin/cgi-delegate/apache-ML/nh/1999/Oct/0189.html
In that case, an "if" was considered too much :-)
> I would assume that any compiler worth it's salt would inline this function as well.
Very bad assumption. But this isn't the place to discuss compilers.
> This patch also keeps the LIFO behavior, which is important, because it means
> that it is much less likely that an item allocated out of the pool when the cleanup
> was registered will no longer be there when the cleanup is run.
I am telling you that dependency upon items in the pool is bogus. You cannot
do that. Thus, LIFO means diddly squat.
Consider a case where A inserts its cleanup, expecting to have B present. At
some point between then and the pool cleanup, B goes and removes its
cleanup, then inserts some other cleanup. This new one, B', will now run
*before* A's cleanup.
Gee. Guess what? Your oh-so-needed LIFO behavior has been screwed. B is
*not* present for A to use.
The *only* guarantees you have are between parent and child pools. Within a
pool, you've got nothing.
> > while ((c = p->cleanups) != NULL) {
> > p->cleanups = NULL;
> > run_cleanups(c);
> > }
>
> Which function is this in? I have looked, but the only place that I can find to
> put this code would be in apr_pool_clear, around the run_cleanups code.
Yes, it would go in apr_pool_clear. And a similar one for the child cleanup
case.
> > Basically, the above code processes the cleanups in batches. Everything
> > that was initially registered is processed, then everything registerd
> > during the first cleanup round, etc.
>
> This makes it more more likely that a variable in the same pool that was available
> when the cleanup was registered would not be available when your cleanup
> ran. I would really want to see a performance analysis before we broke that
> behavior.
Broke what behavior? Registering a cleanup in a cleanup doesn't work now.
The new behavior is, "if you register it, I'll run it after I'm done running
these other cleanups."
Stop. Take a breath. Think about it. LIFO doesn't matter.
-g
--
Greg Stein, http://www.lyra.org/
Re: [PATCH] fix cleanups in cleanups
Posted by Ryan Bloom <rb...@covalent.net>.
On Thursday 20 September 2001 05:48 pm, Greg Stein wrote:
> On Thu, Sep 20, 2001 at 11:18:55AM -0700, Aaron Bannert wrote:
> >...
> > Does this fix it for you? All testmem tests passed for me and your code
> > above properly flushes "Cleanup" to the file.
> >
> > (Someone needs to check my work on run_child_cleanups() and make sure
> > that the popping is necessary. It looked to be in the same situation.)
>
> Calling pop_cleanup() on every iteration is a bit much. Consider the
> following patch:
Why is it a bit much? I just took a quick look at it, it is an if, and three assignments.
I would assume that any compiler worth it's salt would inline this function as well.
This patch also keeps the LIFO behavior, which is important, because it means
that it is much less likely that an item allocated out of the pool when the cleanup
was registered will no longer be there when the cleanup is run.
>
> while ((c = p->cleanups) != NULL) {
> p->cleanups = NULL;
> run_cleanups(c);
> }
Which function is this in? I have looked, but the only place that I can find to
put this code would be in apr_pool_clear, around the run_cleanups code.
> Basically, the above code processes the cleanups in batches. Everything
> that was initially registered is processed, then everything registerd
> during the first cleanup round, etc.
This makes it more more likely that a variable in the same pool that was available
when the cleanup was registered would not be available when your cleanup
ran. I would really want to see a performance analysis before we broke that
behavior.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: [PATCH] fix cleanups in cleanups
Posted by Ryan Bloom <rb...@covalent.net>.
On Thursday 20 September 2001 05:48 pm, Greg Stein wrote:
> On Thu, Sep 20, 2001 at 11:18:55AM -0700, Aaron Bannert wrote:
> >...
> > Does this fix it for you? All testmem tests passed for me and your code
> > above properly flushes "Cleanup" to the file.
> >
> > (Someone needs to check my work on run_child_cleanups() and make sure
> > that the popping is necessary. It looked to be in the same situation.)
>
> Calling pop_cleanup() on every iteration is a bit much. Consider the
> following patch:
>
> while ((c = p->cleanups) != NULL) {
> p->cleanups = NULL;
> run_cleanups(c);
> }
>
> You don't even have to change run_cleanups or run_child_cleanups.
>
> Basically, the above code processes the cleanups in batches. Everything
> that was initially registered is processed, then everything registerd
> during the first cleanup round, etc.
>
> It does not maintain the LIFO behavior where cleanup A registers cleanup B
> and expects B to run *just after* cleanup A finishes. If A wanted that,
> then it could just calll B. But the most important part: all cleanups *do*
> get run.
You've got to keep the LIFO behavior, or the kind of problems you posted
about yesterday are more likely.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: [PATCH] fix cleanups in cleanups
Posted by Ryan Bloom <rb...@covalent.net>.
On Thursday 20 September 2001 05:48 pm, Greg Stein wrote:
> On Thu, Sep 20, 2001 at 11:18:55AM -0700, Aaron Bannert wrote:
> >...
> > Does this fix it for you? All testmem tests passed for me and your code
> > above properly flushes "Cleanup" to the file.
> >
> > (Someone needs to check my work on run_child_cleanups() and make sure
> > that the popping is necessary. It looked to be in the same situation.)
>
> Calling pop_cleanup() on every iteration is a bit much. Consider the
> following patch:
>
> while ((c = p->cleanups) != NULL) {
> p->cleanups = NULL;
> run_cleanups(c);
> }
>
> You don't even have to change run_cleanups or run_child_cleanups.
>
> Basically, the above code processes the cleanups in batches. Everything
> that was initially registered is processed, then everything registerd
> during the first cleanup round, etc.
>
> It does not maintain the LIFO behavior where cleanup A registers cleanup B
> and expects B to run *just after* cleanup A finishes. If A wanted that,
> then it could just calll B. But the most important part: all cleanups *do*
> get run.
You've got to keep the LIFO behavior, or the kind of problems you posted
about yesterday are more likely.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: [PATCH] fix cleanups in cleanups
Posted by Ryan Bloom <rb...@covalent.net>.
On Thursday 20 September 2001 05:48 pm, Greg Stein wrote:
> On Thu, Sep 20, 2001 at 11:18:55AM -0700, Aaron Bannert wrote:
> >...
> > Does this fix it for you? All testmem tests passed for me and your code
> > above properly flushes "Cleanup" to the file.
> >
> > (Someone needs to check my work on run_child_cleanups() and make sure
> > that the popping is necessary. It looked to be in the same situation.)
>
> Calling pop_cleanup() on every iteration is a bit much. Consider the
> following patch:
Why is it a bit much? I just took a quick look at it, it is an if, and three assignments.
I would assume that any compiler worth it's salt would inline this function as well.
This patch also keeps the LIFO behavior, which is important, because it means
that it is much less likely that an item allocated out of the pool when the cleanup
was registered will no longer be there when the cleanup is run.
>
> while ((c = p->cleanups) != NULL) {
> p->cleanups = NULL;
> run_cleanups(c);
> }
Which function is this in? I have looked, but the only place that I can find to
put this code would be in apr_pool_clear, around the run_cleanups code.
> Basically, the above code processes the cleanups in batches. Everything
> that was initially registered is processed, then everything registerd
> during the first cleanup round, etc.
This makes it more more likely that a variable in the same pool that was available
when the cleanup was registered would not be available when your cleanup
ran. I would really want to see a performance analysis before we broke that
behavior.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: [PATCH] fix cleanups in cleanups
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
From: "Aaron Bannert" <aa...@clove.org>
Sent: Thursday, September 20, 2001 10:12 PM
> On Thu, Sep 20, 2001 at 05:48:45PM -0700, Greg Stein wrote:
> > On Thu, Sep 20, 2001 at 11:18:55AM -0700, Aaron Bannert wrote:
> > >...
> > > Does this fix it for you? All testmem tests passed for me and your code
> > > above properly flushes "Cleanup" to the file.
> > >
> > > (Someone needs to check my work on run_child_cleanups() and make sure
> > > that the popping is necessary. It looked to be in the same situation.)
> >
> > Calling pop_cleanup() on every iteration is a bit much. Consider the
> > following patch:
>
> The reason I went that route, instead of just inlining the pop_cleanup()
> code is that it gets called in two places. If it is a performance issue,
> I'd just put it all inside the while loop in run_cleanups(). Would
> that be preferable?
Aaron, you were right on here. If I'm in a cleanup, which has to create an
apr_foo entity on the same pool, when I'm done with my cleanup, that new
cleanup added by apr_foo should be called _next_. So it's not overkill to
treat this as a pure stack.
> > Basically, the above code processes the cleanups in batches. Everything that
> > was initially registered is processed, then everything registerd during the
> > first cleanup round, etc.
>
> That does encourage deeper recursion, would this be a potential problem
> for OSs like Netware that have a rather small and limited stack size?
> I don't really know how much the pool_clear() routines consume stack space.
This actually sounds cyclically hazerdous. (not that the other cleanup couldn't
suffer the same, if three different cleanups kept registering one another, round
robin style, by creating objects of each other.)
I'd rather stay very clean, and know that everything in existance when a cleanup
is registered may still exist when that cleanup is called. This 'batching'
appears too hazerdous, from that perspective.
> > It does not maintain the LIFO behavior where cleanup A registers cleanup B
> > and expects B to run *just after* cleanup A finishes. If A wanted that, then
> > it could just calll B. But the most important part: all cleanups *do* get
> > run.
>
> Correct me if I'm wrong, it is a LIFO and what Ryan wants is a FIFO.
>
> LIFO == cleanup registers a cleanup, it gets run after the cleanups
> FIFO == cleanup registers a cleanup, it gets run as soon as this one returns
>
> Am I missing something?
Yes, FIFO = cleanup registers a cleaup, it was the _last_ one registered, so it
would be cleaned up last. That's exactly what we don't want.
LIFO = cleanup registers a cleanup, it is the _last_ one registered, it is the
first one cleaned off (even if we are in the processing of running the list of
cleanups as it was registered.)
Bill
Re: [PATCH] fix cleanups in cleanups
Posted by Aaron Bannert <aa...@clove.org>.
On Thu, Sep 20, 2001 at 05:48:45PM -0700, Greg Stein wrote:
> On Thu, Sep 20, 2001 at 11:18:55AM -0700, Aaron Bannert wrote:
> >...
> > Does this fix it for you? All testmem tests passed for me and your code
> > above properly flushes "Cleanup" to the file.
> >
> > (Someone needs to check my work on run_child_cleanups() and make sure
> > that the popping is necessary. It looked to be in the same situation.)
>
> Calling pop_cleanup() on every iteration is a bit much. Consider the
> following patch:
The reason I went that route, instead of just inlining the pop_cleanup()
code is that it gets called in two places. If it is a performance issue,
I'd just put it all inside the while loop in run_cleanups(). Would
that be preferable?
>
> while ((c = p->cleanups) != NULL) {
> p->cleanups = NULL;
> run_cleanups(c);
> }
>
> You don't even have to change run_cleanups or run_child_cleanups.
Wouldn't that go in run_cleanups(), or does this go in apr_pool_clear()?
> Basically, the above code processes the cleanups in batches. Everything that
> was initially registered is processed, then everything registerd during the
> first cleanup round, etc.
That does encourage deeper recursion, would this be a potential problem
for OSs like Netware that have a rather small and limited stack size?
I don't really know how much the pool_clear() routines consume stack space.
> It does not maintain the LIFO behavior where cleanup A registers cleanup B
> and expects B to run *just after* cleanup A finishes. If A wanted that, then
> it could just calll B. But the most important part: all cleanups *do* get
> run.
Correct me if I'm wrong, it is a LIFO and what Ryan wants is a FIFO.
LIFO == cleanup registers a cleanup, it gets run after the cleanups
FIFO == cleanup registers a cleanup, it gets run as soon as this one returns
Am I missing something?
-aaron
RE: Fix broken cleanups model
Posted by Sander Striker <st...@apache.org>.
> From: William A. Rowe, Jr. [mailto:wrowe@rowe-clan.net]
> Sent: 21 September 2001 16:42
> ----- Original Message -----
> From: "Ben Hyde" <bh...@pobox.com>
> To: <de...@httpd.apache.org>
> Sent: Friday, September 21, 2001 9:12 AM
> > I also think it's a long standing mistake that the subpools aren't
> > unwound via the same cleanup stack as everything else but have their
> > one one off to the side; that's a royal pain when you want to allocate
> > a node into it's own subpool while doing things like the above.
>
> Agreed. Here are my thoughts.
>
> We introduce an apr_cleanup_t. This is a _single_ cleanup holder for
> mutiple, extendable types of cleanups (at this moment, pool cleanup and
> cleanup for exec.)
We tried to change that in SMS. You might want to take a look
at that (concept wise).
Sander
Fix broken cleanups model
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
----- Original Message -----
From: "Ben Hyde" <bh...@pobox.com>
To: <de...@httpd.apache.org>
Sent: Friday, September 21, 2001 9:12 AM
Subject: Re: [PATCH] fix cleanups in cleanups
> I also think it's a long standing mistake that the subpools aren't
> unwound via the same cleanup stack as everything else but have their
> one one off to the side; that's a royal pain when you want to allocate
> a node into it's own subpool while doing things like the above.
Agreed. Here are my thoughts.
We introduce an apr_cleanup_t. This is a _single_ cleanup holder for
mutiple, extendable types of cleanups (at this moment, pool cleanup and
cleanup for exec.)
All apr types (and http types) that need cleanups get one of these
records. Because it exists _within_ the allocated data, we can
set/unset them at will. This would no longer change the registrated
ordering.
The API must _allow_ us to move the ordering when it's absolutely
appropriate. If a type is created, but a second call does the 'dirty
work' of obtaining the resource (such as make/create or create/open
pairs) then the order must be fixed to obtaining the resource. If
someone used the apr structure before a resource is obtained, they
really have a placeholder, not the live resource.
We allow users to register a cleanup as we always have. Once it's
registered, they have an apr_cleanup_t. They can unset that cleanup
and register another (changing the order), or they can simply change
their given cleanup. Whichever is appropriate for that call.
As with any library code, a cleanup must be tolerant that _anyone_
could have closed an apr type, either in the mainline, by reacting to
some event, or in it's own reordered cleanup.
Finally, an apr_pool_t is just another apr type. Its cleanup gets
invoked by the mainline cleanup code. I still need to ponder just
what the interaction between two pool's cleanups are, but I'll save
that for another mail after this headcold clears a bit. Hope this
part made sense.
Bill
Re: [PATCH] fix cleanups in cleanups
Posted by Ben Hyde <bh...@pobox.com>.
Greg Stein wrote:
> code processes the cleanups in batches ...
> ... does not maintain the LIFO behavior ...
I think that's a mistake.
I've certainly have writen lots of code that depends on knowing that the
tree I build will be torn down in the right order with the child
cleanups unlinking them from the parents. Sometimes the parents are in
the same pool and sometime not. Sometimes the nodes in these trees
aren't even in pools.
I also think it's a long standing mistake that the subpools aren't
unwound via the same cleanup stack as everything else but have their
one one off to the side; that's a royal pain when you want to allocate
a node into it's own subpool while doing things like the above.
- ben
http://www.cozy.org:83/bhyde.html
Re: [PATCH] fix cleanups in cleanups
Posted by Aaron Bannert <aa...@clove.org>.
On Thu, Sep 20, 2001 at 05:48:45PM -0700, Greg Stein wrote:
> On Thu, Sep 20, 2001 at 11:18:55AM -0700, Aaron Bannert wrote:
> >...
> > Does this fix it for you? All testmem tests passed for me and your code
> > above properly flushes "Cleanup" to the file.
> >
> > (Someone needs to check my work on run_child_cleanups() and make sure
> > that the popping is necessary. It looked to be in the same situation.)
>
> Calling pop_cleanup() on every iteration is a bit much. Consider the
> following patch:
The reason I went that route, instead of just inlining the pop_cleanup()
code is that it gets called in two places. If it is a performance issue,
I'd just put it all inside the while loop in run_cleanups(). Would
that be preferable?
>
> while ((c = p->cleanups) != NULL) {
> p->cleanups = NULL;
> run_cleanups(c);
> }
>
> You don't even have to change run_cleanups or run_child_cleanups.
Wouldn't that go in run_cleanups(), or does this go in apr_pool_clear()?
> Basically, the above code processes the cleanups in batches. Everything that
> was initially registered is processed, then everything registerd during the
> first cleanup round, etc.
That does encourage deeper recursion, would this be a potential problem
for OSs like Netware that have a rather small and limited stack size?
I don't really know how much the pool_clear() routines consume stack space.
> It does not maintain the LIFO behavior where cleanup A registers cleanup B
> and expects B to run *just after* cleanup A finishes. If A wanted that, then
> it could just calll B. But the most important part: all cleanups *do* get
> run.
Correct me if I'm wrong, it is a LIFO and what Ryan wants is a FIFO.
LIFO == cleanup registers a cleanup, it gets run after the cleanups
FIFO == cleanup registers a cleanup, it gets run as soon as this one returns
Am I missing something?
-aaron