apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "William A. Rowe, Jr." <wr...@rowe-clan.net>
Subject Re: [PATCH] Fix for Subversion issue #1789
Date Thu, 15 Mar 2007 06:27:22 GMT
Jonathan Gilbert wrote:
> At 12:35 AM 3/15/2007 +0100, Erik Huelsmann wrote:
>> On 3/14/07, Jonathan Gilbert <2jx64e902@sneakemail.com> wrote:
> [snip]
>>> large writes to consoles, but some people seem to think it would be better
>>> to always try the large write and then switch to buffered output if the
>>> write attempt fails with the "Insufficient buffer space" error.
>> Yes, that's the general idea I got from Joe.
>>
>>> I think in
>>> principle this makes sense, but in practice it doesn't since there is
>>> actually a performance penalty on the platform for doing large writes in
>>> single calls to WriteFile.
>> Possibly, but it's also an edge-case to use large to even extremely
>> large writes on console handles, so I do really see some benefit in
>> not making this change more complex than necessary.
> 
> Note that the original patch I submitted does the splitting on all writes.
> For files, this was an enormous performance win. At the very worst, I'd
> think if pipes had more sensible characteristics one might see a very small
> performance degradation. There is also a fair chance that pipes could have
> exactly the same performance characteristics as files, or alternately that
> they could have exactly the same behavioural problems as consoles (console
> handles might actually *be* pipes, under the hood). I could whip up a test
> program to see some time.

Again, it's clear to me this is an OS issue, if windows is taking a very
significant and measurable performance hit for writing large buffers, it
tells me they have a significant system bug that isn't our issue to address
and I'm strongly -1 for attempting to work around such core performance
issues when the vendor should fix the bleeding OS.

Speaking to consoles and pipes...

>> To help this change get back on track, I created my own patch (but note: I
>> don't have a Windows dev environment!).
>>
>> This patch does nothing but fall back to a smaller write when there is
>> an "insufficient buffer" error *and* the write was larger than the
>> chunk you've determined to be writeable (ie 30kB). This is it:
>>
> [snip]
>> Bill, Jonathan, could the two of you agree on this patch? Joe?
> 
> I'm a frayed knot. The principle is similar to what Joe suggested, but it
> has two problems:
> 
> 1) It increases the depth of the round-trip through stack frames when
> someone is pumping data. It also requires that people have a
> correctly-implemented loop for doing full writes (though as Joe points out,
> there *is* always apr_file_write_full).

I'm equally neutral on this aspect.

> 2) More importantly, when the bug is being encountered, if a genuinely
> large write is done, it will repeatedly try an oversize write on every call
> except the last one. That is, if the buffer being written has n 30kb
> chunks, there will be 2n + 1 calls to WriteFile, and every other one will
> trigger the error response.

Agreed, we *should* cache the state on the first failure.  We don't have
a fast mechanism to know which will fail until the first has failed, but
we don't have to repeat this hit subsequent writes.

> At the very least, I think a patch designed to work in this way should
> switch the apr_file_t into buffered mode when the error is encountered,
> something along the lines of:
>
> Index: include/arch/win32/apr_arch_file_io.h
> ===================================================================
> --- include/arch/win32/apr_arch_file_io.h	(revision 515905)
> +++ include/arch/win32/apr_arch_file_io.h	(working copy)
> @@ -86,6 +86,14 @@
>  /* For backwards-compat */
>  #define APR_FILE_BUFSIZE APR_FILE_DEFAULT_BUFSIZE
>  
> +/* The WriteFile API fails when used on a console handle if the data
> + * buffer is too large. On Windows XP SP2, the boundary seems to be
> + * around 60 KB, but on earlier versions of Windows, it may be around
> + * 32,000 bytes. This buffer size is the largest multiple of the Win32
> + * page size less than 32,000.
> + */
> +#define APR_CONSOLE_BUFSIZE 28672
> +
>  /* obscure ommissions from msvc's sys/stat.h */
>  #ifdef _MSC_VER
>  #define S_IFIFO        _S_IFIFO /* pipe */
> @@ -167,6 +175,7 @@
>      DWORD dwFileAttributes;
>      int eof_hit;
>      BOOLEAN buffered;          // Use buffered I/O?
> +    BOOLEAN autoflush;         // Automatically flush the buffer after
> each write?
>      int ungetchar;             // Last char provided by an unget op. (-1 =
> no char)
>      int append; 
>  
> Index: file_io/win32/readwrite.c
> ===================================================================
> --- file_io/win32/readwrite.c	(revision 515905)
> +++ file_io/win32/readwrite.c	(working copy)
> @@ -326,9 +326,60 @@
>              rv = APR_SUCCESS;
>          }
>          else {
> -            (*nbytes) = 0;
>              rv = apr_get_os_error();
> -            if (rv == APR_FROM_OS_ERROR(ERROR_IO_PENDING)) {
> +            if (rv == APR_FROM_OS_ERROR(ERROR_INSUFFICIENT_BUFFER)) {
> +                /* This error occurs when performing writes larger than
> +                 * ~30kb to console handles. We respond by making the
> +                 * handle buffered with a 28kb buffer size (as 28kb is
> +                 * a multiple of the page size on Win32).
> +                 *
> +                 * First, though, if we're in append mode then we can't
> +                 * use this strategy because buffered writing isn't
> +                 * compatible with append mode.
> +                 */
> +                if (thefile->append) {
> +                    /* If the write size seems to have been on the large
> +                     * side, then retry with a smaller size, otherwise
> +                     * simply return the error to the caller.
> +                     */
> +                    if (*nbytes > APR_CONSOLE_BUFSIZE)
> +                    {
> +                        *nbytes = APR_CONSOLE_BUFSIZE;
> +                        rv = apr_file_write(thefile, buf, nbytes);
> +                    }
> +
> +                    return rv;
> +                }

+1 to failing on any unexpected small write, although if the pipe buffer
was opened with, say, 8kb, a second fallback size wouldn't be unreasonable.

> +
> +                /* Bring on the buffering, as we're not in append mode. */
> +                thefile->buffered = 1;
> +                thefile->autoflush = 1;
> +                thefile->bufsize = APR_CONSOLE_BUFSIZE;
> +                thefile->buffer = apr_palloc(thefile->pool,
> thefile->bufsize);
> +
> +                /* As we were not already in append mode, we do not
> +                 * have a mutex, which is required for buffered I/O.
> +                 */
> +                rv = apr_thread_mutex_create(&thefile->mutex, 
> +                                APR_THREAD_MUTEX_DEFAULT, thefile->pool);
> +                if (rv) {
> +                    /* Mutex creation failed, so cancel the direct
> +                     * buffering strategy. Let the caller figure out
> +                     * what to do with the error, since if a mutex
> +                     * can't be created, there's probably something
> +                     * else wrong.
> +                     */
> +                    thefile->buffered = 0;
> +                    return rv;
> +                }
> +
> +                /* Retry the write, this time with buffering. Return
> +                 * the result directly, since we don't want the file
> +                 * pointer to be incremented a second time below.
> +                 */
> +                return apr_file_write(thefile, buf, nbytes);
> +            }
> +            else if (rv == APR_FROM_OS_ERROR(ERROR_IO_PENDING)) {
>   
>                  DWORD timeout_ms;
>  
> @@ -364,6 +415,8 @@
>                          CancelIo(thefile->filehand);
>                  }
>              }
> +
> +            (*nbytes) = 0;
>          }
>          if (rv == APR_SUCCESS && thefile->pOverlapped && !thefile->pipe)
{
>              thefile->filePtr += *nbytes;
> 
> The patch is untested, but it seems to cover all bases on the surface. It
> doesn't check the return value of apr_pcalloc, but then neither does the
> code in file_io/win32/open.c that I leeched the buffer setup from. Is this
> the correct way to use apr_pcalloc?

Right - we consider pcalloc failures critical failures and let the allocator
callback signal this death-state.  No issue there.

My gut says the extensive additional code to flip to buffered mode merits
it's very own function (internal to apr would be fine, for now, but the
fact that we need to trigger it hints that others might want to too, in
some future 1.3.0 release.)

We can't return 0 bytes written, too many apps will presume 0 means file
closed, other signals tripped, error etc.  We should do at least one
short-write before returning in either case.

Thoughts?

Mime
View raw message