Hi Christian, On Fri, Nov 06, 2015 at 11:00:05PM +0100, Christian Grothoff wrote: > Reproduced, located, fixed: SVN 36643 (diff attached). > Basically, contrary to what I was thinking, MHD wasn't somehow having an > issue with the pool having allocations in it that would limit the > available memory. The issue was simply that MHD would at the end of > reading a request shrink the read buffer to the size that was needed for > the request (to have more space for the write buffer). Then, for the > next request, it would *only* grow the read buffer if it was strictly > necessary (and not use the usual more generous allocation of half of the > memory anymore). That's also why it never failed: if the read buffer > was actually to small, MHD would eventually grow it. But that would only > happen after the IO buffer had gotten MUCH too small for performance. > > Combined with the fact that it would take a while for the shrinkage to > kick in (long-lasting multi-request POST session), this was not commonly > observed. > > Anyway, should now really be killed. Thanks for the report & the testcase!
Yes :-) Thanks a lot: tested against SVN 36643 I can not reproduce this now. Perfect. Happy Hacking ;-) ! > On 11/06/2015 10:09 PM, Olivier Delhomme wrote: > > On Wed, Nov 04, 2015 at 11:00:42PM +0100, Christian Grothoff wrote: > >> Hmm. That's not even clearly indicative of an MHD-related issue. > > > > I'm even not sure that I'm doing things the right way... > > > >> What you should look at is the buffer size MHD offers when calling > >> recv() on the TCP stream. The size of the buffer MHD offers to the > >> application can be smaller simply because the OS didn't receive 13k data > >> chunks between calls anymore -- be it because the sender is slower, TCP > >> had a congestion event or your receiver happens to be draining the > >> buffer slightly faster. So there are many _other_ reasons that could > >> explain a decrease in upload_buffer_size. > >> > >> So to diagnose this better, you first should check if the cause is > >> really the size of the IO buffer offered by MHD to the kernel. Given > >> that this is all for the same connection, I doubt this is the cause --- > >> unless your application again doesn't always handle 100% of the data in > >> each call, then you could of course also see such patterns emerge. > > > > I'm sorry but I'm not sure to understand what your are saying as I'm > > not an expert in TCP nor network related kernel things. > > > > To diagnose it I managed to code a minimalistic server (160 lines with > > comments) and a minimalistic client (200 lines with comments) that > > simulates the behavior of my programs. It is published here: > > https://github.com/dupgit/shrinkage > > > > I found the same behavior regarding the *upload_size value as you can > > see in the *.png images and the file shrinkserver.output. The whole > > run to observe this is about 6-7 minutes. I use the lo interface and > > the throughput is around 80 Mbits/s. It has been tested on my laptop > > that was running gnome desktop with very few opened applications. > > The distribution is a Debian Jessie. shrinkserver is linked with > > libmicrohttpd r36641 and shrinkclient is linked with system's libcurl > > 7.38.0-4+deb8u2. > > > > Can you tell me if this code leads to something similar with your > > environment ? > Index: ChangeLog > =================================================================== > --- ChangeLog (revision 36640) > +++ ChangeLog (working copy) > @@ -1,3 +1,6 @@ > +Fri Nov 6 22:54:38 CET 2015 > + Fixing the buffer shrinkage issue, this time with test. -CG > + > Tue Nov 3 23:24:52 CET 2015 > Undoing change from Sun Oct 25 15:29:23 CET 2015 > as the original code was counter-intuitive but > Index: src/include/microhttpd.h > =================================================================== > --- src/include/microhttpd.h (revision 36640) > +++ src/include/microhttpd.h (working copy) > @@ -130,7 +130,7 @@ > * Current version of the library. > * 0x01093001 = 1.9.30-1. > */ > -#define MHD_VERSION 0x00094600 > +#define MHD_VERSION 0x00094601 > > /** > * MHD-internal return code for "YES". > Index: src/microhttpd/connection.c > =================================================================== > --- src/microhttpd/connection.c (revision 36640) > +++ src/microhttpd/connection.c (working copy) > @@ -2559,14 +2559,15 @@ > /* can try to keep-alive */ > connection->version = NULL; > connection->state = MHD_CONNECTION_INIT; > - /* read_buffer_size is correct here, as we want to > - preserve the entire read buffer allocation, even > - if in terms of the data we only care to preserve > - up to "read_buffer_offset" */ > + /* Reset the read buffer to the starting size, > + preserving the bytes we have already read. */ > connection->read_buffer > = MHD_pool_reset (connection->pool, > connection->read_buffer, > - connection->read_buffer_size); > + connection->read_buffer_offset, > + connection->daemon->pool_size / 2); > + connection->read_buffer_size > + = connection->daemon->pool_size / 2; > } > connection->client_aware = MHD_NO; > connection->client_context = NULL; > Index: src/microhttpd/memorypool.c > =================================================================== > --- src/microhttpd/memorypool.c (revision 36640) > +++ src/microhttpd/memorypool.c (working copy) > @@ -219,7 +219,8 @@ > if ((pool->end < old_size) || (pool->end < asize)) > return NULL; /* unsatisfiable or bogus request */ > > - if ((pool->pos >= old_size) && (&pool->memory[pool->pos - old_size] == > old)) > + if ( (pool->pos >= old_size) && > + (&pool->memory[pool->pos - old_size] == old) ) > { > /* was the previous allocation - optimize! */ > if (pool->pos + asize - old_size <= pool->end) > @@ -251,32 +252,40 @@ > > /** > * Clear all entries from the memory pool except > - * for @a keep of the given @a size. > + * for @a keep of the given @a size. The pointer > + * returned should be a buffer of @a new_size where > + * the first @a copy_bytes are from @a keep. > * > * @param pool memory pool to use for the operation > * @param keep pointer to the entry to keep (maybe NULL) > - * @param size how many bytes need to be kept at this address > + * @param copy_bytes how many bytes need to be kept at this address > + * @param new_size how many bytes should the allocation we return have? > + * (should be larger or equal to @a copy_bytes) > * @return addr new address of @a keep (if it had to change) > */ > void * > MHD_pool_reset (struct MemoryPool *pool, > void *keep, > - size_t size) > + size_t copy_bytes, > + size_t new_size) > { > if (NULL != keep) > { > if (keep != pool->memory) > { > - memmove (pool->memory, keep, size); > + memmove (pool->memory, > + keep, > + copy_bytes); > keep = pool->memory; > } > } > pool->end = pool->size; > - memset (&pool->memory[size], > + /* technically not needed, but safer to zero out */ > + memset (&pool->memory[copy_bytes], > 0, > - pool->size - size); > + pool->size - copy_bytes); > if (NULL != keep) > - pool->pos = ROUND_TO_ALIGN(size); > + pool->pos = ROUND_TO_ALIGN (new_size); > return keep; > } > > Index: src/microhttpd/memorypool.h > =================================================================== > --- src/microhttpd/memorypool.h (revision 36640) > +++ src/microhttpd/memorypool.h (working copy) > @@ -99,16 +99,21 @@ > > /** > * Clear all entries from the memory pool except > - * for "keep" of the given "size". > + * for @a keep of the given @a copy_bytes. The pointer > + * returned should be a buffer of @a new_size where > + * the first @a copy_bytes are from @a keep. > * > * @param pool memory pool to use for the operation > * @param keep pointer to the entry to keep (maybe NULL) > - * @param size how many bytes need to be kept at this address > - * @return addr new address of "keep" (if it had to change) > + * @param copy_bytes how many bytes need to be kept at this address > + * @param new_size how many bytes should the allocation we return have? > + * (should be larger or equal to @a copy_bytes) > + * @return addr new address of @a keep (if it had to change) > */ > void * > MHD_pool_reset (struct MemoryPool *pool, > void *keep, > - size_t size); > + size_t copy_bytes, > + size_t new_size); > > #endif -- "Quand la vérité n'est pas libre, la liberté n'est pas vraie." [Jacques Prévert]
