Subject: Re: [design] work around new apr_poll leakage? - msg#00599

Previous Message by Date: click to view message preview

Re: [design] work around new apr_poll leakage?

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

Next Message by Date: click to view message preview

Re: [design] work around new apr_poll leakage?

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

Previous Message by Thread: click to view message preview

RE: [design] work around new apr_poll leakage?

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 > . >

Next Message by Thread: click to view message preview

[PATCH] apr/threadproc/win32/proc.c:apr_proc_create()

The HANDLE members in the STARTUPINFO struct used in the call to CreateProcess() aren't currently initialized properly... --rob diff -u -r1.80 proc.c --- proc.c 5 Jul 2002 17:58:10 -0000 1.80 +++ proc.c 18 Jul 2002 04:31:54 -0000 @@ -590,6 +590,10 @@ memset(&si, 0, sizeof(si)); si.cb = sizeof(si); + si.hStdInput = INVALID_HANDLE_VALUE; + si.hStdOutput = INVALID_HANDLE_VALUE; + si.hStdError = INVALID_HANDLE_VALUE; + if (attr->detached) { si.dwFlags |= STARTF_USESHOWWINDOW; si.wShowWindow = SW_HIDE;