You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Jeff Trawick <tr...@attglobal.net> on 2002/07/03 13:56:44 UTC
[PATCH] speed up network timeout processing
A little bird told me that FD_ZERO() burns lots of cycles in
apr_wait_for_io_or_timeout(). It turns out that this is an easy
conversion to poll(), which doesn't have such overhead in the
interface.
This works for me with some testing (timeouts on read and write work
for me).
--- /tmp/sendrecv.c Wed Jul 3 07:19:33 2002
+++ network_io/unix/sendrecv.c Wed Jul 3 06:42:41 2002
@@ -61,6 +61,33 @@
apr_status_t apr_wait_for_io_or_timeout(apr_socket_t *sock, int for_read)
{
+#ifdef HAVE_POLL
+ int rc, timeout;
+ struct pollfd fd;
+
+ if (sock->timeout < 0) {
+ timeout = -1;
+ }
+ else {
+ timeout = sock->timeout / 1000; /* convert usecs to msecs */
+ }
+
+ fd.fd = sock->socketdes;
+ fd.events = for_read ? POLLIN|POLLRDNORM|POLLRDBAND|POLLPRI :
+ POLLOUT;
+
+ do {
+ rc = poll(&fd, 1, timeout);
+ } while (rc == -1 && errno == EINTR);
+
+ if (rc == 0) {
+ return APR_TIMEUP;
+ }
+ else if (rc < 0) {
+ return errno;
+ }
+ return APR_SUCCESS;
+#else /* HAVE_POLL */
struct timeval tv, *tvptr;
fd_set fdset;
int srv;
@@ -91,6 +118,7 @@
return errno;
}
return APR_SUCCESS;
+#endif /* HAVE_POLL */
}
apr_status_t apr_send(apr_socket_t *sock, const char *buf, apr_size_t *len)
--
Jeff Trawick | trawick@attglobal.net
Born in Roswell... married an alien...
RE: [PATCH] speed up network timeout processing
Posted by Sander Striker <st...@apache.org>.
> From: Brian Pane [mailto:brianp@apache.org]
> Sent: 03 July 2002 18:05
>>You are missing the point. If apr_poll() is to be useful to external
>>projects, then it must perform well. If it performs so poorly that we
>>refuse to use it inside of APR, then it couldn't possibly be useful to
>>external projects. That is straight-line reasoning in my mind.
>
> My concern is that you're obscuring a fundamental point: different
> applications have different performance needs. Sometimes a lightweight
> library function performs adequately for the target application.
> Sometimes you have to inline to implementation because you're using
> it in code where speed is more critical. In order to make apr_poll()
> suitable for *all* applications, from a performance perspective, we'd
> have to turn the entire function into a macro so it can be inlined.
> But that's a bad idea for a lot of other reasons, so we leave it a
> function so that it meets the needs of 99% of the code that might
> want to use it. The other 1%, the 1% with the most extreme performance
> concerns, will always need a custom solution. That's no different from
> our use of libc: we try to use a function in the public API if it's
> helpful to do so, but we write an inline equivalent in cases where
> we need the highest possible speed or specialized semantics.
Could someone please profile this function and see where the problems
are? That way we know why we wish to bypass it inside APR (or know
what to fix in apr_poll).
Thanks,
Sander
RE: [PATCH] speed up network timeout processing
Posted by Ryan Bloom <rb...@covalent.net>.
> From: Brian Pane [mailto:brianp@apache.org]
>
> Ryan Bloom wrote:
>
> >>From: trawick@rdu88-250-182.nc.rr.com [mailto:trawick@rdu88-250-
> >>
> >>"Ryan Bloom" <rb...@covalent.net> writes:
> >>
> >>
> >>
> >>>>From: trawick@rdu88-250-182.nc.rr.com [mailto:trawick@rdu88-250-
> >>>>
> >>>>"Ryan Bloom" <rb...@covalent.net> writes:
> >>>>
> >>>>
> >>>>
> >>>>>>From: trawick@rdu88-250-182.nc.rr.com
> >>>>>>
> >>>>>>
> >[mailto:trawick@rdu88-250-
> >
> >You are missing the point. If apr_poll() is to be useful to external
> >projects, then it must perform well. If it performs so poorly that
we
> >refuse to use it inside of APR, then it couldn't possibly be useful
to
> >external projects. That is straight-line reasoning in my mind.
> >
>
> My concern is that you're obscuring a fundamental point: different
> applications have different performance needs. Sometimes a
lightweight
> library function performs adequately for the target application.
But the whole point of poll() is that it performs better than select.
That is the only reason the poll() API was ever created. If apr_poll()
doesn't meet that need, then we have a major problem. Add to it that
the performance problems with apr_poll() have been brought up on this
list before, and I believe you have a compelling reason to fix the
function.
This all comes down to eating our own dog food, which does multiple
things. 1) It helps us ensure optimal performance and 2) it helps us
find bugs.
> Sometimes you have to inline to implementation because you're using
> it in code where speed is more critical. In order to make apr_poll()
> suitable for *all* applications, from a performance perspective, we'd
> have to turn the entire function into a macro so it can be inlined.
> But that's a bad idea for a lot of other reasons, so we leave it a
> function so that it meets the needs of 99% of the code that might
> want to use it. The other 1%, the 1% with the most extreme
performance
> concerns, will always need a custom solution. That's no different
from
> our use of libc: we try to use a function in the public API if it's
> helpful to do so, but we write an inline equivalent in cases where
> we need the highest possible speed or specialized semantics.
I still have the question, have we even tried profiling with
apr_poll???? If the answer is no, then we are falling into the common
trap of assuming poor performance without any real numbers. If yes,
then let's look at the implementation of apr_poll and see where the
problems really lie and if they are fixable.
> >Why in the world would we leave an API in APR that performs so poorly
> >that we refuse to use it in our own library? Doing that boggles my
> >mind.
>
> We've made no promise to users of APR that APR functions will be
> implemented as calls to other APR functions, and IMHO it wouldn't be
> helpful to users if we ever did so. The promise that APR makes to
> its users should be: "each function in this library's public API
> is as well-tuned as we can make it." That's much more useful to
> a customer of the library than a promise "each function in this
> library is implemented via calls to other functions in this library."
We have made the promise that using APR will not seriously impact the
performance of your app when compared to not using it. If apr_poll() is
so poor from a performance viewpoint that it has major overhead when
compared to poll(), then we haven't met that promise.
Ryan
Re: [PATCH] speed up network timeout processing
Posted by Brian Pane <br...@apache.org>.
Ryan Bloom wrote:
>>From: trawick@rdu88-250-182.nc.rr.com [mailto:trawick@rdu88-250-
>>
>>"Ryan Bloom" <rb...@covalent.net> writes:
>>
>>
>>
>>>>From: trawick@rdu88-250-182.nc.rr.com [mailto:trawick@rdu88-250-
>>>>
>>>>"Ryan Bloom" <rb...@covalent.net> writes:
>>>>
>>>>
>>>>
>>>>>>From: trawick@rdu88-250-182.nc.rr.com
>>>>>>
>>>>>>
>[mailto:trawick@rdu88-250-
>
>
>>>>>>A little bird told me that FD_ZERO() burns lots of cycles in
>>>>>>apr_wait_for_io_or_timeout(). It turns out that this is an
>>>>>>
>>>>>>
>easy
>
>
>>>>>>conversion to poll(), which doesn't have such overhead in the
>>>>>>interface.
>>>>>>
>>>>>>This works for me with some testing (timeouts on read and
>>>>>>
>>>>>>
>write
>
>
>>>work
>>>
>>>
>>>>>>for me).
>>>>>>
>>>>>>
>>>>>Can we remove the #ifdef's by just using apr_poll here?
>>>>>
>>>>>
>>>>I'd rather we not, since that introduces a fair amount of extra
>>>>overhead.
>>>>
>>>>
>>>Then let's get rid of the overhead.
>>>
>>>
>>redesign the API
>>
>>
>
>The redesign the API, but FIX the performance problem!
>
>
>
>>> If we don't use apr_poll,
>>>
>>>
>then
>
>
>>the
>>
>>
>>>overhead is maintenance,
>>>
>>>
>>the marginal extra maintenance is certainly something I can live with
>>here... this is an important path within APR... if we can use the
>>most efficient mechanism without much extra maintenance then we
>>should...
>>
>>
>
>You are missing the point. If apr_poll() is to be useful to external
>projects, then it must perform well. If it performs so poorly that we
>refuse to use it inside of APR, then it couldn't possibly be useful to
>external projects. That is straight-line reasoning in my mind.
>
My concern is that you're obscuring a fundamental point: different
applications have different performance needs. Sometimes a lightweight
library function performs adequately for the target application.
Sometimes you have to inline to implementation because you're using
it in code where speed is more critical. In order to make apr_poll()
suitable for *all* applications, from a performance perspective, we'd
have to turn the entire function into a macro so it can be inlined.
But that's a bad idea for a lot of other reasons, so we leave it a
function so that it meets the needs of 99% of the code that might
want to use it. The other 1%, the 1% with the most extreme performance
concerns, will always need a custom solution. That's no different from
our use of libc: we try to use a function in the public API if it's
helpful to do so, but we write an inline equivalent in cases where
we need the highest possible speed or specialized semantics.
>Why in the world would we leave an API in APR that performs so poorly
>that we refuse to use it in our own library? Doing that boggles my
>mind.
>
>
We've made no promise to users of APR that APR functions will be
implemented as calls to other APR functions, and IMHO it wouldn't be
helpful to users if we ever did so. The promise that APR makes to
its users should be: "each function in this library's public API
is as well-tuned as we can make it." That's much more useful to
a customer of the library than a promise "each function in this
library is implemented via calls to other functions in this library."
--Brian
RE: [PATCH] speed up network timeout processing
Posted by Ryan Bloom <rb...@covalent.net>.
> From: trawick@rdu88-250-182.nc.rr.com [mailto:trawick@rdu88-250-
>
> "Ryan Bloom" <rb...@covalent.net> writes:
>
> > > From: trawick@rdu88-250-182.nc.rr.com [mailto:trawick@rdu88-250-
> > >
> > > "Ryan Bloom" <rb...@covalent.net> writes:
> > >
> > > > > From: trawick@rdu88-250-182.nc.rr.com
[mailto:trawick@rdu88-250-
> > > > >
> > > > > A little bird told me that FD_ZERO() burns lots of cycles in
> > > > > apr_wait_for_io_or_timeout(). It turns out that this is an
easy
> > > > > conversion to poll(), which doesn't have such overhead in the
> > > > > interface.
> > > > >
> > > > > This works for me with some testing (timeouts on read and
write
> > work
> > > > > for me).
> > > >
> > > > Can we remove the #ifdef's by just using apr_poll here?
> > >
> > > I'd rather we not, since that introduces a fair amount of extra
> > > overhead.
> >
> > Then let's get rid of the overhead.
>
> redesign the API
The redesign the API, but FIX the performance problem!
> > If we don't use apr_poll,
then
> the
> > overhead is maintenance,
>
> the marginal extra maintenance is certainly something I can live with
> here... this is an important path within APR... if we can use the
> most efficient mechanism without much extra maintenance then we
> should...
You are missing the point. If apr_poll() is to be useful to external
projects, then it must perform well. If it performs so poorly that we
refuse to use it inside of APR, then it couldn't possibly be useful to
external projects. That is straight-line reasoning in my mind.
Why in the world would we leave an API in APR that performs so poorly
that we refuse to use it in our own library? Doing that boggles my
mind.
Ryan
Re: [PATCH] speed up network timeout processing
Posted by Jeff Trawick <tr...@attglobal.net>.
"Ryan Bloom" <rb...@covalent.net> writes:
> > From: trawick@rdu88-250-182.nc.rr.com [mailto:trawick@rdu88-250-
> >
> > "Ryan Bloom" <rb...@covalent.net> writes:
> >
> > > > From: trawick@rdu88-250-182.nc.rr.com [mailto:trawick@rdu88-250-
> > > >
> > > > A little bird told me that FD_ZERO() burns lots of cycles in
> > > > apr_wait_for_io_or_timeout(). It turns out that this is an easy
> > > > conversion to poll(), which doesn't have such overhead in the
> > > > interface.
> > > >
> > > > This works for me with some testing (timeouts on read and write
> work
> > > > for me).
> > >
> > > Can we remove the #ifdef's by just using apr_poll here?
> >
> > I'd rather we not, since that introduces a fair amount of extra
> > overhead.
>
> Then let's get rid of the overhead.
redesign the API
> If we don't use apr_poll, then the
> overhead is maintenance,
the marginal extra maintenance is certainly something I can live with
here... this is an important path within APR... if we can use the
most efficient mechanism without much extra maintenance then we
should...
--
Jeff Trawick | trawick@attglobal.net
Born in Roswell... married an alien...
RE: New apr_poll() implementation
Posted by Ryan Bloom <rb...@covalent.net>.
> From: Sander Striker [mailto:striker@apache.org]
>
> > From: Ryan Bloom [mailto:rbb@covalent.net]
> > Sent: 09 July 2002 15:46
>
> >> From: Ryan Bloom [mailto:rbb@covalent.net]
> >>> From: Justin Erenkrantz [mailto:jerenkrantz@apache.org]
> >>> On Sat, Jul 06, 2002 at 12:11:59PM -0700, Ryan Bloom wrote:
> >>>> parameters. I would like to fix that mistake for apr_poll now,
as
> >>>> long as we are changing the implementation.
> >>>
> >>> Getting back to this conversation for a brief second, I think the
> >>> additional parameter with the fd count is unneeded (but for a
> >>> different reason than IN/OUT). The count should be stored in
> >>> apr_pollfd_t - it does not need to be passed into apr_poll().
> >>
> >> The count should absolutely NOT be stored in apr_pollfd_t. That is
> >> user owned structure, not an APR owned structure.
> >
> > I should clarify this. The whole point of this change, is that the
user
> > can do the following:
>
> [...]
> > That code will work. Notice that no where before the apr_poll, did
we
> > tell the poll implementation the size of the pollset. Nor can we.
This
> > is where the real performance benefits of this patch come in.
Because
> > the user has complete control over the pollset, we can remove all
but
> > one pool allocation, and that one can be removed with an
optimization
> > for small pollsets.
> >
> > Now, we can make macros to add sockets and files, but we should not
EVER
> > be telling APR how many descriptors we are polling until we call
> > apr_poll(). The functions that currently setup the pollset and add
> > sockets and set events and revents are all deprecated, and were only
> > left in the patch to allow for as much source backwards compat as
> > possible while the patch is being discussed. Once the patch has
been
> > committed, I will go through the source and start to remove those
> > functions completely. The only real example of apr_poll() that is a
> > part of the patch is wait_for_io_or_timeout. I probably didn't make
> > that clear enough.
>
> Ah, no. Without the accessor functions it doesn't make much sense
> to store the pollset size in apr_pollfd_t. I surely didn't see
removing
> the accessor functions comming. Not sure if I like that (+0, -0). I
can
> see the perf benefit, but if it makes for a better API, I don't know.
You can keep the accessor functions if you want to, but you don't need
them, and they shouldn't be functions regardless. The whole point of
the new poll implementation is that it opens up the structure for
anybody to manage the memory. If we keep the accessors, then we should
use macros not functions.
Either way, the setup function goes away, because the user can allocate
the memory, and there is no place to give the size of the structure to
the apr_pollfd_t before the call to apr_poll().
Ryan
RE: New apr_poll() implementation
Posted by Sander Striker <st...@apache.org>.
> From: Ryan Bloom [mailto:rbb@covalent.net]
> Sent: 09 July 2002 15:46
>> From: Ryan Bloom [mailto:rbb@covalent.net]
>>> From: Justin Erenkrantz [mailto:jerenkrantz@apache.org]
>>> On Sat, Jul 06, 2002 at 12:11:59PM -0700, Ryan Bloom wrote:
>>>> parameters. I would like to fix that mistake for apr_poll now, as
>>>> long as we are changing the implementation.
>>>
>>> Getting back to this conversation for a brief second, I think the
>>> additional parameter with the fd count is unneeded (but for a
>>> different reason than IN/OUT). The count should be stored in
>>> apr_pollfd_t - it does not need to be passed into apr_poll().
>>
>> The count should absolutely NOT be stored in apr_pollfd_t. That is
>> user owned structure, not an APR owned structure.
>
> I should clarify this. The whole point of this change, is that the user
> can do the following:
[...]
> That code will work. Notice that no where before the apr_poll, did we
> tell the poll implementation the size of the pollset. Nor can we. This
> is where the real performance benefits of this patch come in. Because
> the user has complete control over the pollset, we can remove all but
> one pool allocation, and that one can be removed with an optimization
> for small pollsets.
>
> Now, we can make macros to add sockets and files, but we should not EVER
> be telling APR how many descriptors we are polling until we call
> apr_poll(). The functions that currently setup the pollset and add
> sockets and set events and revents are all deprecated, and were only
> left in the patch to allow for as much source backwards compat as
> possible while the patch is being discussed. Once the patch has been
> committed, I will go through the source and start to remove those
> functions completely. The only real example of apr_poll() that is a
> part of the patch is wait_for_io_or_timeout. I probably didn't make
> that clear enough.
Ah, no. Without the accessor functions it doesn't make much sense
to store the pollset size in apr_pollfd_t. I surely didn't see removing
the accessor functions comming. Not sure if I like that (+0, -0). I can
see the perf benefit, but if it makes for a better API, I don't know.
Oh well, my $0.02,
Sander
RE: New apr_poll() implementation
Posted by Ryan Bloom <rb...@covalent.net>.
> From: Ryan Bloom [mailto:rbb@covalent.net]
>
> > From: Justin Erenkrantz [mailto:jerenkrantz@apache.org]
> >
> > On Sat, Jul 06, 2002 at 12:11:59PM -0700, Ryan Bloom wrote:
> > > parameters. I would like to fix that mistake for apr_poll now, as
> long
> > > as we are changing the implementation.
> >
> > Getting back to this conversation for a brief second, I think the
> > additional parameter with the fd count is unneeded (but for a
> > different reason than IN/OUT). The count should be stored in
> > apr_pollfd_t - it does not need to be passed into apr_poll().
>
> The count should absolutely NOT be stored in apr_pollfd_t. That is
user
> owned structure, not an APR owned structure.
I should clarify this. The whole point of this change, is that the user
can do the following:
Apr_pollfd_t pollset[2];
Pollset[0].desc_type = APR_POLL_FILE;
Pollset[0].desc.file = f1;
Pollset[0].events = APR_POLLIN;
Pollset[1].desc_type = APR_POLL_SOCKET;
Pollset[1].desc.sock = s1;
Pollset[0].events = APR_POLLIN;
Apr_poll(pollset, 2, &n, pool);
That code will work. Notice that no where before the apr_poll, did we
tell the poll implementation the size of the pollset. Nor can we. This
is where the real performance benefits of this patch come in. Because
the user has complete control over the pollset, we can remove all but
one pool allocation, and that one can be removed with an optimization
for small pollsets.
Now, we can make macros to add sockets and files, but we should not EVER
be telling APR how many descriptors we are polling until we call
apr_poll(). The functions that currently setup the pollset and add
sockets and set events and revents are all deprecated, and were only
left in the patch to allow for as much source backwards compat as
possible while the patch is being discussed. Once the patch has been
committed, I will go through the source and start to remove those
functions completely. The only real example of apr_poll() that is a
part of the patch is wait_for_io_or_timeout. I probably didn't make
that clear enough.
Ryan
RE: New apr_poll() implementation
Posted by Ryan Bloom <rb...@covalent.net>.
> From: Justin Erenkrantz [mailto:jerenkrantz@apache.org]
>
> On Sat, Jul 06, 2002 at 12:11:59PM -0700, Ryan Bloom wrote:
> > parameters. I would like to fix that mistake for apr_poll now, as
long
> > as we are changing the implementation.
>
> Getting back to this conversation for a brief second, I think the
> additional parameter with the fd count is unneeded (but for a
> different reason than IN/OUT). The count should be stored in
> apr_pollfd_t - it does not need to be passed into apr_poll().
The count should absolutely NOT be stored in apr_pollfd_t. That is user
owned structure, not an APR owned structure.
> Since you have to call apr_poll_socket_{add|remove}, you can't
> populate apr_pollfd_t on your own (which is something you can do
> with pollfd, hence why poll() needs the count). apr_poll_setup()
> should take the maximal size and init the counter to 0. As fds
> are added or removed, the counter is adjusted.
The apr_poll_socket_(add|remove) are deprecated, and actually aren't and
shouldn't be used. The whole point of this patch, and the reason it
performs better is that the apr_pollfd_t structure is transparent, and
it is meant to be allocated and owned by the user.
> BTW, we also need apr_poll_file_{add|remove} (seems to be missing
> from rbb's patch). -- Justin
We don't need it, although we may create it, as a macro. As proof that
we don't need either however, look at wait_for_io_or_timeout. That
function is a consumer of apr_poll now, and it acts just like any
consumer outside of APR should act. It does not add sockets or files
with functions.
The only reason the other functions are still in the code, is for
backwards source compat. They have been marked as deprecated in my
tree, although I did it after I posted the patch, because I forgot about
it.
Ryan
RE: New apr_poll() implementation
Posted by Sander Striker <st...@apache.org>.
> From: Justin Erenkrantz [mailto:jerenkrantz@apache.org]
> Sent: 09 July 2002 09:44
> On Sat, Jul 06, 2002 at 12:11:59PM -0700, Ryan Bloom wrote:
> > parameters. I would like to fix that mistake for apr_poll now, as long
> > as we are changing the implementation.
>
> Getting back to this conversation for a brief second, I think the
> additional parameter with the fd count is unneeded (but for a
> different reason than IN/OUT). The count should be stored in
> apr_pollfd_t - it does not need to be passed into apr_poll().
+1. It doesn't seem to make sense to keep track of that in the
application. Mismatch of actual fd count and passed in fd count
is best prevented.
> Since you have to call apr_poll_socket_{add|remove}, you can't
> populate apr_pollfd_t on your own (which is something you can do
> with pollfd, hence why poll() needs the count). apr_poll_setup()
> should take the maximal size and init the counter to 0. As fds
> are added or removed, the counter is adjusted.
Makes sense, +1.
> BTW, we also need apr_poll_file_{add|remove} (seems to be missing
> from rbb's patch). -- justin
Sander
Re: New apr_poll() implementation
Posted by Justin Erenkrantz <je...@apache.org>.
On Sat, Jul 06, 2002 at 12:11:59PM -0700, Ryan Bloom wrote:
> parameters. I would like to fix that mistake for apr_poll now, as long
> as we are changing the implementation.
Getting back to this conversation for a brief second, I think the
additional parameter with the fd count is unneeded (but for a
different reason than IN/OUT). The count should be stored in
apr_pollfd_t - it does not need to be passed into apr_poll().
Since you have to call apr_poll_socket_{add|remove}, you can't
populate apr_pollfd_t on your own (which is something you can do
with pollfd, hence why poll() needs the count). apr_poll_setup()
should take the maximal size and init the counter to 0. As fds
are added or removed, the counter is adjusted.
BTW, we also need apr_poll_file_{add|remove} (seems to be missing
from rbb's patch). -- justin
Re: New apr_poll() implementation was Re: [PATCH] speed up
network timeout processing
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 11:47 AM 7/7/2002, Aaron Bannert wrote:
>On Sat, Jul 06, 2002 at 12:11:59PM -0700, Ryan Bloom wrote:
> > I vote to fix the API so that these kinds of mistakes can't happen in
> > the future.
>
>+1, let's fix the API now. (I also strongly dislike input/output parameters.)
Microsoft's Win32 API was built on IN,OUT arguments. They are confusing,
you lose the original value ... when you needed to hang on to it you needed
to copy to another variable before you invoked the call. It is much simpler
to deal with the other case;
int somefunc(int inarg, int *outarg)
and pass
somefunc (foo, &foo)
when you don't care about the original value.
Since they have become far more OO/IDL/COM/.NET centric, very, very few
new functions are introduced as [IN,OUT], most are IN foo, OUT bar instead.
They learned from their mistake, so should we :-)
Bill
Re: New apr_poll() implementation was Re: [PATCH] speed up network timeout processing
Posted by Aaron Bannert <aa...@clove.org>.
On Sat, Jul 06, 2002 at 12:11:59PM -0700, Ryan Bloom wrote:
> I vote to fix the API so that these kinds of mistakes can't happen in
> the future.
+1, let's fix the API now. (I also strongly dislike input/output parameters.)
-aaron
RE: New apr_poll() implementation was Re: [PATCH] speed up network timeout processing
Posted by Ryan Bloom <rb...@covalent.net>.
> From: Justin Erenkrantz [mailto:jerenkrantz@apache.org]
>
> On Sat, Jul 06, 2002 at 12:11:59PM -0700, Ryan Bloom wrote:
> > I vote to fix the API so that these kinds of mistakes can't happen
in
> > the future. I made a lot of mistakes when I designed APR (even
though
> > Manoj tried to convince me I was wrong). One of those mistakes is
> > having functions use a single variable for both input and result
> > parameters. I would like to fix that mistake for apr_poll now, as
long
> > as we are changing the implementation.
>
> I don't think that was a mistake. You are adding extra parameters
> where it just confuses the API. I see no problem with using the
> same parameter on input/output - provided that it has the same
> meaning on input and output (same as apr_file_read()).
It is a mistake, and it is confusing, as should be obvious, because
people screwed up.
> Regardless, why don't we just fix apr_poll() and then discuss
> whether we should change/fix the API? -- Justin
Because I want more opinions before I commit, for one. And for another,
I want to fix it right. Multiple commits doesn't make sense. Have the
discussion, and then we can commit. Waiting until Monday or Tuesday
isn't going to hurt anything, and I would like Jeff and Bill to have an
opportunity to comment. Not to mention that I still need time to fix
the other platforms versions of apr_poll. Wheil the API doesn't have to
change, the implementation is changing, and committing on just one
platform won't work in this case.
Ryan
Re: New apr_poll() implementation was Re: [PATCH] speed up network timeout processing
Posted by Justin Erenkrantz <je...@apache.org>.
On Sat, Jul 06, 2002 at 12:11:59PM -0700, Ryan Bloom wrote:
> I vote to fix the API so that these kinds of mistakes can't happen in
> the future. I made a lot of mistakes when I designed APR (even though
> Manoj tried to convince me I was wrong). One of those mistakes is
> having functions use a single variable for both input and result
> parameters. I would like to fix that mistake for apr_poll now, as long
> as we are changing the implementation.
I don't think that was a mistake. You are adding extra parameters
where it just confuses the API. I see no problem with using the
same parameter on input/output - provided that it has the same
meaning on input and output (same as apr_file_read()).
And, it does here, as on input, *nsds is the number of fds to
poll and, on output, *nsds is the number of fds polled. The
typical Unix convention is to return the *nsds as the output of
the function, but APR has explicit error codes, so we can't do
that. I don't see a problem with the API and I'm not convinced
that we should change it.
Regardless, why don't we just fix apr_poll() and then discuss
whether we should change/fix the API? -- justin
RE: New apr_poll() implementation was Re: [PATCH] speed up network timeout processing
Posted by Ryan Bloom <rb...@covalent.net>.
> From: Justin Erenkrantz [mailto:jerenkrantz@apache.org]
>
> On Sat, Jul 06, 2002 at 08:32:18AM -0700, Ryan Bloom wrote:
> > Because if Apache can't get it right, then I am assuming that nobody
> > else can either. I had originally coded it to use *nsds just as you
> > describe below, and it didn't pass any tests, because throughout the
> > code people were passing 0 as *nsds. I decided to fix an API
problem
> > that I created years ago by using the same variable for both input
and
> > output parameters in this patch. I am not tied to adding num, but I
do
> > believe that it is the correct approach.
>
> I vote to fix httpd rather than add an extra parameter. Perhaps we
> need to make it clearer that *nsds is the number of fd's on input.
> But, I know that I was under that impression by reading the docs.
> However, I can see where people were confused if they read the docs.
I vote to fix the API so that these kinds of mistakes can't happen in
the future. I made a lot of mistakes when I designed APR (even though
Manoj tried to convince me I was wrong). One of those mistakes is
having functions use a single variable for both input and result
parameters. I would like to fix that mistake for apr_poll now, as long
as we are changing the implementation.
BTW, I should also mention that the APR_POLL_LASTDESC enum is only there
for backwards compat. Most code that uses apr_poll would never use that
enum.
Ryan
Re: New apr_poll() implementation was Re: [PATCH] speed up network timeout processing
Posted by Justin Erenkrantz <je...@apache.org>.
On Sat, Jul 06, 2002 at 08:32:18AM -0700, Ryan Bloom wrote:
> Because if Apache can't get it right, then I am assuming that nobody
> else can either. I had originally coded it to use *nsds just as you
> describe below, and it didn't pass any tests, because throughout the
> code people were passing 0 as *nsds. I decided to fix an API problem
> that I created years ago by using the same variable for both input and
> output parameters in this patch. I am not tied to adding num, but I do
> believe that it is the correct approach.
I vote to fix httpd rather than add an extra parameter. Perhaps we
need to make it clearer that *nsds is the number of fd's on input.
But, I know that I was under that impression by reading the docs.
However, I can see where people were confused if they read the docs.
> The small array on the stack is an optimization that I was discussing
> with bpane last night. It would require some testing to find the right
> number, and I wasn't ready to do that. I wanted to get the
> implementation out there, and I figured we could continue to optimize it
> once it was done.
Cool. I'd bet any number >2 will work for our purposes in httpd
as the common case seems to be 2. But, I do recall 6 being used
somewhere and I can't fathom where I saw that. -- justin
RE: New apr_poll() implementation was Re: [PATCH] speed up network timeout processing
Posted by Ryan Bloom <rb...@covalent.net>.
> From: Justin Erenkrantz [mailto:jerenkrantz@apache.org]
>
> On Fri, Jul 05, 2002 at 08:47:32PM -0400, rbb@apache.org wrote:
> > This also creates a support library for APR, this is basically just
a
> > series of functions that APR can use internally to get the job
> > done. Since wait_for_io_or_timeout was identical between files and
> > sockets, I have moved that function to the support lib, it also now
uses
> > apr_poll().
>
> What's the rationale behind a 'support library' for APR? All of the
> content in newpoll.tar.gz seems like it should just stay in APR. You
> have me thoroughly confused here. Perhaps in a new directory, but
> definitely not a separate library.
Support library may have been the wrong words. It is just a set of
functions for use inside of APR that don't belong to any other category
of APR functions. It is implemented as a directory inside of APR, but
the functions aren't meant to be used outside of APR.
> I'm also not clear what the additional num parameter to apr_poll is
> buying us. Why not just use the *nsds value on input as your num
> parameter? The number of descriptors read is what *nsds is on the
> output. No signature change required.
Because if Apache can't get it right, then I am assuming that nobody
else can either. I had originally coded it to use *nsds just as you
describe below, and it didn't pass any tests, because throughout the
code people were passing 0 as *nsds. I decided to fix an API problem
that I created years ago by using the same variable for both input and
output parameters in this patch. I am not tied to adding num, but I do
believe that it is the correct approach.
> So, why not do something like this (very rough):
>
> (Some OS does the low magic number off the stack, but I can't
> remember which for the life of me.)
>
> apr_status_t apr_poll(apr_pollfd_t *aprset, apr_int32_t *nsds,
> apr_interval_time_t timeout)
> {
> int i, rv;
> struct pollfd *pollset;
> struct pollfd stack_pollset[SOME_LOW_MAGIC_NUMBER_LIKE_SAY_6];
The small array on the stack is an optimization that I was discussing
with bpane last night. It would require some testing to find the right
number, and I wasn't ready to do that. I wanted to get the
implementation out there, and I figured we could continue to optimize it
once it was done.
Ryan
New apr_poll() implementation was Re: [PATCH] speed up network timeout processing
Posted by Justin Erenkrantz <je...@apache.org>.
On Fri, Jul 05, 2002 at 08:47:32PM -0400, rbb@apache.org wrote:
> This also creates a support library for APR, this is basically just a
> series of functions that APR can use internally to get the job
> done. Since wait_for_io_or_timeout was identical between files and
> sockets, I have moved that function to the support lib, it also now uses
> apr_poll().
What's the rationale behind a 'support library' for APR? All of the
content in newpoll.tar.gz seems like it should just stay in APR. You
have me thoroughly confused here. Perhaps in a new directory, but
definitely not a separate library.
I'm also not clear what the additional num parameter to apr_poll is
buying us. Why not just use the *nsds value on input as your num
parameter? The number of descriptors read is what *nsds is on the
output. No signature change required.
Aha, it seems that httpd uses a broken apr_poll() semantic. If you
read the documentation for apr_poll, it says:
* @param nsds The number of sockets we are polling.
...
* The number of sockets signalled is returned in the second argument (nsds)
But, httpd-2.0 doesn't pass in the number of sockets we are polling.
flood does because I read the docs not the code. I think we should
switch the code to match the docs.
I know I took it to mean that nsds on input is the number of fds we
are polling and on completion, it is the number of signaled fds. I
believe that the docs have the right API.
So, why not do something like this (very rough):
(Some OS does the low magic number off the stack, but I can't
remember which for the life of me.)
apr_status_t apr_poll(apr_pollfd_t *aprset, apr_int32_t *nsds,
apr_interval_time_t timeout)
{
int i, rv;
struct pollfd *pollset;
struct pollfd stack_pollset[SOME_LOW_MAGIC_NUMBER_LIKE_SAY_6];
if (*nsds < SOME_LOW_MAGIC_NUMBER_LIKE_SAY_6) {
pollset = &stack_pollset;
}
else {
pollset = apr_palloc(aprset->p,
sizeof(struct pollfd) * *nsds);
}
for (i = 0; i < *nsds; i++) {
if (aprset[i].desc_type == APR_POLL_SOCKET) {
pollset[i].fd = aprset[i].desc.s->socketdes;
}
else if (aprset[i].desc_type == APR_POLL_FILE) {
pollset[i].fd = aprset[i].desc.f->filedes;
}
pollset[i].events = get_event(aprset[i].events);
}
if (timeout > 0) {
/* FIXME: need apr conversion macro from apr_interval_time_t
* to milliseconds.
*/
timeout /= 1000; /* convert microseconds to milliseconds */
}
rv = poll(pollset, *nsds, timeout);
if (rv < 0) {
return errno;
}
for (i = 0; i < *nsds; i++) {
aprset[i].revents = get_revent(pollset[i].revents);
}
*nsds = rv;
return APR_SUCCESS;
}
It seems that you might have originally meant to go down this route
because you have:
struct pollfd *pollset = apr_palloc(aprset->p,
sizeof(struct pollfd) * *nsds);
Which means that the pollset is allocated to contain *nsds
pollfds. And, I think that means you need to have *nsds to be
equivalent to num, or you will segfault. (Perhaps it hasn't in
your tests by sheer luck.) -- justin
RE: [PATCH] speed up network timeout processing
Posted by Ryan Bloom <rb...@covalent.net>.
> From: Brian Pane [mailto:bpane@pacbell.net]
>
> rbb@apache.org wrote:
>
> >As promised, a new implementation of apr_poll, which should improve
> >performance. I have only implemented the HAVE_POLL case so far, but
the
> >API is 99% the same, so I have no fear that the others are not
> >possible. In fact, the ONLY change to the function API, is that
> >apr_poll() itself takes one more argument. The types however are now
> >complete, and apr_poll can be used on both files and sockets (as well
as
> >anything else we want to add).
> >
>
> I really like the basic design concept for this. It provides a
> good abstraction that would allow us to plug in /dev/poll in the
> future if needed.
>
> A couple of suggestions on the details:
>
> * Instead of having apr_poll_setup just create an array of poll
> objects, it may be more flexible to have it create a wrapper object
> that contains the array. (Just in case we need to add anything else
> to it in the future, like a file handle for /dev/poll.)
I considered a wrapper object that contains the array, but that is what
we started with. Having a wrapper object means that the user either
needs to manage both the wrapper and the actual object, or we need to
have a set of functions to create the inner object. Either one seemed
more complex than I wanted it to be. Since the whole point of poll is
to provide ultimate performance, I wanted to give the user control over
the memory as much as possible.
As for /dev/poll support, the point of apr_poll() in this design, is
that function converts from the apr_pollfd_t to the interior functions.
I don't have a great answer for how to /dev/poll, but my initial answer
would be to add a void pointer to the apr_pollfd_t for different
apr_poll() implementations to store arbitrary data.
> * The apr_poll_socket_add function looks like it's O(n). That could
be
> a problem if we ever need to poll a large number of descriptors
(like
> in an event-loop server). Adding a faster lookup would make the
data
> structure more complicated, though. Maybe it's best to just stick
> with the O(n) array approach for now, but encapsulate the data
> structures
> so that we can swap in a new design later if needed?
It is O(n), but it should never be used. The fastest way to add sockets
or files to the pollset is to add them directly. We can and should make
it easier to do this by creating a couple of macros.
Both of those functions have been marked as deprecated. The intent is
that people will create their apr_pollfd_t array themselves and they
will own maintaining it. I have only left those functions around for
backwards compat, because I didn't feel like fixing all of Apache today.
Thanks for the feedback.
Ryan
Re: [PATCH] speed up network timeout processing
Posted by Brian Pane <bp...@pacbell.net>.
rbb@apache.org wrote:
>As promised, a new implementation of apr_poll, which should improve
>performance. I have only implemented the HAVE_POLL case so far, but the
>API is 99% the same, so I have no fear that the others are not
>possible. In fact, the ONLY change to the function API, is that
>apr_poll() itself takes one more argument. The types however are now
>complete, and apr_poll can be used on both files and sockets (as well as
>anything else we want to add).
>
I really like the basic design concept for this. It provides a
good abstraction that would allow us to plug in /dev/poll in the
future if needed.
A couple of suggestions on the details:
* Instead of having apr_poll_setup just create an array of poll
objects, it may be more flexible to have it create a wrapper object
that contains the array. (Just in case we need to add anything else
to it in the future, like a file handle for /dev/poll.)
* The apr_poll_socket_add function looks like it's O(n). That could be
a problem if we ever need to poll a large number of descriptors (like
in an event-loop server). Adding a faster lookup would make the data
structure more complicated, though. Maybe it's best to just stick
with the O(n) array approach for now, but encapsulate the data structures
so that we can swap in a new design later if needed?
--Brian
Re: [PATCH] speed up network timeout processing
Posted by rb...@apache.org.
Oh, one more advantage to this code, is that most of the
apr_poll() support functions can either go away, or they can be merged to
a common implementation. The only function that won't be common is
apr_poll() itself.
Ryan
Re: [PATCH] speed up network timeout processing
Posted by rb...@apache.org.
As promised, a new implementation of apr_poll, which should improve
performance. I have only implemented the HAVE_POLL case so far, but the
API is 99% the same, so I have no fear that the others are not
possible. In fact, the ONLY change to the function API, is that
apr_poll() itself takes one more argument. The types however are now
complete, and apr_poll can be used on both files and sockets (as well as
anything else we want to add).
This also creates a support library for APR, this is basically just a
series of functions that APR can use internally to get the job
done. Since wait_for_io_or_timeout was identical between files and
sockets, I have moved that function to the support lib, it also now uses
apr_poll().
The patch below includes all of the changes for both APR and Apache, and
while it has been tested, I haven't really stressed it yet. Also, I did
the bare minimum for most of Apache, which means that most of the Apache
code is still using the deprecated functions (which I forgot to mark as
such, whoops). To get the most performance boost, when the
apr_poll() logic is committed, Apache should migrate to controlling the
memory itself.
There is still one pool allocation in the code, I can already see how to
remove it for the common case of a single file descriptor, but I am trying
to find a way to remove it for all cases.
To apply this patch, apply the patch below from httpd-2.0, and un-tar
the attached tarball from within srclib/apr. You will need to re-run
buildconf and configure to make this work.
Ryan
Index: modules/experimental/mod_ext_filter.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/experimental/mod_ext_filter.c,v
retrieving revision 1.31
diff -u -d -b -w -u -r1.31 mod_ext_filter.c
--- modules/experimental/mod_ext_filter.c 28 Jun 2002 08:40:24 -0000 1.31
+++ modules/experimental/mod_ext_filter.c 6 Jul 2002 00:39:25 -0000
@@ -71,6 +71,7 @@
#include "apr_strings.h"
#include "apr_hash.h"
#include "apr_lib.h"
+#include "apr_poll.h"
#define APR_WANT_STRFUNC
#include "apr_want.h"
@@ -626,7 +627,7 @@
#if APR_FILES_AS_SOCKETS
int num_events;
- rv = apr_poll(ctx->pollset,
+ rv = apr_poll(ctx->pollset, 2
&num_events,
f->r->server->timeout);
if (rv || dc->debug >= DBGLVL_GORY) {
Index: modules/proxy/proxy_connect.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/proxy/proxy_connect.c,v
retrieving revision 1.59
diff -u -d -b -w -u -r1.59 proxy_connect.c
--- modules/proxy/proxy_connect.c 17 May 2002 11:24:16 -0000 1.59
+++ modules/proxy/proxy_connect.c 6 Jul 2002 00:39:25 -0000
@@ -61,6 +61,7 @@
#define CORE_PRIVATE
#include "mod_proxy.h"
+#include "apr_poll.h"
module AP_MODULE_DECLARE_DATA proxy_connect_module;
@@ -320,7 +321,7 @@
while (1) { /* Infinite loop until error (one side closes the connection) */
/* ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server, "proxy: CONNECT: going to sleep (poll)");*/
- if ((rv = apr_poll(pollfd, &pollcnt, -1)) != APR_SUCCESS)
+ if ((rv = apr_poll(pollfd, 2, &pollcnt, -1)) != APR_SUCCESS)
{
apr_socket_close(sock);
ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, "proxy: CONNECT: error apr_poll()");
Index: server/mpm/beos/beos.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/mpm/beos/beos.c,v
retrieving revision 1.97
diff -u -d -b -w -u -r1.97 beos.c
--- server/mpm/beos/beos.c 4 Jul 2002 15:20:52 -0000 1.97
+++ server/mpm/beos/beos.c 6 Jul 2002 00:39:26 -0000
@@ -87,6 +87,7 @@
#include "mpm.h"
#include "mpm_default.h"
#include "apr_thread_mutex.h"
+#include "apr_poll.h"
extern int _kset_fd_limit_(int num);
@@ -421,7 +422,7 @@
apr_int16_t event;
apr_status_t ret;
- ret = apr_poll(pollset, &srv, -1);
+ ret = apr_poll(pollset, num_listening_sockets, &srv, -1);
if (ret != APR_SUCCESS) {
if (APR_STATUS_IS_EINTR(ret)) {
Index: server/mpm/experimental/leader/leader.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/mpm/experimental/leader/leader.c,v
retrieving revision 1.22
diff -u -d -b -w -u -r1.22 leader.c
--- server/mpm/experimental/leader/leader.c 4 Jul 2002 15:20:53 -0000 1.22
+++ server/mpm/experimental/leader/leader.c 6 Jul 2002 00:39:28 -0000
@@ -106,6 +106,7 @@
#include "ap_listen.h"
#include "scoreboard.h"
#include "mpm_default.h"
+#include "apr_poll.h"
#include <signal.h>
#include <limits.h> /* for INT_MAX */
@@ -888,7 +889,7 @@
apr_status_t ret;
apr_int16_t event;
- ret = apr_poll(pollset, &n, -1);
+ ret = apr_poll(pollset, num_listensocks, &n, -1);
if (ret != APR_SUCCESS) {
if (APR_STATUS_IS_EINTR(ret)) {
continue;
Index: server/mpm/experimental/perchild/perchild.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/mpm/experimental/perchild/perchild.c,v
retrieving revision 1.128
diff -u -d -b -w -u -r1.128 perchild.c
--- server/mpm/experimental/perchild/perchild.c 30 Jun 2002 21:59:50 -0000 1.128
+++ server/mpm/experimental/perchild/perchild.c 6 Jul 2002 00:39:29 -0000
@@ -95,6 +95,7 @@
#include "mpm.h"
#include "scoreboard.h"
#include "util_filter.h"
+#include "apr_poll.h"
/* ### should be APR-ized */
#include <poll.h>
@@ -748,7 +749,7 @@
while (!workers_may_exit) {
apr_int16_t event;
- srv = apr_poll(pollset, &n, -1);
+ srv = apr_poll(pollset, num_listensocks, &n, -1);
if (srv != APR_SUCCESS) {
if (APR_STATUS_IS_EINTR(srv)) {
Index: server/mpm/experimental/threadpool/threadpool.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/mpm/experimental/threadpool/threadpool.c,v
retrieving revision 1.12
diff -u -d -b -w -u -r1.12 threadpool.c
--- server/mpm/experimental/threadpool/threadpool.c 4 Jul 2002 15:20:53 -0000 1.12
+++ server/mpm/experimental/threadpool/threadpool.c 6 Jul 2002 00:39:31 -0000
@@ -69,6 +69,7 @@
#include "apr_file_io.h"
#include "apr_thread_proc.h"
#include "apr_signal.h"
+#include "apr_poll.h"
#include "apr_thread_mutex.h"
#include "apr_thread_cond.h"
#include "apr_proc_mutex.h"
@@ -913,7 +914,7 @@
apr_status_t ret;
apr_int16_t event;
- ret = apr_poll(pollset, &n, -1);
+ ret = apr_poll(pollset, num_listensocks, &n, -1);
if (ret != APR_SUCCESS) {
if (APR_STATUS_IS_EINTR(ret)) {
continue;
Index: server/mpm/mpmt_os2/mpmt_os2_child.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/mpm/mpmt_os2/mpmt_os2_child.c,v
retrieving revision 1.22
diff -u -d -b -w -u -r1.22 mpmt_os2_child.c
--- server/mpm/mpmt_os2/mpmt_os2_child.c 17 May 2002 11:11:39 -0000 1.22
+++ server/mpm/mpmt_os2/mpmt_os2_child.c 6 Jul 2002 00:39:31 -0000
@@ -73,6 +73,7 @@
#include "ap_mpm.h"
#include "ap_listen.h"
#include "apr_portable.h"
+#include "apr_poll.h"
#include "mpm_common.h"
#include "apr_strings.h"
#include <os2.h>
@@ -250,7 +251,7 @@
rv = APR_FROM_OS_ERROR(rc);
if (rv == APR_SUCCESS) {
- rv = apr_poll(pollset, &nsds, -1);
+ rv = apr_poll(pollset, num_listeners, &nsds, -1);
DosReleaseMutexSem(ap_mpm_accept_mutex);
}
Index: server/mpm/prefork/prefork.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/mpm/prefork/prefork.c,v
retrieving revision 1.268
diff -u -d -b -w -u -r1.268 prefork.c
--- server/mpm/prefork/prefork.c 4 Jul 2002 15:20:54 -0000 1.268
+++ server/mpm/prefork/prefork.c 6 Jul 2002 00:39:32 -0000
@@ -89,6 +89,7 @@
#include "mpm_common.h"
#include "ap_listen.h"
#include "ap_mmn.h"
+#include "apr_poll.h"
#ifdef HAVE_BSTRING_H
#include <bstring.h> /* for IRIX, FD_SET calls bzero() */
@@ -631,7 +632,7 @@
apr_int16_t event;
apr_int32_t n;
- ret = apr_poll(pollset, &n, -1);
+ ret = apr_poll(pollset, num_listensocks, &n, -1);
if (ret != APR_SUCCESS) {
if (APR_STATUS_IS_EINTR(ret)) {
continue;
Index: server/mpm/worker/worker.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/mpm/worker/worker.c,v
retrieving revision 1.130
diff -u -d -b -w -u -r1.130 worker.c
--- server/mpm/worker/worker.c 4 Jul 2002 22:41:48 -0000 1.130
+++ server/mpm/worker/worker.c 6 Jul 2002 00:39:38 -0000
@@ -71,6 +71,7 @@
#include "apr_signal.h"
#include "apr_thread_mutex.h"
#include "apr_proc_mutex.h"
+#include "apr_poll.h"
#define APR_WANT_STRFUNC
#include "apr_want.h"
@@ -758,7 +759,7 @@
apr_status_t ret;
apr_int16_t event;
- ret = apr_poll(pollset, &n, -1);
+ ret = apr_poll(pollset, num_listensocks, &n, -1);
if (ret != APR_SUCCESS) {
if (APR_STATUS_IS_EINTR(ret)) {
continue;
Index: srclib/apr/configure.in
===================================================================
RCS file: /home/cvs/apr/configure.in,v
retrieving revision 1.460
diff -u -d -b -w -u -r1.460 configure.in
--- srclib/apr/configure.in 2 Jul 2002 21:33:43 -0000 1.460
+++ srclib/apr/configure.in 6 Jul 2002 00:39:38 -0000
@@ -82,7 +82,7 @@
DEFAULT_OSDIR="unix"
echo "(Default will be ${DEFAULT_OSDIR})"
-apr_modules="file_io network_io threadproc misc locks time mmap shmem i18n user memory atomic"
+apr_modules="file_io network_io threadproc misc locks time mmap shmem i18n user memory atomic poll support"
dnl Checks for programs.
AC_PROG_MAKE_SET
Index: srclib/apr/file_io/unix/readwrite.c
===================================================================
RCS file: /home/cvs/apr/file_io/unix/readwrite.c,v
retrieving revision 1.80
diff -u -d -b -w -u -r1.80 readwrite.c
--- srclib/apr/file_io/unix/readwrite.c 4 Jul 2002 23:10:24 -0000 1.80
+++ srclib/apr/file_io/unix/readwrite.c 6 Jul 2002 00:39:38 -0000
@@ -55,49 +55,7 @@
#include "fileio.h"
#include "apr_strings.h"
#include "apr_thread_mutex.h"
-
-/* The only case where we don't use wait_for_io_or_timeout is on
- * pre-BONE BeOS, so this check should be sufficient and simpler */
-#if !BEOS_R5
-#define USE_WAIT_FOR_IO
-#endif
-
-#ifdef USE_WAIT_FOR_IO
-static apr_status_t wait_for_io_or_timeout(apr_file_t *file, int for_read)
-{
- struct timeval tv, *tvptr;
- fd_set fdset;
- int srv;
-
- /* TODO - timeout should be less each time through this loop */
-
- do {
- FD_ZERO(&fdset);
- FD_SET(file->filedes, &fdset);
- if (file->timeout >= 0) {
- tv.tv_sec = apr_time_sec(file->timeout);
- tv.tv_usec = apr_time_usec(file->timeout);
- tvptr = &tv;
- }
- else {
- tvptr = NULL;
- }
- srv = select(file->filedes + 1,
- for_read ? &fdset : NULL,
- for_read ? NULL : &fdset,
- NULL,
- tvptr);
- } while (srv == -1 && errno == EINTR);
-
- if (srv == 0) {
- return APR_TIMEUP;
- }
- else if (srv < 0) {
- return errno;
- }
- return APR_SUCCESS;
-}
-#endif
+#include "apr_support.h"
/* problems:
* 1) ungetchar not used for buffered files
@@ -193,7 +151,7 @@
if (rv == -1 &&
(errno == EAGAIN || errno == EWOULDBLOCK) &&
thefile->timeout != 0) {
- apr_status_t arv = wait_for_io_or_timeout(thefile, 1);
+ apr_status_t arv = wait_for_io_or_timeout(thefile, NULL, 1);
if (arv != APR_SUCCESS) {
*nbytes = bytes_read;
return arv;
@@ -272,7 +230,7 @@
if (rv == (apr_size_t)-1 &&
(errno == EAGAIN || errno == EWOULDBLOCK) &&
thefile->timeout != 0) {
- apr_status_t arv = wait_for_io_or_timeout(thefile, 0);
+ apr_status_t arv = wait_for_io_or_timeout(thefile, NULL, 0);
if (arv != APR_SUCCESS) {
*nbytes = 0;
return arv;
Index: srclib/apr/include/apr_network_io.h
===================================================================
RCS file: /home/cvs/apr/include/apr_network_io.h,v
retrieving revision 1.124
diff -u -d -b -w -u -r1.124 apr_network_io.h
--- srclib/apr/include/apr_network_io.h 5 Jul 2002 17:58:10 -0000 1.124
+++ srclib/apr/include/apr_network_io.h 6 Jul 2002 00:39:38 -0000
@@ -187,7 +187,6 @@
#endif
typedef struct apr_socket_t apr_socket_t;
-typedef struct apr_pollfd_t apr_pollfd_t;
/**
* A structure to encapsulate headers and trailers for apr_sendfile
*/
@@ -634,123 +633,6 @@
APR_DECLARE(int) apr_sockaddr_equal(const apr_sockaddr_t *addr1,
const apr_sockaddr_t *addr2);
-/**
- * Setup the memory required for poll to operate properly
- * @param new_poll The poll structure to be used.
- * @param num The number of socket descriptors to be polled.
- * @param cont The pool to operate on.
- */
-APR_DECLARE(apr_status_t) apr_poll_setup(apr_pollfd_t **new_poll,
- apr_int32_t num,
- apr_pool_t *cont);
-
-/**
- * Poll the sockets in the poll structure
- * @param aprset The poll structure we will be using.
- * @param nsds The number of sockets we are polling.
- * @param timeout The amount of time in microseconds to wait. This is
- * a maximum, not a minimum. If a socket is signalled, we
- * will wake up before this time. A negative number means
- * wait until a socket is signalled.
- * @remark
- * <PRE>
- * The number of sockets signalled is returned in the second argument.
- *
- * This is a blocking call, and it will not return until either a
- * socket has been signalled, or the timeout has expired.
- * </PRE>
- */
-APR_DECLARE(apr_status_t) apr_poll(apr_pollfd_t *aprset, apr_int32_t *nsds,
- apr_interval_time_t timeout);
-
-/**
- * Add a socket to the poll structure.
- * @param aprset The poll structure we will be using.
- * @param socket The socket to add to the current poll structure.
- * @param event The events to look for when we do the poll. One of:
- * <PRE>
- * APR_POLLIN signal if read will not block
- * APR_POLLPRI signal if prioirty data is availble to be read
- * APR_POLLOUT signal if write will not block
- * </PRE>
- */
-APR_DECLARE(apr_status_t) apr_poll_socket_add(apr_pollfd_t *aprset,
- apr_socket_t *sock,
- apr_int16_t event);
-
-/**
- * Modify a socket in the poll structure with mask.
- * @param aprset The poll structure we will be using.
- * @param sock The socket to modify in poll structure.
- * @param events The events to stop looking for during the poll. One of:
- * <PRE>
- * APR_POLLIN signal if read will not block
- * APR_POLLPRI signal if priority data is available to be read
- * APR_POLLOUT signal if write will not block
- * </PRE>
- */
-APR_DECLARE(apr_status_t) apr_poll_socket_mask(apr_pollfd_t *aprset,
- apr_socket_t *sock,
- apr_int16_t events);
-/**
- * Remove a socket from the poll structure.
- * @param aprset The poll structure we will be using.
- * @param sock The socket to remove from the current poll structure.
- */
-APR_DECLARE(apr_status_t) apr_poll_socket_remove(apr_pollfd_t *aprset,
- apr_socket_t *sock);
-
-/**
- * Remove all sockets from the poll structure.
- * @param aprset The poll structure we will be using.
- * @param events The events to clear from all sockets. One of:
- * <PRE>
- * APR_POLLIN signal if read will not block
- * APR_POLLPRI signal if priority data is available to be read
- * APR_POLLOUT signal if write will not block
- * </PRE>
- */
-APR_DECLARE(apr_status_t) apr_poll_socket_clear(apr_pollfd_t *aprset,
- apr_int16_t events);
-
-/**
- * Get the return events for the specified socket.
- * @param event The returned events for the socket. One of:
- * <PRE>
- * APR_POLLIN Data is available to be read
- * APR_POLLPRI Priority data is availble to be read
- * APR_POLLOUT Write will succeed
- * APR_POLLERR An error occurred on the socket
- * APR_POLLHUP The connection has been terminated
- * APR_POLLNVAL This is an invalid socket to poll on.
- * Socket not open.
- * </PRE>
- * @param sock The socket we wish to get information about.
- * @param aprset The poll structure we will be using.
- */
-APR_DECLARE(apr_status_t) apr_poll_revents_get(apr_int16_t *event,
- apr_socket_t *sock,
- apr_pollfd_t *aprset);
-
-/**
- * Return the data associated with the current poll.
- * @param pollfd The currently open pollfd.
- * @param key The key to use for retrieving data associated with a poll struct.
- * @param data The user data associated with the pollfd.
- */
-APR_DECLARE(apr_status_t) apr_poll_data_get(apr_pollfd_t *pollfd,
- const char *key, void *data);
-
-/**
- * Set the data associated with the current poll.
- * @param pollfd The currently open pollfd.
- * @param data The key to associate with the data.
- * @param key The user data to associate with the pollfd.
- * @param cleanup The cleanup function
- */
-APR_DECLARE(apr_status_t) apr_poll_data_set(apr_pollfd_t *pollfd, void *data,
- const char *key,
- apr_status_t (*cleanup)(void *));
#if APR_FILES_AS_SOCKETS || defined(DOXYGEN)
Index: srclib/apr/include/arch/unix/networkio.h
===================================================================
RCS file: /home/cvs/apr/include/arch/unix/networkio.h,v
retrieving revision 1.53
diff -u -d -b -w -u -r1.53 networkio.h
--- srclib/apr/include/arch/unix/networkio.h 1 Apr 2002 14:13:45 -0000 1.53
+++ srclib/apr/include/arch/unix/networkio.h 6 Jul 2002 00:39:38 -0000
@@ -69,12 +69,6 @@
#if APR_HAVE_SYS_UIO_H
#include <sys/uio.h>
#endif
-#ifdef HAVE_SYS_POLL_H
-#include <sys/poll.h>
-#endif
-#ifdef HAVE_POLL_H
-#include <poll.h>
-#endif
#ifdef HAVE_SYS_SELECT_H
#include <sys/select.h>
#endif
@@ -138,32 +132,8 @@
apr_int32_t inherit;
};
-struct apr_pollfd_t {
- apr_pool_t *cntxt;
-#ifdef HAVE_POLL
- struct pollfd *pollset;
- int num;
- int curpos;
-#else
- fd_set *read;
- fd_set *write;
- fd_set *except;
- int highsock;
- fd_set *read_set;
- fd_set *write_set;
- fd_set *except_set;
- apr_int16_t *events;
- apr_int16_t *revents;
-#ifdef BEOS
- int lowsock;
- int ncks;
-#endif
-#endif
-};
-
const char *apr_inet_ntop(int af, const void *src, char *dst, apr_size_t size);
int apr_inet_pton(int af, const char *src, void *dst);
-apr_status_t apr_wait_for_io_or_timeout(apr_socket_t *sock, int for_read);
void apr_sockaddr_vars_set(apr_sockaddr_t *, int, apr_port_t);
#define apr_is_option_set(mask, option) ((mask & option) ==option)
Index: srclib/apr/network_io/unix/Makefile.in
===================================================================
RCS file: /home/cvs/apr/network_io/unix/Makefile.in,v
retrieving revision 1.23
diff -u -d -b -w -u -r1.23 Makefile.in
--- srclib/apr/network_io/unix/Makefile.in 22 Apr 2002 01:24:50 -0000 1.23
+++ srclib/apr/network_io/unix/Makefile.in 6 Jul 2002 00:39:38 -0000
@@ -2,7 +2,6 @@
VPATH = @srcdir@
TARGETS = \
- poll.lo \
sendrecv.lo \
sockets.lo \
sockopt.lo \
Index: srclib/apr/network_io/unix/sendrecv.c
===================================================================
RCS file: /home/cvs/apr/network_io/unix/sendrecv.c,v
retrieving revision 1.85
diff -u -d -b -w -u -r1.85 sendrecv.c
--- srclib/apr/network_io/unix/sendrecv.c 2 Jul 2002 21:33:43 -0000 1.85
+++ srclib/apr/network_io/unix/sendrecv.c 6 Jul 2002 00:39:38 -0000
@@ -53,6 +53,7 @@
*/
#include "networkio.h"
+#include "apr_support.h"
#if APR_HAS_SENDFILE
/* This file is needed to allow us access to the apr_file_t internals. */
@@ -63,40 +64,6 @@
#include <sys/sysctl.h>
#endif
-apr_status_t apr_wait_for_io_or_timeout(apr_socket_t *sock, int for_read)
-{
- struct timeval tv, *tvptr;
- fd_set fdset;
- int srv;
-
- do {
- FD_ZERO(&fdset);
- FD_SET(sock->socketdes, &fdset);
- if (sock->timeout < 0) {
- tvptr = NULL;
- }
- else {
- tv.tv_sec = sock->timeout / APR_USEC_PER_SEC;
- tv.tv_usec = sock->timeout % APR_USEC_PER_SEC;
- tvptr = &tv;
- }
- srv = select(sock->socketdes + 1,
- for_read ? &fdset : NULL,
- for_read ? NULL : &fdset,
- NULL,
- tvptr);
- /* TODO - timeout should be smaller on repeats of this loop */
- } while (srv == -1 && errno == EINTR);
-
- if (srv == 0) {
- return APR_TIMEUP;
- }
- else if (srv < 0) {
- return errno;
- }
- return APR_SUCCESS;
-}
-
apr_status_t apr_send(apr_socket_t *sock, const char *buf, apr_size_t *len)
{
ssize_t rv;
@@ -114,7 +81,7 @@
&& sock->timeout != 0) {
apr_status_t arv;
do_select:
- arv = apr_wait_for_io_or_timeout(sock, 0);
+ arv = apr_wait_for_io_or_timeout(NULL, sock, 0);
if (arv != APR_SUCCESS) {
*len = 0;
return arv;
@@ -153,7 +120,7 @@
if (rv == -1 && (errno == EAGAIN || errno == EWOULDBLOCK) &&
sock->timeout != 0) {
do_select:
- arv = apr_wait_for_io_or_timeout(sock, 1);
+ arv = apr_wait_for_io_or_timeout(NULL, sock, 1);
if (arv != APR_SUCCESS) {
*len = 0;
return arv;
@@ -191,7 +158,7 @@
if (rv == -1 && (errno == EAGAIN || errno == EWOULDBLOCK)
&& sock->timeout != 0) {
- apr_status_t arv = apr_wait_for_io_or_timeout(sock, 0);
+ apr_status_t arv = apr_wait_for_io_or_timeout(NULL, sock, 0);
if (arv != APR_SUCCESS) {
*len = 0;
return arv;
@@ -224,7 +191,7 @@
if (rv == -1 && (errno == EAGAIN || errno == EWOULDBLOCK) &&
sock->timeout != 0) {
- apr_status_t arv = apr_wait_for_io_or_timeout(sock, 1);
+ apr_status_t arv = apr_wait_for_io_or_timeout(NULL, sock, 1);
if (arv != APR_SUCCESS) {
*len = 0;
return arv;
@@ -272,7 +239,7 @@
sock->timeout != 0) {
apr_status_t arv;
do_select:
- arv = apr_wait_for_io_or_timeout(sock, 0);
+ arv = apr_wait_for_io_or_timeout(NULL, sock, 0);
if (arv != APR_SUCCESS) {
*len = 0;
return arv;
@@ -370,7 +337,7 @@
(errno == EAGAIN || errno == EWOULDBLOCK) &&
sock->timeout > 0) {
do_select:
- arv = apr_wait_for_io_or_timeout(sock, 0);
+ arv = apr_wait_for_io_or_timeout(NULL, sock, 0);
if (arv != APR_SUCCESS) {
*len = 0;
return arv;
@@ -528,7 +495,7 @@
if (sock->netmask & APR_INCOMPLETE_WRITE) {
apr_status_t arv;
sock->netmask &= ~APR_INCOMPLETE_WRITE;
- arv = apr_wait_for_io_or_timeout(sock, 0);
+ arv = apr_wait_for_io_or_timeout(NULL, sock, 0);
if (arv != APR_SUCCESS) {
*len = 0;
return arv;
@@ -590,7 +557,7 @@
if (rv == -1 &&
errno == EAGAIN &&
sock->timeout > 0) {
- apr_status_t arv = apr_wait_for_io_or_timeout(sock, 0);
+ apr_status_t arv = apr_wait_for_io_or_timeout(NULL, sock, 0);
if (arv != APR_SUCCESS) {
*len = 0;
return arv;
@@ -708,7 +675,7 @@
if (rc == -1 &&
(errno == EAGAIN || errno == EWOULDBLOCK) &&
sock->timeout > 0) {
- apr_status_t arv = apr_wait_for_io_or_timeout(sock, 0);
+ apr_status_t arv = apr_wait_for_io_or_timeout(NULL, sock, 0);
if (arv != APR_SUCCESS) {
*len = 0;
@@ -847,7 +814,7 @@
(errno == EAGAIN || errno == EWOULDBLOCK) &&
sock->timeout > 0) {
do_select:
- arv = apr_wait_for_io_or_timeout(sock, 0);
+ arv = apr_wait_for_io_or_timeout(NULL, sock, 0);
if (arv != APR_SUCCESS) {
*len = 0;
return arv;
@@ -957,7 +924,7 @@
*/
if (sock->netmask & APR_INCOMPLETE_WRITE) {
sock->netmask &= ~APR_INCOMPLETE_WRITE;
- arv = apr_wait_for_io_or_timeout(sock, 0);
+ arv = apr_wait_for_io_or_timeout(NULL, sock, 0);
if (arv != APR_SUCCESS) {
*len = 0;
return arv;
@@ -989,7 +956,7 @@
else if (!arv &&
apr_is_option_set(sock->netmask, APR_SO_TIMEOUT) == 1)
{
- apr_status_t t = apr_wait_for_io_or_timeout(sock, 0);
+ apr_status_t t = apr_wait_for_io_or_timeout(NULL, sock, 0);
if (t != APR_SUCCESS)
{
Index: srclib/apr/test/sendfile.c
===================================================================
RCS file: /home/cvs/apr/test/sendfile.c,v
retrieving revision 1.20
diff -u -d -b -w -u -r1.20 sendfile.c
--- srclib/apr/test/sendfile.c 13 Mar 2002 20:39:27 -0000 1.20
+++ srclib/apr/test/sendfile.c 6 Jul 2002 00:39:38 -0000
@@ -60,6 +60,7 @@
#include "apr_network_io.h"
#include "apr_errno.h"
#include "apr_general.h"
+#include "apr_poll.h"
#if !APR_HAS_SENDFILE
int main(void)
@@ -373,7 +374,7 @@
if (APR_STATUS_IS_EAGAIN(rv)) {
assert(tmplen == 0);
nsocks = 1;
- tmprv = apr_poll(pfd, &nsocks, -1);
+ tmprv = apr_poll(pfd, 1, &nsocks, -1);
assert(!tmprv);
assert(nsocks == 1);
/* continue; */
Index: srclib/apr/test/server.c
===================================================================
RCS file: /home/cvs/apr/test/server.c,v
retrieving revision 1.34
diff -u -d -b -w -u -r1.34 server.c
--- srclib/apr/test/server.c 13 Mar 2002 20:39:27 -0000 1.34
+++ srclib/apr/test/server.c 6 Jul 2002 00:39:38 -0000
@@ -58,6 +58,7 @@
#include <stdlib.h>
#include "apr_network_io.h"
#include "apr_getopt.h"
+#include "apr_poll.h"
#define STRLEN 15
@@ -138,7 +139,7 @@
pollres = 1;
APR_TEST_BEGIN(rv, "Polling for socket",
- apr_poll(sdset, &pollres, -1))
+ apr_poll(sdset, 1, &pollres, -1))
if (pollres == 0) {
fprintf(stdout, "Failed\n");
Index: srclib/apr/test/testfile.c
===================================================================
RCS file: /home/cvs/apr/test/testfile.c,v
retrieving revision 1.49
diff -u -d -b -w -u -r1.49 testfile.c
--- srclib/apr/test/testfile.c 5 Jul 2002 00:26:36 -0000 1.49
+++ srclib/apr/test/testfile.c 6 Jul 2002 00:39:44 -0000
@@ -61,6 +61,7 @@
#include "apr_network_io.h"
#include "apr_errno.h"
#include "apr_general.h"
+#include "apr_poll.h"
#include "apr_lib.h"
#include "test_apr.h"
@@ -157,7 +158,7 @@
apr_poll_socket_add(sdset, testsock, APR_POLLIN);
num = 1;
STD_TEST_NEQ(" Checking for incoming data",
- apr_poll(sdset, &num, apr_time_from_sec(1)));
+ apr_poll(sdset, 1, &num, apr_time_from_sec(1)));
if (num == 0) {
printf("** This platform doesn't return readability on a regular file.**\n");
}
Index: srclib/apr/test/testpoll.c
===================================================================
RCS file: /home/cvs/apr/test/testpoll.c,v
retrieving revision 1.8
diff -u -d -b -w -u -r1.8 testpoll.c
--- srclib/apr/test/testpoll.c 13 Mar 2002 20:39:27 -0000 1.8
+++ srclib/apr/test/testpoll.c 6 Jul 2002 00:39:44 -0000
@@ -57,6 +57,7 @@
#include "apr_general.h"
#include "apr_lib.h"
#include "apr_network_io.h"
+#include "apr_poll.h"
#if APR_HAVE_UNISTD_H
#include <unistd.h>
#endif
@@ -145,7 +146,7 @@
apr_socket_t *s[3];
apr_sockaddr_t *sa[3];
apr_pollfd_t *pollset;
- int i = 0, srv = 0;
+ int i = 0, srv = 3;
fprintf (stdout,"APR Poll Test\n*************\n\n");
@@ -186,29 +187,29 @@
printf("OK\n");
printf("Starting Tests\n");
- apr_poll(pollset, &srv, 10);
+ apr_poll(pollset, 3, &srv, 10);
check_sockets(pollset, s);
send_msg(s, sa, 2);
- apr_poll(pollset, &srv, 10);
+ apr_poll(pollset, 3, &srv, 10);
check_sockets(pollset, s);
recv_msg(s, 2, context);
send_msg(s, sa, 1);
- apr_poll(pollset, &srv, 10);
+ apr_poll(pollset, 3, &srv, 10);
check_sockets(pollset, s);
send_msg(s, sa, 2);
- apr_poll(pollset, &srv, 10);
+ apr_poll(pollset, 3, &srv, 10);
check_sockets(pollset, s);
recv_msg(s, 1, context);
send_msg(s, sa, 0);
- apr_poll(pollset, &srv, 10);
+ apr_poll(pollset, 3, &srv, 10);
check_sockets(pollset, s);
printf("Tests completed.\n");
Index: support/ab.c
===================================================================
RCS file: /home/cvs/httpd-2.0/support/ab.c,v
retrieving revision 1.106
diff -u -d -b -w -u -r1.106 ab.c
--- support/ab.c 4 Jul 2002 22:56:51 -0000 1.106
+++ support/ab.c 6 Jul 2002 00:39:45 -0000
@@ -166,6 +166,7 @@
#include "apr_lib.h"
#include "apr_portable.h"
#include "ap_release.h"
+#include "apr_poll.h"
#define APR_WANT_STRFUNC
#include "apr_want.h"
@@ -1648,7 +1649,7 @@
status = APR_SUCCESS;
else
#endif
- status = apr_poll(readbits, &n, aprtimeout);
+ status = apr_poll(readbits, concurrency, &n, aprtimeout);
if (status != APR_SUCCESS)
apr_err("apr_poll", status);
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
550 Jean St
Oakland CA 94610
-------------------------------------------------------------------------------
Re: [PATCH] speed up network timeout processing
Posted by rb...@apache.org.
On Fri, 5 Jul 2002, Roy T. Fielding wrote:
> > If that was all we were doing, I would agree with you. But `Jeff's patch
> > implements BOTH select and poll with an #ifdef, because not every platform
> > has poll(). This is exactly the reason for having apr_poll(), and not
> > using it is stupid. If the argument is performance, then back it up with
> > numbers. If the performance is as bad as everybody seems to think it is,
> > then fix the functions. If the problem is the API itself, then fix the
> > API.
>
> But the only way to fix the API is to supply it with a block of memory
> rather than a pool pointer.
Which is exactly what my current implementation does. I only use a pool
once throughout the new poll code. I think we can optimize that pool out
for the most common case, but that is for after my current set of testing.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
550 Jean St
Oakland CA 94610
-------------------------------------------------------------------------------
Re: [PATCH] speed up network timeout processing
Posted by "Roy T. Fielding" <fi...@apache.org>.
> If that was all we were doing, I would agree with you. But `Jeff's patch
> implements BOTH select and poll with an #ifdef, because not every platform
> has poll(). This is exactly the reason for having apr_poll(), and not
> using it is stupid. If the argument is performance, then back it up with
> numbers. If the performance is as bad as everybody seems to think it is,
> then fix the functions. If the problem is the API itself, then fix the
> API.
But the only way to fix the API is to supply it with a block of memory
rather than a pool pointer.
....Roy
RE: [PATCH] speed up network timeout processing
Posted by Bill Stoddard <bi...@wstoddard.com>.
>
> On Wed, 3 Jul 2002, Ian Holsman wrote:
>
> > Bill Stoddard wrote:
> > >>>From: trawick@rdu88-250-182.nc.rr.com [mailto:trawick@rdu88-250-
> >
> > <snip>
> >
> > > I'd rather pay the maintenance overhead on this code. BTW,
> testing on AIX
> > > yields up to a 9% increase in throughput with this patch
> (serving 500 byte
> > > file out of mod_mem_cache). Huge win for a small change!
> > >
> > > Bill
> > >
> >
> >
> > fantastic speed improvement that rocks.
> >
> > like Brian said the APR is to provide a consistent usable interface to
> > the outside world. if we choose to implement the timeout using the
> > poll() function instead of a select() function than that is perfectly
> > valid.
>
> If that was all we were doing, I would agree with you. But `Jeff's patch
> implements BOTH select and poll with an #ifdef, because not every platform
> has poll(). This is exactly the reason for having apr_poll(), and not
> using it is stupid. If the argument is performance, then back it up with
> numbers. If the performance is as bad as everybody seems to think it is,
> then fix the functions. If the problem is the API itself, then fix the
> API.
Ryan,
APR does have an impact on performance. Every function call impacts
performance and APR adds several function calls as compared to a native
system call. We live with this because we need Apache 2 to run on multiple
platforms. There is no need to be pedantic about using the same API within
APR. Using apr_poll WILL add instructions. Give me an implementation and
I'll compare the two. And as Jeff has already pointed out, using apr_poll
in this case will increase complexity of the code.
Bill
Re: [PATCH] speed up network timeout processing
Posted by rb...@apache.org.
On Wed, 3 Jul 2002, Ian Holsman wrote:
> Bill Stoddard wrote:
> >>>From: trawick@rdu88-250-182.nc.rr.com [mailto:trawick@rdu88-250-
>
> <snip>
>
> > I'd rather pay the maintenance overhead on this code. BTW, testing on AIX
> > yields up to a 9% increase in throughput with this patch (serving 500 byte
> > file out of mod_mem_cache). Huge win for a small change!
> >
> > Bill
> >
>
>
> fantastic speed improvement that rocks.
>
> like Brian said the APR is to provide a consistent usable interface to
> the outside world. if we choose to implement the timeout using the
> poll() function instead of a select() function than that is perfectly
> valid.
If that was all we were doing, I would agree with you. But `Jeff's patch
implements BOTH select and poll with an #ifdef, because not every platform
has poll(). This is exactly the reason for having apr_poll(), and not
using it is stupid. If the argument is performance, then back it up with
numbers. If the performance is as bad as everybody seems to think it is,
then fix the functions. If the problem is the API itself, then fix the
API.
> another case in which we do the SAME THING is how we implement
> sendfile() on machines which don't have it. we don't use apr_file_writev
> we use another standard system call writev
It isn't the same thing. First of all, according to my reading of the
code, we don't implement sendfile at all if the platform doesn't suppoort
it. Apache itself does, but it uses sendv. Second of all, as I said
above I wouldn't have a problem with the approach if it didn't require
#ifdef's to implement two options to do the same code, which is the exact
problem APR is supposed to solve.
Even worse, is that we are making decisions without ANY numbers to back up
what people aare saying. I am planning to do some measurements on Friday
to look at the REAL numbers before we can make a decision. If the numbers
are much worse for apr_poll, then I have some thoughts on how to improve
that API to fix the performance problems.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
550 Jean St
Oakland CA 94610
-------------------------------------------------------------------------------
Re: [PATCH] speed up network timeout processing
Posted by Ian Holsman <ia...@apache.org>.
Bill Stoddard wrote:
>>>From: trawick@rdu88-250-182.nc.rr.com [mailto:trawick@rdu88-250-
<snip>
> I'd rather pay the maintenance overhead on this code. BTW, testing on AIX
> yields up to a 9% increase in throughput with this patch (serving 500 byte
> file out of mod_mem_cache). Huge win for a small change!
>
> Bill
>
fantastic speed improvement that rocks.
like Brian said the APR is to provide a consistent usable interface to
the outside world. if we choose to implement the timeout using the
poll() function instead of a select() function than that is perfectly
valid.
another case in which we do the SAME THING is how we implement
sendfile() on machines which don't have it. we don't use apr_file_writev
we use another standard system call writev
Ian
RE: [PATCH] speed up network timeout processing
Posted by Bill Stoddard <bi...@wstoddard.com>.
> > From: trawick@rdu88-250-182.nc.rr.com [mailto:trawick@rdu88-250-
> >
> > "Ryan Bloom" <rb...@covalent.net> writes:
> >
> > > > From: trawick@rdu88-250-182.nc.rr.com [mailto:trawick@rdu88-250-
> > > >
> > > > A little bird told me that FD_ZERO() burns lots of cycles in
> > > > apr_wait_for_io_or_timeout(). It turns out that this is an easy
> > > > conversion to poll(), which doesn't have such overhead in the
> > > > interface.
> > > >
> > > > This works for me with some testing (timeouts on read and write
> work
> > > > for me).
> > >
> > > Can we remove the #ifdef's by just using apr_poll here?
> >
> > I'd rather we not, since that introduces a fair amount of extra
> > overhead.
>
> Then let's get rid of the overhead. If we don't use apr_poll, then the
> overhead is maintenance, because we have abstracted out the poll/select
> difference twice in the code.
I'd rather pay the maintenance overhead on this code. BTW, testing on AIX
yields up to a 9% increase in throughput with this patch (serving 500 byte
file out of mod_mem_cache). Huge win for a small change!
Bill
RE: [PATCH] speed up network timeout processing
Posted by Ryan Bloom <rb...@covalent.net>.
> From: trawick@rdu88-250-182.nc.rr.com [mailto:trawick@rdu88-250-
>
> "Ryan Bloom" <rb...@covalent.net> writes:
>
> > > From: trawick@rdu88-250-182.nc.rr.com [mailto:trawick@rdu88-250-
> > >
> > > A little bird told me that FD_ZERO() burns lots of cycles in
> > > apr_wait_for_io_or_timeout(). It turns out that this is an easy
> > > conversion to poll(), which doesn't have such overhead in the
> > > interface.
> > >
> > > This works for me with some testing (timeouts on read and write
work
> > > for me).
> >
> > Can we remove the #ifdef's by just using apr_poll here?
>
> I'd rather we not, since that introduces a fair amount of extra
> overhead.
Then let's get rid of the overhead. If we don't use apr_poll, then the
overhead is maintenance, because we have abstracted out the poll/select
difference twice in the code.
Ryan
Re: [PATCH] speed up network timeout processing
Posted by Jeff Trawick <tr...@attglobal.net>.
"Ryan Bloom" <rb...@covalent.net> writes:
> > From: trawick@rdu88-250-182.nc.rr.com [mailto:trawick@rdu88-250-
> >
> > A little bird told me that FD_ZERO() burns lots of cycles in
> > apr_wait_for_io_or_timeout(). It turns out that this is an easy
> > conversion to poll(), which doesn't have such overhead in the
> > interface.
> >
> > This works for me with some testing (timeouts on read and write work
> > for me).
>
> Can we remove the #ifdef's by just using apr_poll here?
I'd rather we not, since that introduces a fair amount of extra
overhead.
--
Jeff Trawick | trawick@attglobal.net
Born in Roswell... married an alien...
RE: [PATCH] speed up network timeout processing
Posted by Sander Striker <st...@apache.org>.
> From: trawick@rdu88-250-182.nc.rr.com
> [mailto:trawick@rdu88-250-182.nc.rr.com]On Behalf Of Jeff Trawick
> Sent: 03 July 2002 21:16
> "Bill Stoddard" <bi...@wstoddard.com> writes:
>
> > > Have either of you benchmarked with apr_poll() or are you assuming that
> > > the problem exists?
> > >
> > > Ryan
> > >
> > >
> >
> > Sorry didn't answer you here... There definitely are extra instructions and
> > function calls involved with using apr_poll() in this case. I don't know the
> > exact number but I could find out. The affects of a few dozen extra
> > instructions would be insignificant in overall server performance, but you
> > can say that about any place in the server. Adding function call overhead
> > adds up. In this case, is easy to just avoid the extra calls altogether with
> > no significant loss in code readability.
>
> Using apr_poll() is more complex than using poll() in this situation.
> We'll have to be careful not to leak storage (i.e., create/destroy a
> subpool or attach the apr_pollfd_t to the apr_socket_t the first time
> we need to wait_for_io_or_timeout. And since this deals with more
> than just sockets we'll have to play with the interface to
> wait_for_io_or_timeout. So we munge a lot of code so that we only
> call poll() from one place and we're left with extra complexity and
> extra pathlength.
>
> An implementation using apr_poll() which is more complex and less
> speedy by inspection than using poll() directly is going to be
> vetoed.
Oh, come on guys, let's not play the veto card so early.
By inspection != real performance. Numbers first.
Sander 'sounding a bit more harsh than he means to'
Re: [PATCH] speed up network timeout processing
Posted by Jeff Trawick <tr...@attglobal.net>.
"Bill Stoddard" <bi...@wstoddard.com> writes:
> > Have either of you benchmarked with apr_poll() or are you assuming that
> > the problem exists?
> >
> > Ryan
> >
> >
>
> Sorry didn't answer you here... There definitely are extra instructions and
> function calls involved with using apr_poll() in this case. I don't know the
> exact number but I could find out. The affects of a few dozen extra
> instructions would be insignificant in overall server performance, but you
> can say that about any place in the server. Adding function call overhead
> adds up. In this case, is easy to just avoid the extra calls altogether with
> no significant loss in code readability.
Using apr_poll() is more complex than using poll() in this situation.
We'll have to be careful not to leak storage (i.e., create/destroy a
subpool or attach the apr_pollfd_t to the apr_socket_t the first time
we need to wait_for_io_or_timeout. And since this deals with more
than just sockets we'll have to play with the interface to
wait_for_io_or_timeout. So we munge a lot of code so that we only
call poll() from one place and we're left with extra complexity and
extra pathlength.
An implementation using apr_poll() which is more complex and less
speedy by inspection than using poll() directly is going to be
vetoed.
--
Jeff Trawick | trawick@attglobal.net
Born in Roswell... married an alien...
RE: [PATCH] speed up network timeout processing
Posted by Bill Stoddard <bi...@wstoddard.com>.
> Have either of you benchmarked with apr_poll() or are you assuming that
> the problem exists?
>
> Ryan
>
>
Sorry didn't answer you here... There definitely are extra instructions and
function calls involved with using apr_poll() in this case. I don't know the
exact number but I could find out. The affects of a few dozen extra
instructions would be insignificant in overall server performance, but you
can say that about any place in the server. Adding function call overhead
adds up. In this case, is easy to just avoid the extra calls altogether with
no significant loss in code readability.
Bill
RE: [PATCH] speed up network timeout processing
Posted by Ryan Bloom <rb...@covalent.net>.
> From: Bill Stoddard [mailto:bill@wstoddard.com]
>
>
> > > From: Bill Stoddard [mailto:bill@wstoddard.com]
> > >
> > >
> > > > > From: trawick@rdu88-250-182.nc.rr.com
[mailto:trawick@rdu88-250-
> > > > >
> > > > > A little bird told me that FD_ZERO() burns lots of cycles in
> > > > > apr_wait_for_io_or_timeout(). It turns out that this is an
easy
> > > > > conversion to poll(), which doesn't have such overhead in the
> > > > > interface.
> > > > >
> > > > > This works for me with some testing (timeouts on read and
write
> > work
> > > > > for me).
> > > >
> > > > Can we remove the #ifdef's by just using apr_poll here?
> > > >
> > > > Ryan
> > >
> > > I'd prefer not to do that. calling apr_poll will just add extra
> > function
> > > call overhead. This code is pretty simple (ie, the #ifdef does not
> > > signicantly impact code readability).
> >
> > This is bogus. If apr_poll is so heavy-weight that APR can't use
it,
> > then it is just too heavy-weight. Avoiding the function doesn't do
> > anything other than prove that apr_poll is not useful in its current
> > form. FIX THE PROBLEM!
>
> What problem? I have NO problem with the code as it is. I see NO
> advantage
> to using apr_poll() in this particular application.
apr_wait_for_timeout()
> takes two arguments, an apr_socket and a flag indicating whether the
poll
> is
> for a read or a write. There are no opportunities to set the pollset
in
> this
> function so why in the world do you want to add all the extra function
> calls
> to use apr functions to set the pollset? That's just goofy Ryan.
It's not goofy. The problem that I am referring to is that you and Jeff
are saying that apr_poll is too heavy-weight to use inside of APR. If
that is true, then how can you expect ANYBODY outside of APR to find it
useful??????? The whole point of apr_poll is that it abstracts out the
difference between poll() and select(). What Jeff's patch does is to
abstract out the difference between poll() and select(). In other
words, two paths that do the same thing. That is goofy.
Have either of you benchmarked with apr_poll() or are you assuming that
the problem exists?
Ryan
RE: [PATCH] speed up network timeout processing
Posted by Bill Stoddard <bi...@wstoddard.com>.
> > From: Bill Stoddard [mailto:bill@wstoddard.com]
> >
> >
> > > > From: trawick@rdu88-250-182.nc.rr.com [mailto:trawick@rdu88-250-
> > > >
> > > > A little bird told me that FD_ZERO() burns lots of cycles in
> > > > apr_wait_for_io_or_timeout(). It turns out that this is an easy
> > > > conversion to poll(), which doesn't have such overhead in the
> > > > interface.
> > > >
> > > > This works for me with some testing (timeouts on read and write
> work
> > > > for me).
> > >
> > > Can we remove the #ifdef's by just using apr_poll here?
> > >
> > > Ryan
> >
> > I'd prefer not to do that. calling apr_poll will just add extra
> function
> > call overhead. This code is pretty simple (ie, the #ifdef does not
> > signicantly impact code readability).
>
> This is bogus. If apr_poll is so heavy-weight that APR can't use it,
> then it is just too heavy-weight. Avoiding the function doesn't do
> anything other than prove that apr_poll is not useful in its current
> form. FIX THE PROBLEM!
What problem? I have NO problem with the code as it is. I see NO advantage
to using apr_poll() in this particular application. apr_wait_for_timeout()
takes two arguments, an apr_socket and a flag indicating whether the poll is
for a read or a write. There are no opportunities to set the pollset in this
function so why in the world do you want to add all the extra function calls
to use apr functions to set the pollset? That's just goofy Ryan.
Bill
RE: [PATCH] speed up network timeout processing
Posted by Ryan Bloom <rb...@covalent.net>.
> From: Bill Stoddard [mailto:bill@wstoddard.com]
>
>
> > > From: trawick@rdu88-250-182.nc.rr.com [mailto:trawick@rdu88-250-
> > >
> > > A little bird told me that FD_ZERO() burns lots of cycles in
> > > apr_wait_for_io_or_timeout(). It turns out that this is an easy
> > > conversion to poll(), which doesn't have such overhead in the
> > > interface.
> > >
> > > This works for me with some testing (timeouts on read and write
work
> > > for me).
> >
> > Can we remove the #ifdef's by just using apr_poll here?
> >
> > Ryan
>
> I'd prefer not to do that. calling apr_poll will just add extra
function
> call overhead. This code is pretty simple (ie, the #ifdef does not
> signicantly impact code readability).
This is bogus. If apr_poll is so heavy-weight that APR can't use it,
then it is just too heavy-weight. Avoiding the function doesn't do
anything other than prove that apr_poll is not useful in its current
form. FIX THE PROBLEM! It is completely insane to re-write the
select/poll abstraction in two different parts of APR. If APR can't use
apr_poll, then the API should be removed, because it doesn't perform
well enough for applications to use.
Ryan
RE: [PATCH] speed up network timeout processing
Posted by Bill Stoddard <bi...@wstoddard.com>.
> > From: trawick@rdu88-250-182.nc.rr.com [mailto:trawick@rdu88-250-
> >
> > A little bird told me that FD_ZERO() burns lots of cycles in
> > apr_wait_for_io_or_timeout(). It turns out that this is an easy
> > conversion to poll(), which doesn't have such overhead in the
> > interface.
> >
> > This works for me with some testing (timeouts on read and write work
> > for me).
>
> Can we remove the #ifdef's by just using apr_poll here?
>
> Ryan
I'd prefer not to do that. calling apr_poll will just add extra function
call overhead. This code is pretty simple (ie, the #ifdef does not
signicantly impact code readability).
Bill
RE: [PATCH] speed up network timeout processing
Posted by Ryan Bloom <rb...@covalent.net>.
> From: trawick@rdu88-250-182.nc.rr.com [mailto:trawick@rdu88-250-
>
> A little bird told me that FD_ZERO() burns lots of cycles in
> apr_wait_for_io_or_timeout(). It turns out that this is an easy
> conversion to poll(), which doesn't have such overhead in the
> interface.
>
> This works for me with some testing (timeouts on read and write work
> for me).
Can we remove the #ifdef's by just using apr_poll here?
Ryan