apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jonathan Gilbert" <2jx64e...@sneakemail.com>
Subject Re: [PATCH] Fix for Subversion issue #1789
Date Mon, 19 Feb 2007 16:39:38 GMT
At 03:16 AM 2/19/2007 -0600, William A. Rowe, Jr. wrote:
>Jonathan Gilbert wrote:
>> 
>> This is a repost of a message sent to the Subversion development mailing
>> list containing a patch for APR.
>
>Thanks for forwarding.  In the future, you might want to also log this sort
>of patch to issues.apache.org/bugzilla/ - to catch the maintainer's attention
>the PatchAvailable bug keyword marks it as very-low-hanging fruit.

Okay. :-)

>Question, are you the author?  If not, could that author please submit us
>their patch directly so we follow our IP constraints?  Otherwise this
>discussion is off the table till I have time to have someone else look at
>this patch who hasn't followed the emails.

I am the author of the patch. :-)

>> Windows has a bug in its WriteFile API. When the file handle is a console
>> output handle and the data chunk is greater than some value around 60 KB,
>> instead of doing either a complete write (which would be ideal) or a
>> partial write (which is permitted by the API contract), it returns an
>> incorrect error causing Subversion to display an error message such as:
>
>I've heard 32000 at one time /shrug.  It might vary by os release.

That is entirely possible :-) I did do an extremely simple test case on
Windows XP Service Pack 2 and found that a single 70 KB block could not be
written without the patch, but went through fine as a 48 KB block followed
by a 22 KB block with the patch applied, which would support the ~60 KB the
original bug reporter gave in the Subversion Issue Tracker, but that's the
only OS I have to test on.

>Forget it :)  I believe you underestimate things on a major level; consider
>someone copying an ISO image.  VERY measurable in terms of kernel state
>transitions, depending on what the destination driver/device is.

Consider that Windows itself, when copying a file from one place to another
in, say, Exploder uses 64 KB blocks for that copy operation. Are you
suggesting that Exploder's file copies are significantly slower than they
would be if they used, say, a 2 MB block for the copy operation? CMD.EXE
uses the same block size, and other utilities often use smaller blocks.

I could do a benchmark, though I don't have any particularly high-end
hardware to try this on, but my experience in the past has been that the
overhead becomes close to insignificant anywhere over 8 KB.

In any event, I think your proposal below makes sense :-)

>> I've attached a patch which achieves this, however after an hour and a half
>> of trying to work with the Subversion build system for Windows (which I am
>> not familiar with), I was not able to get a working build. So, I present
>> this patch as "carefully reviewed & probably good but untested". I was
>> careful to preserve the semantics of the border case where *nbytes is
>> initially 0; as before, the function still calls WriteFile() with a count
>> of 0 once.
>
>Thanks for the heads up :)

For what it's worth, as mentioned above, I did do a minimal unit test of
the APR function itself; I just wasn't able to try it through Subversion
and see if I had actually patched the place where console output from the
command-line utility eventually reached.

>> If anyone would like to give me some hints on building under Windows, that
>> would also be great. I used "gen-make.py -t vcproj" as per the
>> instructions, but ended up with projects that do not reference their
>> dependencies and DLLs that do not export any functions at all.
>
>No clue at all, FWIW if all you are building is apr, there is a visual
>studio .dsw (workspace) that modern studios will convert to the .sln
>solution file on open.  The -win32-src.zip sources include a makefile
>generated from the .dsw/.dsp files.  (.sln/.vcproj files won't export
>to .mak files, so we've refused to migrate so far.)
>
>As far as gen-make.py - I don't personally use it so can't vouch for it.

Yeah, actually I had no problems at all building APR :-) It worked perfectly.

Someone on the Subversion list suggested that perhaps Visual Studio needs
to be able to find the Python runtime in order to perform the build, so I'm
going to try that at some point and see if it fixes the problem -- with any
luck, it will, and then I'll be able to say whether my patch cured
Subversion's symptoms.

>> + * "Not enough storage is available to process this command."
>> + */
>> +#define MAXIMUM_SYNCHRONOUS_WRITE_SIZE (48 * 1024)
>
>Safer to set this to 32000 per earlier console pipe documention.
>
>I'd veto the solution below, as I noted above you make a really horrid
>assumption that writing small chunks would be efficient for writing
>out tens or hundreds of megabytes to disk.  But we have a small,
>saving grace...

I don't think 48 KB chunks aren't really particularly small, but..

>"Can't write to stream: Not enough storage is available to process this
>command."
>
>What if we caught that exception after the existing writefile statement,
>and in the specific case of that error, we were to use your patch below
>to shuttle in one 32000 segment at a time?  It would significantly slow
>those writes for drivers or pipes subject to this limit, BUT I believe
>that's the rare exception (most writes being within the size) and we
>benefit from not slowing down 'just for consoles'.
>
>What are your thoughts on that approach?

It sounds sensible, though it will require more structural change to the
Win32 apr_file_write function than the current patch. I'll take a look at
it tonight.

Jonathan Gilbert

Mime
View raw message