httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Aaron Bannert <aa...@ebuilt.com>
Subject Re: [PATCH] APR thread updates and associated httpd-2.0 changes
Date Sat, 21 Jul 2001 06:38:50 GMT
> > Readability, consistency, and even a slight performance boost.
> >
> > I think os2 had it right, so I changed the others to be the same.
> > Not really a name change at all.
> 
> Still a name change, and at the very least this change has nothing to do with 
> the fix that you are saying this patch is all about.  This could and should have 
> been submitted as a separate patch, because with this change, the patch is 
> larger, and harder to review.
> 
> If I remember back to my compiler class, I seem to believe that the value should 
> have been moved into a register after the first reference, so I don't see a huge 
> performance win.  Of course, it has been a while since I took that class.

[warning: geek info]
There are of course no guarantees, and optimized register allocation
will only really apply within basic blocks, rarely across.  When you
have function calls like apr_pcalloc(), all bets are off.

The only way to make sure on all compilers that it does [pretty much]
what you want it to would be to use a single-indirection (a pointer
to the struct), instead of double each time.


> > This change mirrors the same change to unix. Again, because I don't
> > have access to beos I can only do my best to get that code as close
> > as possible to what I think it should be, but in the end I'd need
> > someone who is familiar with that system to review my changes.
> 
> Then you should have left the original code in tact.  At the very least, this should
be:
> 
> if (wait_for_thread(thd->td, (void *)&thrstat) == B_NO_ERROR) {
>     *retval = *thrstat;
> }

I'll leave this intact.

> > > > -    *retval = (apr_status_t)thd->rv;
> > > > +    if (retval != NULL)
> > > > +        *retval = (apr_status_t)thd->rv;
> > >
> > > Again, we don't need this change.
> >
> > How is NULL an invalid return value from a thread? Here are the API
> > docs:
> >
> > /**
> >  * block until the desired thread stops executing.
> >  * @param retval The return value from the dead thread.
> >  * @param thd The thread to join
> >  * @deffunc apr_status_t apr_thread_join(apr_status_t *retval,
> > apr_thread_t *thd); */
> >
> > What if you don't want to pass it an apr_status_t, you just want to
> > join? My changes bring apr_thread_join() more inline with
> > pthread_join(). It's perfectly valid to give pthread_join() a NULL
> > and it won't bother trying to set the return value for you.
> 
> You aren't programming to pthreads right now.  You are programming for APR.
> They are different beasts, with different goals and priorities.  In APR, return
> values mean something, and just ignoring them is not valid.  If you don't want 
> to return a value, then APR_SUCCESS should be returned.

Aha, you see: you can't just return APR_SUCCESS, since apr_thread_exit()
expects a *pointer* to an apr_success_t, and that means an integer's
going to have to be allocated from the heap. I think you'll agree that
in the least that is cumbersome, and more pratically it's a terribly
expensive operation to be performing for a simple status return.

So, should we revise the set of valid input parameters to apr_thread_exit()
and apr_thread_join(), or should we change the prototype to apr_thread_exit()?


> > This particular function is not in use *anywhere* in the httpd-2.0
> > or apr/apr-util code trees. AFAIK, flood is the only place where
> > this is being used. I would be very concerned about changing
> > functionality of existing code if that were not the case, but since
> > it is not I think there is MUCH room for interpretation.
> 
> I disagree, and I designed this API.  The difference is what you are passing
> around.  In pthreads, you are passing in a (void **), which implies that the return
> type is unknown.  In APR, it is an integer, and ALL threads exit with a return
> code.  It doesn't matter where you know about the code being used.  I happen to
> know of multiple closed source programs using APR, neither one of us knows
> everything about those programs so making decisions based on who is and who is
> not using a function doesn't work.

Like I said above, it's not an integer, it's a (apr_status_t*). However,
I see your point about this not being pthread.

I totally agree that in general we shouldn't be making decisions based on who
may or may not be using APR in their private code. My point still remains,
and your statement that "ALL threads exit with a return code" is false.
Read the source. httpd-2.0 returns NULL from a thread worker function
in 3 places that I count.


In an effort not to let this slow us down, I will remove these calls
from the original patch and repost.

> > > > @@ -178,8 +189,11 @@
> > > >  apr_status_t apr_thread_join(apr_status_t *retval, apr_thread_t *thd)
{
> > > >      apr_status_t stat;
> > > > +    apr_status_t *thrstat = NULL;
> > > >
> > > > -    if ((stat = pthread_join(*thd->td,(void *)&retval)) == 0)
{
> > > > +    if ((stat = pthread_join(*thd->td,(void **)&thrstat)) == 0)
{
> > > > +        if (retval != NULL && thrstat != NULL)
> > > > +            *retval = *thrstat;
> > >
> > > And again, this could have been fixed by just adding a * to the
> > > (void *), couldn't it?
> >
> > The cast just gets rid of the warning, but doesn't change the value
> > of the parameter. pthread_join() will set thrstat to point to the
> > value returned from apr_thread_exit(), but that is just a
> > pass-by-value parameter so it never makes it back out to the
> > caller. That is why it didn't work before.
> 
> I must be missing something.  retval is an (apr_status_t *), and thrstat
> is an (apr_status_t *), but one is going to work properly, and the other
> isn't?  I'm sorry, what am I missing?

pthread_join() will set the return value of the thread to the
location pointed to by the second parameter.

1) We pass what amounts to an (int*) to apr_thread_join(). (Actually,
a copy is pushed onto the stack before the call).

2) We pass a pointer to this copied (int*) to pthread_join(), which
promptly sets that (int*) to point to the return value of the thread.

3) apr_thread_join() exits, and our copied (int*) falls out of scope,
never having actually affected the memory at the address in the (int*).

I will supply my original fix in a subsequent patch.


> > Your inspection is valid, but I'm not sure it is in the best
> > interest of the commiters on this list for me to be supplying many
> > smaller and less important patches all day long. The
> > code-test-commit cycle is just way to long for that. In any case, I
> > will try to keep the patches short and sweet in the future.
> 
> Why do you think the code-test-commit cycle is too long?  This patch has
> been out for under one week, which is far shorter than the months that
> others have to wait sometimes.  The thing is, what is in the best interest of
> the committers, is for patches to be reviewed in detail.  Posting five small patches
> that do one thing is a much better way to get patches reviewed.  Shoot, one large
> patch that does one thing can be reviewed easily, but this is a large patch that
> does many things.

My objection is rescinded. I have seen the light of smaller patches. I
apoligise for putting too many things into one patch. I'm not quite used
to this whole OSS thing yet, afterall, one tends to see larger-scale
patches in the corporate settings I'm more used to. :)


> > As for updating this patch, the only problem that I see is there
> > was a variable left over that is no longer is use, and it's
> > declaration should be removed. Do you still want me to revert all
> > the 'thread' identifiers back to '(*new)'??
> 
> See above, I still see more problems than you do.  :-)

I'll resubmit shortly.

-aaron


Mime
View raw message