apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "William A. Rowe, Jr." <wr...@apache.org>
Subject Re: submission of new Windows rwlock code
Date Wed, 18 Jun 2003 20:46:14 GMT
At 07:05 PM 6/17/2003, Marc M. Adkins wrote:
>I've re-coded the Windows rwlock based on the OS/2 algorithm implemented by
>Brian Havard.  My test program doesn't show starvation with this algorithm.
>I'm assuming that the code is stable and works correctly on OS/2, and it's
>essentially the same code here, plus it passes all the tests I know how to
>do so I'm submitting the code for review.

Great work!  Consider this reviewed+=1.

Mr. Stoddard was correct, the patch needed style updates (I'll discard my
edited copies and replace them with his :-)  Not that we don't like your own
preferred style, but as a coder working in a dozen different code bases over
the course of a week, strong style affinity within a project helps keep my
mind on the right page.

I have a few other small kibitzes.  OS2 is unique from Win32 in that it will
generally provide the return value from the function, not a separate error
lookup.  In general we've used apr_get_os_error() which does apr_status_t 
conversion + GetLastError().  Stoddard was guilty of the same this a.m. :-)

>Somewhat to my surprise the performance test program that comes with APR
>seems to show an increase in speed of 30% - 50%.  Possibly because the old
>code had two mutex waits for a read lock and the new code has one.
>I was initially surprised because the new code has tests on return values
>and an extra level of subroutine calling not present in the old code.  Not
>to mention additional functionality:  tryrdlock and trywrlock are both
>working now and starvation should be prevented.  When does more
>functionality ever mean faster?

The magic of well written locking logic on single CPU boxes :-)

>I'm attaching the two changed files, which were essentially rewritten.  So
>far as I can tell, the 2.1 code is still the same in CVS, so just replacing
>the files in their entirety should be sufficient.  I can do diffs if that is

In general we prefer cvs diff -u3 as the format, even when a file will change
substantially.  Remember that folks a year from now will be trying to follow
the delta from one commit to another, between an older and newer release.
Please always try to minimize gratuitous whitespace and decorative changes,
so that the cvs diff's are as clean as possible.

>I don't personally have CVS write access.

At the rate you are going this may change sooner rather than later :-)  Please
try to observe the style guidelines, as that is a chief issue when considering
when to grant our consistent contributors their own cvs write access.


Thanks again for this rewrite, and thanks to Brian for the original logic!


View raw message