Duplicate code.

[Ryan Bloom <rb...@raleigh.ibm.com>]

We have a real problem with duplicate code between MPM's.  I am currently
changing the fprintf's to ap_log_error's within the MPM's, and what I keep
seeing, is the same error message surrounded by the same if statements in
the same functions in EVERY MPM directory.

The scoreboard and accept locking code seem to be the biggest offenders,
but they are by no means the only offenders.  Can we PLEASE do something
to get the common code together in a single file.  I would personally
suggest ripping the scoreboard and acceptlock files out of either dexter
or mpmt_pthread, and moving it to src/main.  Then, remove all of the
duplicated code, and use the functions that are provided in the files that
we just moved.  I REALLY think MPM's need to be MUCH smaller than they are
now.  

The idea itself is wonderful, but our execution has MPM's implementing all
of the helper functions that used to be common code.  Why can't MPM's just
be responsible for spawning threads and creating processes, and mapping
requests to those execution primitives it sees fit.  Anything else is
superflous, and most likely common to at least two MPM's.

I have ranted about this before, and I will rant again.  :-)

Ryan

_______________________________________________________________________
Ryan Bloom		rbb@raleigh.ibm.com
4205 S Miami Blvd	
RTP, NC 27709		It's a beautiful sight to see good dancers 
			doing simple steps.  It's a painful sight to
			see beginners doing complicated patterns.	

Re: Duplicate code.

[Ryan Bloom <rb...@raleigh.ibm.com>]

> This reminds me, does APR work on systems without threads and locks
> yet?

APR has always worked on systems without threads.  You don't get
APR_HAS_THREADS.  If you want to simulate that, configure APR with
--disable-threads or --enable-threads=no.  If your system doesn't have
threads, I don't try to create cross-thread locks, no.  APR does always
have cross-process locks however.  I have not yet modified the configure
process enough to allow you to build without other sections of APR.
Threads is the only module that has this option right now.

Ryan

_______________________________________________________________________
Ryan Bloom		rbb@raleigh.ibm.com
4205 S Miami Blvd	
RTP, NC 27709		It's a beautiful sight to see good dancers 
			doing simple steps.  It's a painful sight to
			see beginners doing complicated patterns.	

Re: Duplicate code.

[Manoj Kasichainula <ma...@io.com>]

On Fri, Nov 19, 1999 at 03:03:45PM -0500, Ryan Bloom wrote:
> Every MPM has a section for ap_thread_mutex functions.  None of them are
> used as near as I can tell.  We never once in ANY of the MPM's call
> ap_thread_mutex_new.  The code is useless and it's duplicated.  

The original purpose of those calls was to allow the core code to
make mutex calls around critical sections when using a threaded MPM,
and to not try to do so otherwise, e.g. prefork.  IIRC, this was only
used in alloc.c. When the server was moved to APR pools, those
functions didn't get deleted. Go ahead and do it now.

This reminds me, does APR work on systems without threads and locks
yet?

> MPM tree, and didn't get a chance to finish it.  MPM's
> should be tiny things.  They should implement VERY VERY little.

I'm a very big fan of MPMs being small. But, for example, the Windows
MPM has to have special socket handling code to do its cool
asynchronous I/O features. Eventually (after 2.0 I think), when I or
someone else starts putting select-based serving into the Unix MPMs,
they will have to do more than just thread and process handling.

-- 
Manoj Kasichainula - manojk at io dot com - http://www.io.com/~manojk/

Re: Duplicate code.

[Ryan Bloom <rb...@raleigh.ibm.com>]

Would you like another example?

Every MPM has a section for ap_thread_mutex functions.  None of them are
used as near as I can tell.  We never once in ANY of the MPM's call
ap_thread_mutex_new.  The code is useless and it's duplicated.  

I think the reason we have this problem (and this is not blame), is that
Dean started the MPM tree, and didn't get a chance to finish it.  MPM's
should be tiny things.  They should implement VERY VERY little.  In fact,
most MPM's should be ONE file, IMHO.  We took what Dean gave us, and
rather than refine it and make it smaller and tighter and cleaner, we just
ran with it.  Great.  But now, we have to either clean it up, or watch it
just get bigger.

Ryan

On Fri, 19 Nov 1999, Manoj Kasichainula wrote:

> On Fri, Nov 19, 1999 at 10:16:33AM -0500, Ryan Bloom wrote:
> > The scoreboard and accept locking code seem to be the biggest offenders,
> 
> Ummm, as of Monday, the accept lock code is based on APR for the two
> threaded Unix MPMs. Prefork still needs work, but that shouldn't be
> too difficult.
> 
> You're seeing duplicate messages, because that's what you're working
> on, but that's really the only significant accept lock piece
> duplicated between mpmt_pthread and dexter. I don't think it's worth
> the effort to create a common ap_lock_with_error_report call.
> 
> And besides, I'm hoping to take cross-process locking out of dexter
> completely by taking advantage of non-blocking accept.
> 
> The scoreboard code is somewhat duplicated, but it's doing somewhat
> different things in the various Unix MPMs. In prefork and
> mpmt_pthread, it contains all the old-style scoreboard code. Dexter
> doesn't use the old-style scoreboard at all, so it just has generic
> code to create a block of shared memory, and another chunk of code to
> use it to maintain connection status. I think it would be useful to do
> a similar split of the scoreboard code in the other MPMs, but the
> proper way to do this is to get shared memory code into APR.
> 
> > The idea itself is wonderful, but our execution has MPM's implementing all
> > of the helper functions that used to be common code.  Why can't MPM's just
> > be responsible for spawning threads and creating processes, and mapping
> > requests to those execution primitives it sees fit.  Anything else is
> > superflous, and most likely common to at least two MPM's.
> 
> The Unix MPMs already have piles of common code, see unixd. Sure, more
> stuff could be moved, but the low hanging fruit has been picked
> already. 
> 
> And MPMs will soon have to manage event-based I/O at some point too.
> -- 
> Manoj Kasichainula - manojk at io dot com - http://www.io.com/~manojk/
> 

_______________________________________________________________________
Ryan Bloom		rbb@raleigh.ibm.com
4205 S Miami Blvd	
RTP, NC 27709		It's a beautiful sight to see good dancers 
			doing simple steps.  It's a painful sight to
			see beginners doing complicated patterns.	

Re: Duplicate code.

[Manoj Kasichainula <ma...@io.com>]

On Fri, Nov 19, 1999 at 10:16:33AM -0500, Ryan Bloom wrote:
> The scoreboard and accept locking code seem to be the biggest offenders,

Ummm, as of Monday, the accept lock code is based on APR for the two
threaded Unix MPMs. Prefork still needs work, but that shouldn't be
too difficult.

You're seeing duplicate messages, because that's what you're working
on, but that's really the only significant accept lock piece
duplicated between mpmt_pthread and dexter. I don't think it's worth
the effort to create a common ap_lock_with_error_report call.

And besides, I'm hoping to take cross-process locking out of dexter
completely by taking advantage of non-blocking accept.

The scoreboard code is somewhat duplicated, but it's doing somewhat
different things in the various Unix MPMs. In prefork and
mpmt_pthread, it contains all the old-style scoreboard code. Dexter
doesn't use the old-style scoreboard at all, so it just has generic
code to create a block of shared memory, and another chunk of code to
use it to maintain connection status. I think it would be useful to do
a similar split of the scoreboard code in the other MPMs, but the
proper way to do this is to get shared memory code into APR.

> The idea itself is wonderful, but our execution has MPM's implementing all
> of the helper functions that used to be common code.  Why can't MPM's just
> be responsible for spawning threads and creating processes, and mapping
> requests to those execution primitives it sees fit.  Anything else is
> superflous, and most likely common to at least two MPM's.

The Unix MPMs already have piles of common code, see unixd. Sure, more
stuff could be moved, but the low hanging fruit has been picked
already. 

And MPMs will soon have to manage event-based I/O at some point too.
-- 
Manoj Kasichainula - manojk at io dot com - http://www.io.com/~manojk/

Re: Duplicate code.

[Ryan Bloom <rb...@raleigh.ibm.com>]

> Surely (most of) the accept locking should be in APR? Since it is just a
> generic mutex?

Most of it should be changed to use APR, but it doesn't yet.  Even so,
there is plenty of other duplicated code in those MPM's.  Take a look at
the directives.  All of the threaded MPM's share multiple directives.
There is only ONE non-threaded MPM.  Combine those directives to one file,
amd don't have the prefork MPM use that file.  There is a LOT of
duplicated code.  As I said, most of it is scoreboard and acceptlock, but
not all.

Ryan

_______________________________________________________________________
Ryan Bloom		rbb@raleigh.ibm.com
4205 S Miami Blvd	
RTP, NC 27709		It's a beautiful sight to see good dancers 
			doing simple steps.  It's a painful sight to
			see beginners doing complicated patterns.	

Re: Duplicate code.

[Ben Laurie <be...@algroup.co.uk>]

Ryan Bloom wrote:
> 
> We have a real problem with duplicate code between MPM's.  I am currently
> changing the fprintf's to ap_log_error's within the MPM's, and what I keep
> seeing, is the same error message surrounded by the same if statements in
> the same functions in EVERY MPM directory.
> 
> The scoreboard and accept locking code seem to be the biggest offenders,
> but they are by no means the only offenders.  Can we PLEASE do something
> to get the common code together in a single file.  I would personally
> suggest ripping the scoreboard and acceptlock files out of either dexter
> or mpmt_pthread, and moving it to src/main.  Then, remove all of the
> duplicated code, and use the functions that are provided in the files that
> we just moved.  I REALLY think MPM's need to be MUCH smaller than they are
> now.

Surely (most of) the accept locking should be in APR? Since it is just a
generic mutex?

Cheers,

Ben.

--
http://www.apache-ssl.org/ben.html

"My grandfather once told me that there are two kinds of people: those
who work and those who take the credit. He told me to try to be in the
first group; there was less competition there."
     - Indira Gandhi