Jeff:
I think you misunderstood my point here. The preallocation is actually desirable as it gives the sockets closure relative to pool dynamics. In simplified English, that means that we can allocate a socket structure (apr_socket_create), and when we deallocate it, all that we need goes away. The problem with the code I modified is that it causes a dependence to occur on a structure which may not be in the same pool-space. If the pointer is instead copied to the preallocated structure, I can get rid of the original value in a more local pool, and let the socket take care of the data it needs when it is finished with it. This enables us to have cleaner interface to the sockets. Otherwise (if you take out the preallocation of the needed elements), you would really have to specify somewhere in the docs that the structure being pointed to would require persistence for the life of the socket. This is really easier to manage through the socket itself.
-Norman Tuttle [EMAIL PROTECTED]
Jeff Trawick wrote:
Norman Tuttle wrote:
While trying to memory-tune code which we are working on based on Apache Flood, an apr project, I noticed some potentially memory-leak prone code. While alloc_socket() called by apr_socket_create() does an apr_palloc() to allocate memory for a socket's remote_addr member, the apr_socket_connect() function sets the remote_addr directly to the apr_sockaddr_t * which it passes. The problem is that that defeats the purpose of allocating a buffer for it, since you're replacing with a new pointer which presumably also had to allocate its space (and the typical apr_socket_addr_get() would do that). So we need to use a memcpy() instead!
not really a leak; I'd call the preallocation of those sockaddrs at socket creation time "unfortunate overhead" ;) for it to be a leak, continued operations on a particular socket would have to result in (unnecessary) storage growth
I don't see any benefit in doing the memcpy instead of the pointer copy. It just slows down the application (barring any CPU cache problems related to locality of the different sockaddrs, which I shouldn't even pretend to be able to point out) and doesn't save any memory.
The interesting question IMO is can we get rid of the unfortunate overhead -- the preallocation of sockaddrs at socket creation time. I believe that is required to support the ancient ideom
apr_socket_create(&sock); apr_socket_addr_get(&remote_addr, REMOTE, sock); apr_sockaddr_ip_set(remote_addr, "1.2.3.4"); apr_sockaddr_port_set(remote_addr, 80); apr_connect(sock, NULL);
and furthermore that the ideom is inherently busted (IPv4 numeric address strings only? yuck!) and furthermore apr_socket_connect(), on Unix at least, hasn't allowed the sockaddr to be NULL for some time, in contradiction to its documentation :) Yet more cruft that I didn't come back and clean up at the appropriate time.
/* XXX assumes IPv4... I don't think this function is needed anyway
* since we have apr_sockaddr_info_get(), but we need to clean up Apache's
* listen.c a bit more first.
*/
APR_DECLARE(apr_status_t) apr_sockaddr_ip_set(apr_sockaddr_t *sockaddr,
const char *addr)
I'll start a new thread on some networking apis to remove and give folks a chance to holler. Once that is resolved, I suspect you'll find that the unfortunate overhead is now worthless, and some alternative patch can clean up that issue.
|PS Question: how many similar issues exist in the current state of the APR?
Is this the lead-in to the not-getting-your-money's-worth-? speech or the we-are-humble-programmers-in-need-of-your-help speech? I can't keep it straight.
