On 3/14/07, Jonathan Gilbert <[EMAIL PROTECTED]> 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.
