apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "William A. Rowe, Jr." <wr...@rowe-clan.net>
Subject Re: [PATCH] fix cleanups in cleanups
Date Fri, 21 Sep 2001 13:44:30 GMT
From: "Sander Striker" <striker@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


View raw message