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 .
