You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Justin Erenkrantz <je...@apache.org> on 2002/07/09 09:43:32 UTC
Re: New apr_poll() implementation
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
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