apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ryan Bloom <...@covalent.net>
Subject Re: [PATCH] new lock APIs, rwlocks, condition variables
Date Tue, 04 Sep 2001 04:53:26 GMT
On Monday 03 September 2001 21:08, Aaron Bannert wrote:
> Before anyone vetoes this patch merely based on size (I know it's
> huge) please take these things into consideration (patch description
> to follow):
>
>  - This patch _changes NO existing functionality_, it merely adds a new
>    lock API in parallel with the existing lock API.
>
>  - Most of the code of this patch is copied directly (with name changes
>    to adapt to the new types) from current code.
>
>  - I've spent the entire weekend testing this code, and although I can't
>    yet say it's as fast (2-5% slower) as the old code (some places are
>    still lagging, haven't figured out why), I would not be posting it
>    here unless I was confident enough in it's functional correctness
>    for public review.
>
>
> Now, for the summary:
>
>  - Adds a new lock api, breaking out functionality for each of the
> following new types:
>
>      apr_thread_mutex_t     (aka INTRAPROCESS apr_lock_t)
>      apr_proc_mutex_t       (aka CROSSPROCESS apr_lock_t)
>      apr_thread_rwlock_t
>      apr_thread_cond_t      (* this finally gives us condition variables!
> :)
>
>  - From my rather extensive testing, this is in my opinion as functional
>    as the old apr_lock_t api. I have tested the new functions in modified
>    httpd-2.0 and found them to operate correctly (although surprisingly
>    slightly slower in the various MPMs :( I'm still working though -- and
>    with this patch we get more eyes staring at the code).
>
>  - As a side effect I've found a way to speed up a thread_mutex a wee bit.
>
> Caveats:
>
>  - Not yet implemented for beos, netware, os2, win32. My patch provides
>    stubs that simply return APR_ENOTIMPL. It should be fairly trivial for
>    someone from each platform to "port" the old code to the new API.
>
>  - No API is yet provided for the apr_lock_data_get/apr_lock_data_set
>    functions, which will be required before we are able to fully use
>    the API in httpd [for SysV Semaphores, it appears].
>
>
> I have added some tests to the apr/test/testlock.c routines for testing
> the new functionality, but have left them out of this patch for brevity.
> I will post it along with a new test I have created that performs some
> simple test timings on the new locks, in a followup post.
>
> -aaron

I have a couple of comments.  :-)

Overall, with just a quick review, +1 for the concept, I have a few issues with
the implementation.  Most of my problem is the size of the patch, the good
thing is that this patch is mostly empty declarations so that we keep
compiling.

The problem I have, is that this is actually doing two things.  1)  It re-writes the
locking API.  2)  It introduces a new condition variable API.  Can we get the patch
split in two?  To make this a bit easier, I would also recommend that the condition
variable API belongs in it's own file in the locks subdirectory.

Adding that API to the thread locks API that is already there is making the code
harder to read.  If you split it out, then you can re-post the entire patch in 
two pieces.  One, the current API re-org, which can be generated by just doing
a CVS diff.  The other the condition variable API, which can be done by just 
posting the files with the API to the list.

If you can do that, I can review this patch tomorrow or Tuesday, and hopefully
apply it the same day.  Also, very nice that this doesn't remove any of the
current API.  That makes this patch very easy to apply, because I don't have
to worry about breaking things on half our platforms.  :-)

Ryan

______________________________________________________________
Ryan Bloom				rbb@apache.org
Covalent Technologies			rbb@covalent.net
--------------------------------------------------------------

Mime
View raw message