Re: svn commit: rev 6110 - trunk/subversion/svnserve

[Greg Hudson <gh...@MIT.EDU>]

On Mon, 2003-06-02 at 09:14, philip@tigris.org wrote:
> Add multi-threading to svnserve.  Whether threads are used is a
> run-time decision.

So far, I haven't seen anyone ask for the ability to run svnserve
threaded on Unix, so there's no reason to complicate the code by
providing this option.

And the "-S" option doesn't even work (because of svn diff and svn
merge, and right now because of "svn checkout" due to Ben's latest
work).  So that should definitely go.

I will take care of this.


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: rev 6110 - trunk/subversion/svnserve

[Greg Hudson <gh...@MIT.EDU>]

On Mon, 2003-06-02 at 13:45, Philip Martin wrote:
> Do you consider it worse than in the original patch I posted?

I'm not sure.

> Part of the problem (the only tricky bit in my view) is that the fork
> case follows standard practice for pool reuse, while the thread case
> needs a separate pool for each thread.  Changing the fork case to
> behave like the thread case would simplify things.  I image any
> performance loss due to creating/destroying rather than
> clearing/reusing a pool is negligible in comparison to fork.

Having two different kinds of pool management for the same while loop is
definitely a big part of the reason I don't like the current structure
of the main loop.  I'm fine with changing the fork case to create and
destroy a pool each time, and am willing to review how the code looks
after that change.

On the other hand, the amount of code shared between the fork case and
the thread case seems very small: a call to apr_accept() and an error
check.  So I'm not sure why it wouldn't be better to have two
independent loops.


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: rev 6110 - trunk/subversion/svnserve

[Philip Martin <ph...@codematters.co.uk>]

Greg Hudson <gh...@MIT.EDU> writes:

> But, after hacking on this a bit, I think the level of spaghetti factor
> in the current main loop is totally unacceptable.

Do you consider it worse than in the original patch I posted?

> And I think it would
> take me too long to unravel it.  So I'm going to be unpleasant and
> demand that you either:
>
>   * Eliminate the -S and -F options (-F never had any effect) and

-F has no effect only if fork remains the default.  I see no reason to
change that, but there is little reason to prefer one over the other
as default.

> refactor the code so that there is a completely independent while() loop
> for connection_mode_fork and connection_mode_thread, or
>
>   * Back out your change.  It's not maintainable as-is.

I'll look at it, but I think we will have to disagree on the extent to
which the maintainability has changed.

Part of the problem (the only tricky bit in my view) is that the fork
case follows standard practice for pool reuse, while the thread case
needs a separate pool for each thread.  Changing the fork case to
behave like the thread case would simplify things.  I image any
performance loss due to creating/destroying rather than
clearing/reusing a pool is negligible in comparison to fork.

-- 
Philip Martin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: rev 6110 - trunk/subversion/svnserve

[Greg Hudson <gh...@MIT.EDU>]

On Mon, 2003-06-02 at 12:56, Philip Martin wrote:
> Being able to run multi-threaded svnserve on Unix is the reason I
> wrote the patch!

Okay, the -T option can stay.

But, after hacking on this a bit, I think the level of spaghetti factor
in the current main loop is totally unacceptable.  And I think it would
take me too long to unravel it.  So I'm going to be unpleasant and
demand that you either:

  * Eliminate the -S and -F options (-F never had any effect) and
refactor the code so that there is a completely independent while() loop
for connection_mode_fork and connection_mode_thread, or

  * Back out your change.  It's not maintainable as-is.

It may also be worthwhile to factor out the listen_once mode of
operation into its own little if block before any while loops happen;
that's optional.

If neither APR_HAS_FORK nor APR_HAS_THREADS is defined, the code can
error out or serve one connection at a time, whichever is less code. 
There are no platforms we care about with this characteristic, so the
dominating factor is simplicity.


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: rev 6110 - trunk/subversion/svnserve

[Philip Martin <ph...@codematters.co.uk>]

Greg Hudson <gh...@MIT.EDU> writes:

> On Mon, 2003-06-02 at 09:14, philip@tigris.org wrote:
>> Add multi-threading to svnserve.  Whether threads are used is a
>> run-time decision.
>
> So far, I haven't seen anyone ask for the ability to run svnserve
> threaded on Unix, so there's no reason to complicate the code by
> providing this option.

Being able to run multi-threaded svnserve on Unix is the reason I
wrote the patch!  Most of the Unix testing takes place using multiple
single-threaded processes, to do any multi-threaded testing one needs
to configure Apache with a threaded MPM and then separate the Apache
performance from the Subversion performance.

While writing it I discovered that it is relatively easy to debug a
threaded svnserve--I found it easier than an svnserve that forks
subprocess (I've never managed to get gdb's follow-fork-mode to work).

> And the "-S" option doesn't even work (because of svn diff and svn
> merge, and right now because of "svn checkout" due to Ben's latest
> work).  So that should definitely go.

I have no objections to -S disappearing, I only left it in so that no
existing functionality was removed.

-- 
Philip Martin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: rev 6110 - trunk/subversion/svnserve

[Garrett Rooney <ro...@electricjellyfish.net>]

Greg Hudson wrote:

>On Mon, 2003-06-02 at 09:14, philip@tigris.org wrote:
>  
>
>>Add multi-threading to svnserve.  Whether threads are used is a
>>run-time decision.
>>    
>>
>
>So far, I haven't seen anyone ask for the ability to run svnserve
>threaded on Unix, so there's no reason to complicate the code by
>providing this option.
>

I could see some benefit to it, in that it might make it easier to shut 
down your server without having to track down any running child processes.

>And the "-S" option doesn't even work (because of svn diff and svn
>merge, and right now because of "svn checkout" due to Ben's latest
>work).  So that should definitely go.
>

Yeah, that should probably be eliminated now that there is an alternative.

-garrett


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org