Re: svn commit: r360461 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h include/httpd.h server/protocol.c

["Roy T. Fielding" <fi...@gbiv.com>]

On Dec 31, 2005, at 3:45 PM, brianp@apache.org wrote:

> Author: brianp
> Date: Sat Dec 31 15:45:11 2005
> New Revision: 360461
>
> URL: http://svn.apache.org/viewcvs?rev=360461&view=rev
> Log:
> Refactoring of ap_read_request() to store partial request state
> in the request rec.  The point of this is to allow asynchronous
> MPMs do do nonblocking reads of requests.  (Backported from the
> async-read-dev branch)

Umm, this needs more eyes.

It doesn't seem to me to be doing anything useful.  It just adds
a set of unused input buffer fields to the wrong memory structure,
resulting in what should be a minor (not major) MMN bump, and then
rearranges a critical-path function into several subroutines.

The nonblocking yield should happen inside ap_rgetline (or its
replacement), not in the calling routine.  The thread has nothing
to do until that call is finished or it times out.  In any case,
this code should be independent of the MPM and no MPM is going
to do something useful with a partial HTTP request.

I say -1 to adding the input buffer variables to the request_rec.
Those variables can be local to the input loop.  I don't see any
point in placing this on trunk until it can do something useful.

....Roy

Re: svn commit: r360461 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h include/httpd.h server/protocol.c

[Brian Pane <br...@apache.org>]

On Jan 2, 2006, at 2:14 PM, Roy T. Fielding wrote:

> On Jan 2, 2006, at 1:37 PM, Brian Pane wrote:
>
>> "Significantly faster than prefork" has never been a litmus test for
>> assessing new features, and I'm -1 for adding it now.  A reasonable
>> technical metric for validating the async changes would  
>> "significantly
>> more scalable than the 2.2 Event MPM" or "memory footprint
>> competitive with IIS/Zeus/phttpd/one's-competitive-benchmark-of- 
>> choice."
>
> Those aren't features.  They are properties of the resulting system
> assuming all goes well.
>
>> The bit about degrading the rest of the server is a wonderful sound
>> bite, but we need to engineer the httpd based on data, not FUD.
>
> I said leave it on the async branch until you have data.  You moved
> it to trunk before you've even implemented the async part, which I
> think is wrong because the way you implemented it damages the
> performance of prefork and needlessly creates an incompatible MMN

You have yet to present any data backing up the assertion that
it "damages the performance of prefork."  (Repeating the claim
doesn't count as a proof.)  Having looked at the compiler's
inlining of the code that's been factored into separate functions,
I'm rather skeptical of the claim.

The "incompatible MMN" point is puzzling, to say the least.  As the
4th MMN change in the past quarter, this was by no means an
unusual event.  And on an unreleased development codeline,
the impact to production sites and 3rd party module developers
is near zero.

Brian

Re: svn commit: r360461 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h include/httpd.h server/protocol.c

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

Roy T. Fielding wrote:
>
> Alternatively, rewrite the server to remove all MPMs other than
> event and then show that the new server is better than our existing
> server, and we can adopt that for 3.0.

Well that's a bit silly, leave the others for those who must have an entirely
non-threaded server or other options; but prove that event is more performant
and quite stable for all platforms, if they would only take the time to use
threaded-compatible 3rd party modules, then make event the default 3.0 mpm...

Now that's a metric I'll buy.

Bill

Re: svn commit: r360461 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h include/httpd.h server/protocol.c

["Roy T. Fielding" <fi...@gbiv.com>]

On Jan 3, 2006, at 12:02 AM, William A. Rowe, Jr. wrote:

> Roy T. Fielding wrote:
>> On Jan 2, 2006, at 2:14 PM, Roy T. Fielding wrote:
>>> Now, if you want to tell me that those changes produced a net
>>> performance benefit on prefork (and thus are applicable to other   
>>> MPMs),
>>> then I am all ears.  I am easily convinced by comparative  
>>> performance
>>> figures when the comparison is meaningful.
>
> lol, of course you choose the non-threaded MPM as a reference,  
> which therefore
> should receive no meaningful performance change.  The difference  
> between an
> async wakeup and a poll result should be null for one socket pool,  
> one process,
> one thread (of course select is a differently ugly beast, and if  
> there were
> a platform that supported async with no poll, I'd laugh.)

You seem to misunderstand me -- if I compare two prefork servers, one
with the change and one without the change, and the one with the change
performs better (by whatever various measures of performance you can  
test),
then that is an argument for making the change regardless of the other
concerns.

If, instead, you say that the change improves the event MPM by 10% and
degrades performance on prefork by 1%, then I am -1 on that change.
Prefork is our workhorse MPM.  The task then is to isolate MPM-specific
changes so that they have no significant impact on the critical path
of our primary MPM, even if that means using #ifdefs.

Alternatively, rewrite the server to remove all MPMs other than
event and then show that the new server is better than our existing
server, and we can adopt that for 3.0.

>> BTW, part of the reason I say that is because I have considered
>> replacing the same code with something that doesn't parse the
>> header fields until the request header/body separator line is
>> seen, since that would allow the entire request header to be parsed
>> in-place for the common case.
>
> Well ... you are using protocol knowledge that will render us http- 
> bound
> when it comes time to efficently bind waka (no crlf delims in a  
> binary format
> protocol, no?) or ftp (pushes a 'greeting' before going back to  
> sleep.)

Well, I am assuming that the MIME header parsing code is in the
protocol-specific part of the server, yes.

....Roy

Re: svn commit: r360461 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h include/httpd.h server/protocol.c

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

Roy T. Fielding wrote:
> On Jan 2, 2006, at 2:14 PM, Roy T. Fielding wrote:
> 
>> Now, if you want to tell me that those changes produced a net
>> performance benefit on prefork (and thus are applicable to other  MPMs),
>> then I am all ears.  I am easily convinced by comparative performance
>> figures when the comparison is meaningful.

lol, of course you choose the non-threaded MPM as a reference, which therefore
should receive no meaningful performance change.  The difference between an
async wakeup and a poll result should be null for one socket pool, one process,
one thread (of course select is a differently ugly beast, and if there were
a platform that supported async with no poll, I'd laugh.)

> BTW, part of the reason I say that is because I have considered
> replacing the same code with something that doesn't parse the
> header fields until the request header/body separator line is
> seen, since that would allow the entire request header to be parsed
> in-place for the common case.

Well ... you are using protocol knowledge that will render us http-bound
when it comes time to efficently bind waka (no crlf delims in a binary format
protocol, no?) or ftp (pushes a 'greeting' before going back to sleep.)

But I'd agree these changes are radical enough that maintaining the async
branch a while longer is probably a good thing.

Bill

Re: svn commit: r360461 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h include/httpd.h server/protocol.c

["Roy T. Fielding" <fi...@gbiv.com>]

On Jan 2, 2006, at 2:14 PM, Roy T. Fielding wrote:

> Now, if you want to tell me that those changes produced a net
> performance benefit on prefork (and thus are applicable to other  
> MPMs),
> then I am all ears.  I am easily convinced by comparative performance
> figures when the comparison is meaningful.

BTW, part of the reason I say that is because I have considered
replacing the same code with something that doesn't parse the
header fields until the request header/body separator line is
seen, since that would allow the entire request header to be parsed
in-place for the common case.

....Roy

Re: svn commit: r360461 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h include/httpd.h server/protocol.c

["Roy T. Fielding" <fi...@gbiv.com>]

On Jan 2, 2006, at 1:37 PM, Brian Pane wrote:

> "Significantly faster than prefork" has never been a litmus test for
> assessing new features, and I'm -1 for adding it now.  A reasonable
> technical metric for validating the async changes would "significantly
> more scalable than the 2.2 Event MPM" or "memory footprint
> competitive with IIS/Zeus/phttpd/one's-competitive-benchmark-of- 
> choice."

Those aren't features.  They are properties of the resulting system
assuming all goes well.

> The bit about degrading the rest of the server is a wonderful sound
> bite, but we need to engineer the httpd based on data, not FUD.

I said leave it on the async branch until you have data.  You moved
it to trunk before you've even implemented the async part, which I
think is wrong because the way you implemented it damages the
performance of prefork and needlessly creates an incompatible MMN.
Maybe it would be easier for me to understand why the event loop is
being controlled at such a high level if I could see it work first.

Now, if you want to tell me that those changes produced a net
performance benefit on prefork (and thus are applicable to other MPMs),
then I am all ears.  I am easily convinced by comparative performance
figures when the comparison is meaningful.

....Roy

Re: svn commit: r360461 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h include/httpd.h server/protocol.c

[Brian Pane <br...@apache.org>]

On Jan 2, 2006, at 11:52 AM, Roy T. Fielding wrote:

>> It would be feasible to build up the pending request in a structure
>> other than the request_rec, so that ap_read_async_request() can
>> operate on, say, an ap_partial_request_t instead of a request_rec.
>> My preference so far, though, has been to leave the responsibility
>> for knowing how to parse request headers encapsulated within
>> the request_rec and its associated "methods."
>
> Maybe you should just keep those changes on the async branch for now.
> The rest of the server cannot be allowed to degrade just because you
> want to introduce a new MPM.  After the async branch is proven to be
> significantly faster than prefork, then we can evaluate whether or
> not the additional complications are worth it.

"Significantly faster than prefork" has never been a litmus test for
assessing new features, and I'm -1 for adding it now.  A reasonable
technical metric for validating the async changes would "significantly
more scalable than the 2.2 Event MPM" or "memory footprint
competitive with IIS/Zeus/phttpd/one's-competitive-benchmark-of-choice."

The bit about degrading the rest of the server is a wonderful sound
bite, but we need to engineer the httpd based on data, not FUD.

Brian

Re: svn commit: r360461 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h include/httpd.h server/protocol.c

["Roy T. Fielding" <fi...@gbiv.com>]

On Dec 31, 2005, at 9:55 PM, Brian Pane wrote:
> On Dec 31, 2005, at 6:50 PM, Roy T. Fielding wrote:
>
>> The nonblocking yield should happen inside ap_rgetline (or its
>> replacement), not in the calling routine.  The thread has nothing
>> to do until that call is finished or it times out.
>
> On the contrary, the thread has some very important work to do before
> that call finishes or times out: it has other connections to  
> process.  If
> the thread waits until the ap_rgetline completes, a server  
> configuration
> sized for multiple threads per connection will be vulnerable to a  
> trivially
> implementable DoS.

When I say "thread", I mean a single stream of control with execution
stack, not OS process/thread.  An event MPM is going to have a single
stream of control per event, right?  What I am saying is that the
control should block in rgetline and yield (return to the event pool)
inside that function.  That way, the complications due to yielding are
limited to the I/O routines that might block a thread rather than
being spread all over the server code.

Am I missing something?  This is not a new topic -- Dean Gaudet had
quite a few rants on the subject in the archives.

>>  In any case,
>> this code should be independent of the MPM and no MPM is going
>> to do something useful with a partial HTTP request.
>>
>> I say -1 to adding the input buffer variables to the request_rec.
>> Those variables can be local to the input loop.
>
> Are you proposing that the buffers literally become local variables?
> That generally won't work; the input loop isn't necessarily contained
> within a single function call, and the reading of a single request's
> input can cross threads.

I am saying it doesn't belong in the request_rec.  It belongs on the
execution stack that the yield routine has to save in order to return
to this execution path on the next event.  The request does not care
about partial lines.

> It would be feasible to build up the pending request in a structure
> other than the request_rec, so that ap_read_async_request() can
> operate on, say, an ap_partial_request_t instead of a request_rec.
> My preference so far, though, has been to leave the responsibility
> for knowing how to parse request headers encapsulated within
> the request_rec and its associated "methods."

Maybe you should just keep those changes on the async branch for now.
The rest of the server cannot be allowed to degrade just because you
want to introduce a new MPM.  After the async branch is proven to be
significantly faster than prefork, then we can evaluate whether or
not the additional complications are worth it.

....Roy

Re: svn commit: r360461 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h include/httpd.h server/protocol.c

[Brian Pane <br...@apache.org>]

On Dec 31, 2005, at 6:50 PM, Roy T. Fielding wrote:

> The nonblocking yield should happen inside ap_rgetline (or its
> replacement), not in the calling routine.  The thread has nothing
> to do until that call is finished or it times out.

On the contrary, the thread has some very important work to do before
that call finishes or times out: it has other connections to  
process.  If
the thread waits until the ap_rgetline completes, a server configuration
sized for multiple threads per connection will be vulnerable to a  
trivially
implementable DoS.

>  In any case,
> this code should be independent of the MPM and no MPM is going
> to do something useful with a partial HTTP request.
>
> I say -1 to adding the input buffer variables to the request_rec.
> Those variables can be local to the input loop.

Are you proposing that the buffers literally become local variables?
That generally won't work; the input loop isn't necessarily contained
within a single function call, and the reading of a single request's
input can cross threads.

It would be feasible to build up the pending request in a structure
other than the request_rec, so that ap_read_async_request() can
operate on, say, an ap_partial_request_t instead of a request_rec.
My preference so far, though, has been to leave the responsibility
for knowing how to parse request headers encapsulated within
the request_rec and its associated "methods."

Brian