apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Erik Huelsmann" <ehu...@gmail.com>
Subject Re: [PATCH] Fix for Subversion issue #1789
Date Wed, 14 Mar 2007 23:35:29 GMT
On 3/14/07, Jonathan Gilbert <2jx64e902@sneakemail.com> wrote:
> At 02:22 PM 3/14/2007 +0100, Eric Huelsmann wrote:
> >Jonathan,
> >
> >Any progress on the matter? I'd love to be able to close the issue in
> >Subversion by pointing to the right APR version!
>
> My feeling at the moment is that the patch is ready to be applied, but Bill
> Rowe does not agree. I did some performance tests to see just what the
> effect of buffering would be, and when run on UNIX systems, I saw the
> expected ~4% degradation when splitting up large writes into 30 KB blocks.
> When run on Windows, however, I saw a massive performance *increase*, a
> factor of more than 3. So, a patch which unconditionally breaks writes up
> into 30 KB blocks would apparently be beneficial for Windows in all cases.

> There are currently two patches out there that I've submitted; one of them
> modifies the Win32 apr_file_write directly so that all writes are split,
> and the other one uses APR's built-in buffering on all console-related
> handles (and console handles only). Either one would fix the problem of
> 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. 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:

Index: file_io/win32/readwrite.c
===================================================================
--- file_io/win32/readwrite.c    (revision 518377)
+++ file_io/win32/readwrite.c    (working copy)
@@ -312,6 +312,10 @@
             }
             rv = WriteFile(thefile->filehand, buf, (DWORD)*nbytes, &bwrote,
                            thefile->pOverlapped);
+            if (rv == ERROR_INSUFFICIENT_BUFFER && *nbytes > (32*1024))
+              rv = WriteFile(thefile->filehand, buf,
(DWORD)(32*1024), &bwrote,+
thefile->pOverlapped);
+
             if (thefile->append) {
                 apr_file_unlock(thefile);
                 apr_thread_mutex_unlock(thefile->mutex);
@@ -320,6 +324,9 @@
         else {
             rv = WriteFile(thefile->filehand, buf, (DWORD)*nbytes, &bwrote,
                            thefile->pOverlapped);
+            if (rv == ERROR_INSUFFICIENT_BUFFER && *nbytes > (32*1024))
+              rv = WriteFile(thefile->filehand, buf,
(DWORD)(32*1024), &bwrote,+
thefile->pOverlapped);
         }
         if (rv) {
             *nbytes = bwrote;
@@ -485,7 +492,13 @@
                     numbytes = (DWORD)bytesleft;
                 }

-                if (!WriteFile(thefile->filehand, buffer, numbytes,
&written, NULL)) {
+                rv = WriteFile(thefile->filehand, buffer, numbytes,
+                               &written, NULL);
+                if (rv == ERROR_INSUFFICIENT_BUFFER && numbytes > (32*1024))
+                  rv = WriteFile(thefile->filehand, buffer, (DWORD)(32*1024),
+                                 &bwrote, NULL);
+
+                if (!rv)
                     rc = apr_get_os_error();
                     thefile->filePtr += written;
                     break;

Bill, Jonathan, could the two of you agree on this patch? Joe?

If necessary, I can provide a patch for backporting to 0.9.x too.

Hope that helps.

bye,

Erik.

Mime
View raw message