You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by "William A. Rowe, Jr." <wr...@rowe-clan.net> on 2002/07/18 08:09:25 UTC
[design] work around new apr_poll leakage?
Ok, we have real problems with the palloc in the new design of apr_poll().
But it's fundamentally better than falling back on poll() because our design
just sucks, performance-wise. Ryan speaks of alloca, but that won't
work on all platforms, most especially stack-impaired platforms such as
Netware without the ability to grow the stack.
So here's a design suggestion. Take the apr_pollfd_t element;
struct apr_pollfd_t {
apr_pool_t *p;
apr_datatype_e desc_type;
apr_int16_t reqevents;
apr_int16_t rtnevents;
apr_descriptor desc;
};
If we pull the apr_pool_t *p [which is WAY overkill if you have several
dozen to hundreds of descriptors you might be polling against] and
create a brand new apr_pollfd_set_t [also transparent!]...
typedef struct {
apr_pool_t *p;
apr_pollfd_internal_t *int;
apr_pollfd_t *first;
int count;
} apr_pollfd_set_t;
Modify apr_poll_setup to return a new apr_pollfd_set_t, with *first
already pointing at a newly allocated apr_pollfd_t array, and set up
apr_pollfd_internal_t to NULL.
The *int pointer tracks APR's internal count and arrays that it needs.
If the count to an invocation of apr_poll() exceeds our internally allocated
count, THEN we go and allocate a larger internal structure. But not for
every pass into apr_poll().
ADVANTAGES
. we stop the leak. Once we grow to some given *int pollset size,
we won't be growing anymore.
. we lighten the apr_pollfd_t elements by one pool pointer.
. using *first, we can change our offset into a huge array of elements.
. using count, we can resize the array we are interested in.
. neat feature, we actually can have several apr_pollfd_set_t
indexes floating around, neatly pointing into different parts
of a huge apr_pollfd_t array.
DISADVANTAGES
. if not using apr_poll_setup(), the user is absolutely responsible
for initializing *v to NULL.
. two structures to think about. Not complex structures, but two,
none the less.
Comments or ideas? This was idle brainstorming.
Bill
.
Re: [design] work around new apr_poll leakage?
Posted by Aaron Bannert <aa...@clove.org>.
On Sun, Jul 21, 2002 at 09:45:42PM -0700, Ryan Bloom wrote:
> You are optimizing for apps with large pollsets. Fine, go ahead.
> Create a wrapper structure with an opaque structure as Will suggested.
> You want to re-use your pollset, figure out a solution. But for apps
> (like Apache, and most other non-async apps) with small pollsets, the
> current implemented with a few tweaks is the correct implementation.
I haven't really followed the details of either design, but I do have
this to say: We must take in to consideration the fact that one day we
are going to use this same interface for polling on huge sets of file
descriptors. If we want to move closer to async, let's make async-friendly
apps. An O(n) operation that must be performed once per call to apr_poll()
is an O(m*n) operation, which is terrible. (I don't know if that's what
is really going on, but you get my point.)
-aaron
Re: [design] work around new apr_poll leakage?
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:
> >
> > > No, the old design was completely bogus. As proof, Trawick vetoed
> even
> > > using the damned thing inside of APR.
> >
> > I listed two conditions for the veto:
> >
> > 1) something more complex than calling poll() in that situation
> > 2) something slower than calling poll() in that situation
> >
> > It seems to me that the current code meets both conditions, though
> > not as obnoxiously as code using the older apr_poll() interface would
> > have.
>
> Can you clarify this please? Does that mean that you are vetoing the
> current code too? BTW, I don't understand how you can say that calling
> apr_poll() is more complex than calling poll(), since they have the same
> API. If your saying that they _only_ solution you will accept is
> calling poll() itself, then that is completely bogus.
You left out the pathlength issue... Calling anything which is a
wrapper around poll() is going to be deficient to calling poll() as
far as performance goes.
I'll backpedal on complexity since I forgot the need for
#ifdef HAVE_POLL
poll()
#else
select()
#endif
The current apr_poll() implementation seems to be optimize for the
very simple use of poll() as in apr_wait_for_io_or_timeout(), so I
will mostly agree with you and say that it is only a little more
complex to use apr_poll() when compared with calling poll() directly,
and the need for a select() flavor tips the balance in favor of using
the current apr_poll().
Am I vetoing? At the moment I'm willing to wait until we have a
usable apr_poll() implementation, at which point it will hopefully be
clear how apr_wait_for_io_or_timeout() should work.
--
Jeff Trawick | trawick@attglobal.net
Born in Roswell... married an alien...
RE: [design] work around new apr_poll leakage?
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:
>
> > No, the old design was completely bogus. As proof, Trawick vetoed
even
> > using the damned thing inside of APR.
>
> I listed two conditions for the veto:
>
> 1) something more complex than calling poll() in that situation
> 2) something slower than calling poll() in that situation
>
> It seems to me that the current code meets both conditions, though
> not as obnoxiously as code using the older apr_poll() interface would
> have.
Can you clarify this please? Does that mean that you are vetoing the
current code too? BTW, I don't understand how you can say that calling
apr_poll() is more complex than calling poll(), since they have the same
API. If your saying that they _only_ solution you will accept is
calling poll() itself, then that is completely bogus.
Ryan
Re: [design] work around new apr_poll leakage?
Posted by Jeff Trawick <tr...@attglobal.net>.
"Ryan Bloom" <rb...@covalent.net> writes:
> No, the old design was completely bogus. As proof, Trawick vetoed even
> using the damned thing inside of APR.
I listed two conditions for the veto:
1) something more complex than calling poll() in that situation
2) something slower than calling poll() in that situation
It seems to me that the current code meets both conditions, though
not as obnoxiously as code using the older apr_poll() interface would
have.
--
Jeff Trawick | trawick@attglobal.net
Born in Roswell... married an alien...
Re: [design] work around new apr_poll leakage?
Posted by Brian Pane <br...@apache.org>.
Sander Striker wrote:
>I'd like to keep the current code and see the mem leak fixed. That should
>for the moment suffice. For large pollsets a workable solution was provided,
>making the veto against the current code even harder to grok.
>
Which solution was the workable one for large pollsets? If you mean
the malloc solution, I beg to differ. :-)
What do you think of splitting it into two APIs? I agree that the
current API is okay for small descriptor sets--if only we could
limit its use to cases where the number of descriptors really is
small. One of my messages from yesterday has more details on this
idea, http://marc.theaimsgroup.com/?l=apr-dev&m=102731606500897
--Brian
RE: [design] work around new apr_poll leakage?
Posted by Sander Striker <st...@apache.org>.
> From: Ryan Bloom [mailto:rbb@covalent.net]
> Sent: 22 July 2002 17:10
> Fine. Congratulations. Yet again we are stopped from making any
> progress because people refuse to work together and instead we have to
> use fscking vetos to play together.
>
> The old API sucks. It is unusable, causes mistakes, and doesn't allow
> for a fully featured API. But let's put it back.
Nope. A veto should be acompagnied by an alternative implementation if
it results in a situation where there is no alternative due to vetos anymore.
I'd like to keep the current code and see the mem leak fixed. That should
for the moment suffice. For large pollsets a workable solution was provided,
making the veto against the current code even harder to grok.
> I'm going away for a few more days. I was a lot happier when I wasn't
> reading this e-mail list.
:(
> Ryan
Sander
RE: [design] work around new apr_poll leakage?
Posted by Ryan Bloom <rb...@covalent.net>.
Fine. Congratulations. Yet again we are stopped from making any
progress because people refuse to work together and instead we have to
use fscking vetos to play together.
The old API sucks. It is unusable, causes mistakes, and doesn't allow
for a fully featured API. But let's put it back.
I'm going away for a few more days. I was a lot happier when I wasn't
reading this e-mail list.
Ryan
----------------------------------------------
Ryan Bloom
rbb@covalent.net rbb@apache.org
> -----Original Message-----
> From: Brian Pane [mailto:brianp@apache.org]
> Sent: Monday, July 22, 2002 8:07 AM
> To: rbb@covalent.net
> Cc: dev@apr.apache.org
> Subject: Re: [design] work around new apr_poll leakage?
>
> Ryan Bloom wrote:
>
> >>The current API has a memory leak that makes it unsuitable for
> >>general-purpose use (including the httpd). That's why I vetoed it.
> >>
> >>
> >
> >Thatis just BS. This conversation started today with a solution to
the
> >memory leak.
> >
>
> The conversation started with the showstopper in the httpd STATUS.
> My veto from three days ago still stands:
> http://marc.theaimsgroup.com/?l=apr-dev&m=102705248212752
> I'm happy to see the increased discussion of potential fixes, but that
> discussion alone doesn't resolve the veto. The only two ways to
resolve
> the veto are to: remove the new poll code, or replace it with a
variant
> that doesn't have the memory leak and O(n) scalability problem.
>
> --Brian
>
Re: [design] work around new apr_poll leakage?
Posted by Brian Pane <br...@apache.org>.
Ryan Bloom wrote:
>>The current API has a memory leak that makes it unsuitable for
>>general-purpose use (including the httpd). That's why I vetoed it.
>>
>>
>
>Thatis just BS. This conversation started today with a solution to the
>memory leak.
>
The conversation started with the showstopper in the httpd STATUS.
My veto from three days ago still stands:
http://marc.theaimsgroup.com/?l=apr-dev&m=102705248212752
I'm happy to see the increased discussion of potential fixes, but that
discussion alone doesn't resolve the veto. The only two ways to resolve
the veto are to: remove the new poll code, or replace it with a variant
that doesn't have the memory leak and O(n) scalability problem.
--Brian
RE: [design] work around new apr_poll leakage?
Posted by Ryan Bloom <rb...@covalent.net>.
> >>new implementation results in an O(n) time wrapper (due to the need
to
> >>copy n pollfd objects on every poll call), not to mention the memory
> >>
> >>
> >leak.
> >
> >
> >>>BTW, that interface was removed because it was too bulky and
complex
> >>>
> >>>
> >and
> >
> >
> >>>too slow.
> >>>
> >>>
> >>>
> >>Please go ahead and revert the new implementation. If we need to
tune
> >>
> >>
> >the
> >
> >
> >>old implementation, let's do that.
> >>
> >>
> >
> >No, the old design was completely bogus. As proof, Trawick vetoed
even
> >using the damned thing inside of APR. I don't care if you think that
we
> >don't have to use it, if a developer of APR believes that the API is
too
> >ugly to use in their code, then the API is borked. Damn, I hated the
> >API and I designed it.
> >
> >The current API is correct, because it gives the user back control of
> >their memory.
> >
>
> The current API has a memory leak that makes it unsuitable for
> general-purpose use (including the httpd). That's why I vetoed it.
Thatis just BS. This conversation started today with a solution to the
memory leak.
Ryan
Re: [design] work around new apr_poll leakage?
Posted by Brian Pane <br...@apache.org>.
Ryan Bloom wrote:
>>Ryan Bloom wrote:
>>
>>
>>
>>>Congratulations. You have just designed the interface that was
>>>
>>>
>removed
>
>
>>>two weeks ago. :-(
>>>
>>>
>>>
>>Exactly. The old API had the right design: an abstract poll object
>>
>>
>with
>
>
>>accessor functions, so that the poll wrapper can run in O(1) time.
>>
>>
>The
>
>
>>new implementation results in an O(n) time wrapper (due to the need to
>>copy n pollfd objects on every poll call), not to mention the memory
>>
>>
>leak.
>
>
>>>BTW, that interface was removed because it was too bulky and complex
>>>
>>>
>and
>
>
>>>too slow.
>>>
>>>
>>>
>>Please go ahead and revert the new implementation. If we need to tune
>>
>>
>the
>
>
>>old implementation, let's do that.
>>
>>
>
>No, the old design was completely bogus. As proof, Trawick vetoed even
>using the damned thing inside of APR. I don't care if you think that we
>don't have to use it, if a developer of APR believes that the API is too
>ugly to use in their code, then the API is borked. Damn, I hated the
>API and I designed it.
>
>The current API is correct, because it gives the user back control of
>their memory.
>
The current API has a memory leak that makes it unsuitable for
general-purpose use (including the httpd). That's why I vetoed it.
--Brian
RE: [design] work around new apr_poll leakage?
Posted by Ryan Bloom <rb...@covalent.net>.
> Ryan Bloom wrote:
>
> >Congratulations. You have just designed the interface that was
removed
> >two weeks ago. :-(
> >
>
> Exactly. The old API had the right design: an abstract poll object
with
> accessor functions, so that the poll wrapper can run in O(1) time.
The
> new implementation results in an O(n) time wrapper (due to the need to
> copy n pollfd objects on every poll call), not to mention the memory
leak.
>
> >BTW, that interface was removed because it was too bulky and complex
and
> >too slow.
> >
>
> Please go ahead and revert the new implementation. If we need to tune
the
> old implementation, let's do that.
No, the old design was completely bogus. As proof, Trawick vetoed even
using the damned thing inside of APR. I don't care if you think that we
don't have to use it, if a developer of APR believes that the API is too
ugly to use in their code, then the API is borked. Damn, I hated the
API and I designed it.
The current API is correct, because it gives the user back control of
their memory.
You have a problem with looping through the pollset each time you call
apr_poll? I have a problem with requiring 6 functions to manipulate the
pollset. You can't do it with less than 6, we tried, and we always
ended up with 6. Oh wait, 7 if you want both sockets and files.
You are optimizing for apps with large pollsets. Fine, go ahead.
Create a wrapper structure with an opaque structure as Will suggested.
You want to re-use your pollset, figure out a solution. But for apps
(like Apache, and most other non-async apps) with small pollsets, the
current implemented with a few tweaks is the correct implementation.
Ryan
Re: [design] work around new apr_poll leakage?
Posted by Brian Pane <br...@apache.org>.
Ryan Bloom wrote:
>Congratulations. You have just designed the interface that was removed
>two weeks ago. :-(
>
Exactly. The old API had the right design: an abstract poll object with
accessor functions, so that the poll wrapper can run in O(1) time. The
new implementation results in an O(n) time wrapper (due to the need to
copy n pollfd objects on every poll call), not to mention the memory leak.
>BTW, that interface was removed because it was too bulky and complex and
>too slow.
>
Please go ahead and revert the new implementation. If we need to tune the
old implementation, let's do that.
Thanks,
--Brian
RE: [design] work around new apr_poll leakage?
Posted by Ryan Bloom <rb...@covalent.net>.
Congratulations. You have just designed the interface that was removed
two weeks ago. :-(
BTW, that interface was removed because it was too bulky and complex and
too slow.
Ryan
----------------------------------------------
Ryan Bloom rbb@covalent.net
645 Howard St. rbb@apache.org
San Francisco, CA
> -----Original Message-----
> From: Brian Pane [mailto:brianp@apache.org]
> Sent: Thursday, July 18, 2002 11:28 PM
> To: dev@apr.apache.org
> Subject: Re: [design] work around new apr_poll leakage?
>
> William A. Rowe, Jr. wrote:
>
> > At 11:21 PM 7/18/2002, Brian Pane wrote:
> >
> >> I'm almost in complete agreement with this proposal. We
> >> need a wrapper object to hold the temp pollset so that we
> >> can re-use it for all subsequent poll calls on the same
> >> fd set.
> >
> >
> > Notice that apr_pollfd_internal_t *int; below _IS_ entirely
> > opaque... [and using a reserved keyword, the member name
> > REALLY sucked... how about simply *private.]
>
>
> Right, but the pointer to the vector of apr_pollfd_t
> objects is transparent. And that's the one that will
> cause problems if we try to use something like /dev/poll,
> because the set of descriptors in that vector can get
> out of synch with the descriptors that have been registered
> with /dev/poll.
>
> (Yes, we could avoid that problem by including within
> the apr_pollfd_internal_t a list of the descriptors that
> we used on the last poll call for this apr_pollfd_set_t,
> which would make it possible to detect added/removed
> descriptors since the last call. But that would require
> an O(n)-time scan through the entire vector on each request,
> which defeats the scalability advantages of using a
> faster poll interface.)
>
> Speaking of linear-time code, I just checked the current
> apr_poll(), and it's doing an O(n)-time loop to copy from
> the apr_pollfd_t structs to the pollfd structs. That's
> a major scalability problem for larger values of n.
>
> Fortunately, your new design offers a way to fix the O(n)
> problem: build the pollfd vector once, and re-use it on
> subsequent calls if the apr_pollfd_t vector hasn't changed.
> (The transparency of the apr_pollfd_t vector and apr_pollfd_set_t
> makes this a little tricky, though.)
>
> --Brian
>
Re: [design] work around new apr_poll leakage?
Posted by Brian Pane <br...@apache.org>.
William A. Rowe, Jr. wrote:
> At 11:21 PM 7/18/2002, Brian Pane wrote:
>
>> I'm almost in complete agreement with this proposal. We
>> need a wrapper object to hold the temp pollset so that we
>> can re-use it for all subsequent poll calls on the same
>> fd set.
>
>
> Notice that apr_pollfd_internal_t *int; below _IS_ entirely
> opaque... [and using a reserved keyword, the member name
> REALLY sucked... how about simply *private.]
Right, but the pointer to the vector of apr_pollfd_t
objects is transparent. And that's the one that will
cause problems if we try to use something like /dev/poll,
because the set of descriptors in that vector can get
out of synch with the descriptors that have been registered
with /dev/poll.
(Yes, we could avoid that problem by including within
the apr_pollfd_internal_t a list of the descriptors that
we used on the last poll call for this apr_pollfd_set_t,
which would make it possible to detect added/removed
descriptors since the last call. But that would require
an O(n)-time scan through the entire vector on each request,
which defeats the scalability advantages of using a
faster poll interface.)
Speaking of linear-time code, I just checked the current
apr_poll(), and it's doing an O(n)-time loop to copy from
the apr_pollfd_t structs to the pollfd structs. That's
a major scalability problem for larger values of n.
Fortunately, your new design offers a way to fix the O(n)
problem: build the pollfd vector once, and re-use it on
subsequent calls if the apr_pollfd_t vector hasn't changed.
(The transparency of the apr_pollfd_t vector and apr_pollfd_set_t
makes this a little tricky, though.)
--Brian
Re: [design] work around new apr_poll leakage?
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 11:21 PM 7/18/2002, Brian Pane wrote:
>I'm almost in complete agreement with this proposal. We
>need a wrapper object to hold the temp pollset so that we
>can re-use it for all subsequent poll calls on the same
>fd set.
Notice that apr_pollfd_internal_t *int; below _IS_ entirely
opaque... [and using a reserved keyword, the member name
REALLY sucked... how about simply *private.]
>So,
> -1 on the current apr_poll code with the memory leak
> +1 on Bill's proposal to add the apr_pollfd_set_t wrapper object
> -0.5 on making the structures transparent
As long as we have an opaque accessor which we define
[and later redefine as needed] I don't see any huge problem
with keeping both apr_pollfd_t and apr_pollfd_set_t transparent.
Yes, we need extensible and very private storage, but I think
the design below for apr_pollfd_set_t accomplishes this.
Bill
>William A. Rowe, Jr. wrote:
>
>>typedef struct {
>> apr_pool_t *p;
>> apr_pollfd_internal_t *int;
>> apr_pollfd_t *first;
>> int count;
>>} apr_pollfd_set_t;
Re: [design] work around new apr_poll leakage?
Posted by Brian Pane <br...@apache.org>.
I'm almost in complete agreement with this proposal. We
need a wrapper object to hold the temp pollset so that we
can re-use it for all subsequent poll calls on the same
fd set.
The one problem with this design is that the structures
are transparent. It's a bad idea to expose the representation
of the poll set as a vector of apr_pollfd_t elements. If we
later add /dev/poll support, we'll need to call an ioctl when
the app adds a descriptor to an existing poll set. We can make
that happen if the app adds descriptors by calling a function,
but not if it passes in a vector. Yes, we could add a separate
type of object for /dev/poll, and make that one nontransparent,
but then our poll wrapper and our /dev/poll wrapper would have
very different interfaces.
So,
-1 on the current apr_poll code with the memory leak
+1 on Bill's proposal to add the apr_pollfd_set_t wrapper object
-0.5 on making the structures transparent
--Brian
William A. Rowe, Jr. wrote:
> Ok, we have real problems with the palloc in the new design of
> apr_poll().
> But it's fundamentally better than falling back on poll() because our
> design
> just sucks, performance-wise. Ryan speaks of alloca, but that won't
> work on all platforms, most especially stack-impaired platforms such as
> Netware without the ability to grow the stack.
>
> So here's a design suggestion. Take the apr_pollfd_t element;
>
> struct apr_pollfd_t {
> apr_pool_t *p;
> apr_datatype_e desc_type;
> apr_int16_t reqevents;
> apr_int16_t rtnevents;
> apr_descriptor desc;
> };
>
> If we pull the apr_pool_t *p [which is WAY overkill if you have several
> dozen to hundreds of descriptors you might be polling against] and
> create a brand new apr_pollfd_set_t [also transparent!]...
>
> typedef struct {
> apr_pool_t *p;
> apr_pollfd_internal_t *int;
> apr_pollfd_t *first;
> int count;
> } apr_pollfd_set_t;
>
> Modify apr_poll_setup to return a new apr_pollfd_set_t, with *first
> already pointing at a newly allocated apr_pollfd_t array, and set up
> apr_pollfd_internal_t to NULL.
>
> The *int pointer tracks APR's internal count and arrays that it needs.
> If the count to an invocation of apr_poll() exceeds our internally
> allocated
> count, THEN we go and allocate a larger internal structure. But not for
> every pass into apr_poll().
>
> ADVANTAGES
>
> . we stop the leak. Once we grow to some given *int pollset size,
> we won't be growing anymore.
>
> . we lighten the apr_pollfd_t elements by one pool pointer.
>
> . using *first, we can change our offset into a huge array of elements.
>
> . using count, we can resize the array we are interested in.
>
> . neat feature, we actually can have several apr_pollfd_set_t
> indexes floating around, neatly pointing into different parts
> of a huge apr_pollfd_t array.
>
> DISADVANTAGES
>
> . if not using apr_poll_setup(), the user is absolutely responsible
> for initializing *v to NULL.
>
> . two structures to think about. Not complex structures, but two,
> none the less.
>
> Comments or ideas? This was idle brainstorming.
>
> Bill
> .
>
Re: [design] work around new apr_poll leakage?
Posted by Brian Pane <br...@apache.org>.
Ryan Bloom wrote:
>So, the problem that this design is trying to solve is a memory leak. I
>would rather just focus on fixing that problem. So, there are a couple
>of things here, 1) The apr_pool_t that is in the current code is only
>there because I needed it for the apr_palloc(), if that call is removed
>so should the apr_pool_t. 2) This is a multi-pronged approach. The
>assumption is that modern platforms will get the most benefit, but all
>platforms will have the leak removed. With no further ado, the approach
>I would suggest.
>
>The idea is that we will still end up creating the pollfd_t on every
>call to apr_poll(), this does have a performance penalty, but on modern
>platforms, that performance penalty is constant. The only real
>difference is how you allocate the array. There are three options:
>
>1) C99 specifies Variable Length Arrays (VLAs) specifically to solve
>this problem. If your compiler supports it (gcc does on Linux at least,
>as does MSVC I believe), then we create an array on the stack with a
>size of the num argument. (Obviously, we would need some autoconf
>macros to determine if the platform supports VLAs.)
>
>2) For platforms that don't support VLAs, we create an array of size
>FOO (value to be determined later). Because this is allocated on the
>stack, we just need to fill it out and call poll().
>
>3) If the caller needs more than FOO elements, we use malloc() and
>free() to allocate the pollfd_t array. This is an isolated call to
>malloc/free, only required on platforms that don't support VLAs, and can
>be optimized out if the user sets FOO correctly at compile time. (For
>Apache, the default will most likely be < 10, but we can experiment for
>the best value).
>
The only objection I have to this approach is that anybody who
calls poll with a large number of file descriptors (and thus hits
case 3 above) probably has done a lot of work to design a highly
concurrent application that they think will run efficiently. And
they'll be in for an unpleasant surprise when they profile it
and find out that we're doing a malloc/free on every poll request.
IMHO, it's better to say that we won't support more than "FOO"
descriptors through this API than to introduce a major performance
degradation for apps that go beyond that threshold.
I suppose we could have two APIs: the transparent one for small
numbers of descriptors (low overhead, but not scalable), and the
function-encapsulated API for general use (higher overhead, but
very scalable, possibly with support for /dev/poll in the future).
If we went with the two-API approach, it would resolve my
veto on the current API, because the memory leak would go
away and the O(n) scalability problem wouldn't matter if
n was never allowed to exceed, say, 10.
--Brian
RE: [design] work around new apr_poll leakage?
Posted by Ryan Bloom <rb...@covalent.net>.
Okay, so this is essentially exactly what we had before I re-wrote
apr_poll() a few weeks ago. There are some differences, such as keeping
the allocated structure, but at the end of the day any implementation
based on this design will have the same problem that the previous design
had, namely performance.
So, the problem that this design is trying to solve is a memory leak. I
would rather just focus on fixing that problem. So, there are a couple
of things here, 1) The apr_pool_t that is in the current code is only
there because I needed it for the apr_palloc(), if that call is removed
so should the apr_pool_t. 2) This is a multi-pronged approach. The
assumption is that modern platforms will get the most benefit, but all
platforms will have the leak removed. With no further ado, the approach
I would suggest.
The idea is that we will still end up creating the pollfd_t on every
call to apr_poll(), this does have a performance penalty, but on modern
platforms, that performance penalty is constant. The only real
difference is how you allocate the array. There are three options:
1) C99 specifies Variable Length Arrays (VLAs) specifically to solve
this problem. If your compiler supports it (gcc does on Linux at least,
as does MSVC I believe), then we create an array on the stack with a
size of the num argument. (Obviously, we would need some autoconf
macros to determine if the platform supports VLAs.)
2) For platforms that don't support VLAs, we create an array of size
FOO (value to be determined later). Because this is allocated on the
stack, we just need to fill it out and call poll().
3) If the caller needs more than FOO elements, we use malloc() and
free() to allocate the pollfd_t array. This is an isolated call to
malloc/free, only required on platforms that don't support VLAs, and can
be optimized out if the user sets FOO correctly at compile time. (For
Apache, the default will most likely be < 10, but we can experiment for
the best value).
Obviously FOO will need to be named better, but that is only important
if we take this route.
Ryan
> Ok, we have real problems with the palloc in the new design of
apr_poll().
> But it's fundamentally better than falling back on poll() because our
> design
> just sucks, performance-wise. Ryan speaks of alloca, but that won't
> work on all platforms, most especially stack-impaired platforms such
as
> Netware without the ability to grow the stack.
>
> So here's a design suggestion. Take the apr_pollfd_t element;
>
> struct apr_pollfd_t {
> apr_pool_t *p;
> apr_datatype_e desc_type;
> apr_int16_t reqevents;
> apr_int16_t rtnevents;
> apr_descriptor desc;
> };
>
> If we pull the apr_pool_t *p [which is WAY overkill if you have
several
> dozen to hundreds of descriptors you might be polling against] and
> create a brand new apr_pollfd_set_t [also transparent!]...
>
> typedef struct {
> apr_pool_t *p;
> apr_pollfd_internal_t *int;
> apr_pollfd_t *first;
> int count;
> } apr_pollfd_set_t;
>
> Modify apr_poll_setup to return a new apr_pollfd_set_t, with *first
> already pointing at a newly allocated apr_pollfd_t array, and set up
> apr_pollfd_internal_t to NULL.
>
> The *int pointer tracks APR's internal count and arrays that it needs.
> If the count to an invocation of apr_poll() exceeds our internally
> allocated
> count, THEN we go and allocate a larger internal structure. But not
for
> every pass into apr_poll().
>
> ADVANTAGES
>
> . we stop the leak. Once we grow to some given *int pollset size,
> we won't be growing anymore.
>
> . we lighten the apr_pollfd_t elements by one pool pointer.
>
> . using *first, we can change our offset into a huge array of
elements.
>
> . using count, we can resize the array we are interested in.
>
> . neat feature, we actually can have several apr_pollfd_set_t
> indexes floating around, neatly pointing into different parts
> of a huge apr_pollfd_t array.
>
> DISADVANTAGES
>
> . if not using apr_poll_setup(), the user is absolutely responsible
> for initializing *v to NULL.
>
> . two structures to think about. Not complex structures, but two,
> none the less.
>
> Comments or ideas? This was idle brainstorming.
>
> Bill
> .
>