Mailing-List: contact dev-help@apr.apache.org; run by ezmlm
Precedence: bulk
Received-SPF: pass (herse.apache.org: local policy)
Message-ID: <23138-25897@sneakemail.com>
Date: Thu, 15 Mar 2007 01:02:18 -0500
To: dev@apr.apache.org
From: "Jonathan Gilbert" <2jx64e902@sneakemail.com>
Subject: Re: [PATCH] Fix for Subversion issue #1789
In-Reply-To: <aea328ab0703141635raf07602va815da351d516021@mail.gmail.com
 >
References: <18935-64893@sneakemail.com>
 <15856-75392@sneakemail.com>
 <45DE9A16.1080202@rowe-clan.net>
 <18935-64893@sneakemail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"

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.

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

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.

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;
+                }
+
+                /* 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?

I'm pretty sure my e-mail client will have mangled this patch, so if people
like it, I can provide it as a gzipped attachment as I have the other patches.

Jonathan Gilbert