What's wrong with this code, part 9 - File Copy

re: What's wrong with this code, part 9 - File Copy

B.Y. — Wed, 09 Feb 2005 18:10:00 GMT

Well I can see:
1. No error checking on QueueUserWorkItem() call.
2. Unsynchronized file access from multiple threads (set pointer, read, write).
3. Unsynchronized access to *dataLengthWritten / fileSizeCopied.
4. If the while loop returns, fileSizeCopied goes out of scope. If this happens just before one or more threads execute:

dataBlock->dataLengthWritten ->
QuadPart += dataBlock
->readLength;

stack is trashed.
5. Creating a thread to read and write 64K is really bad. It's better to have one read thread and one write thread.

re: What's wrong with this code, part 9 - File Copy

Skywing — Wed, 09 Feb 2005 18:11:00 GMT

The first thing that comes to mind is no synchronization with the number of bytes written.

re: What's wrong with this code, part 9 - File Copy

Larry Osterman — Wed, 09 Feb 2005 18:13:00 GMT

B.Y. #1 is one I missed :(
#2 is the complicated issue.
#3 is the simple coding issue
#4 can't happen - the loop doesn't execute until after all the threads have completed their reads and updated their value.

And #5 touches on one of of the design issues... But not clearly - there's an explicit reason why its bad.

re: What's wrong with this code, part 9 - File Copy

Stuart Ballard — Wed, 09 Feb 2005 18:32:00 GMT

Is it going to create horribly fragmented files if a block late in the file is written before the earlier blocks?

(I'm not personally familiar with the APIs you're using and most of my own coding is in C#, so I'm guessing as to the behavior of, particularly, the SetFilePointerEx() function)

re: What's wrong with this code, part 9 - File Copy

Larry Osterman — Wed, 09 Feb 2005 18:42:00 GMT

Stuart, yes, possibly - not likely, but possibly. The original concept for the code had a call to SetFIlePositionEx on the destination to match the size of the source file but I pulled it to conserve space.

That would have avoided the issue completely (because the SetFilePositionEx would have extended the file and ensured that the file had storage reserved on the destination).

re: What's wrong with this code, part 9 - File Copy

B.Y. — Wed, 09 Feb 2005 18:47:00 GMT

#4: the first while loop returns if "new FileCopyBlock" returns NULL.

re: What's wrong with this code, part 9 - File Copy

Larry Osterman — Wed, 09 Feb 2005 18:49:00 GMT

B.Y. - Ah, I missed that - you're right, it should be exit(1) in that failure case (which makes the corruption moot).

re: What's wrong with this code, part 9 - File Copy

Harald — Wed, 09 Feb 2005 19:03:00 GMT

Alternate data streams don't get copied (:Zone.Identifier:$DATA gets lost)

The slow part, when copying files are the disks. The code converts a sequentiell access pattern to a possibly random pattern.

The whole code does not have any benefit over using the api CopyFile/CopyFileEx that does not have the problems metioned above.

re: What's wrong with this code, part 9 - File Copy

Paul Winwood — Wed, 09 Feb 2005 19:19:00 GMT

I think FILE_READ_DATA should be GENERIC_READ because GetFileSizeEx requires GENERIC_READ.
I would not use FILE_READ_DATA with CreateFile as its only documented for ZwCreateFile().

re: What's wrong with this code, part 9 - File Copy

Larry Osterman — Wed, 09 Feb 2005 19:37:00 GMT

Harald, I did mention that alternate data streams got lost and that it wasn't one of the bugs.

On the other hand, your second point is 100% spot on, it hits on two of the major issues with this code.

Paul: Are you sure? If GetFileSize requires FILE_READ_ATTRIBUTES, then the GetFileSizeEx call should be replaced with SetFilePointerEx(handle, 0, SEEK_END), since it returns the same information.

re: What's wrong with this code, part 9 - File Copy

Paul Winwood — Wed, 09 Feb 2005 19:55:00 GMT

Ah! You are right GENERIC_READ is the miniumum for 9x/Me only.
Of course the advantage of GetFileSize(Ex) is that it does not move the current position.

re: What's wrong with this code, part 9 - File Copy

B.Y. — Wed, 09 Feb 2005 20:12:00 GMT

I guess you really just need two buffers and use overlapped read and write from your main thread, but I think this is an advantage only if the two files are on separate phyiscal drives.

re: What's wrong with this code, part 9 - File Copy

Larry Osterman — Wed, 09 Feb 2005 20:19:00 GMT

B.Y. Bingo - that's one of the huge issues with the code: overlapping I/O only really helps if you've got more than one spindle.

Otherwise you spend all your time moving the heads back and forth.

There are still a couple of design issues outstanding that haven't been touched upon but most of the big ones have been caught.

There's still at least one really big one though.

re: What's wrong with this code, part 9 - File Copy

Capt. Jean-Luc Pikachu — Wed, 09 Feb 2005 20:24:00 GMT

I thought new couldn't return NULL, it could only throw an exception? Or is that how FileCopyBlock's constructor works?

re: What's wrong with this code, part 9 - File Copy

Johnny Lee — Wed, 09 Feb 2005 20:39:00 GMT

How about:
1) CreateFile on Win9X fails on the dest because there's a template file supplied - so says MSDN.
2) is the code guaranteed srcFile != destFile?
3) You could probably tweak the CreateFile flags using FILE_FLAG_NO_BUFFERING and FILE_FLAG_SEQUENTIAL_SCAN, but your algorithm would have to be modified to ensure sequential scan and using appropriately aligned buffers.

re: What's wrong with this code, part 9 - File Copy

Larry Osterman — Wed, 09 Feb 2005 20:54:00 GMT

Jean-Luc: new can return NULL unless you use the throwing version of new. And if you've been reading for a while, you know how I feel about exceptions :) Hint: I don't like them and avoid them whenever possible.

Johnny: Win9x? What's that?
2: Good catch, not a bug: The code will fail in that case because the call to CreateFile on the destination will fail.
3) I'll talk about FILE_FLAG_NO_BUFFERING a bit in my "answers" post - that's a really good point.

re: What's wrong with this code, part 9 - File Copy

Mike Dimmick — Wed, 09 Feb 2005 21:07:00 GMT

Since you're using new and presumably haven't overloaded operator new, your threads will be contending for the process heap's critical section. That's a fairly minor issue.

The thread pool scheduler might make a bad decision about how many threads to start. Since you've got blocking I/Os in the thread I'd suggest using WT_EXECUTELONGFUNCTION. By default the pool will create up to 500 threads which will typically use 500MB of virtual address space. For a large file this limit might be hit due to the blocking I/Os - in fact, you only need a 32MB file before you've queued 500 work items.

The CreateFile call for the file to be read doesn't specify any sharing flags. I'd suggest setting FILE_SHARE_READ so that at least another process can read the file while you're copying it!

The documentation for WT_EXECUTEINIOTHREAD mentions APCs and the need to handle re-entrancy. I'm not sure what it's talking about here - presumably this would only apply if you were using alertable waits.

re: What's wrong with this code, part 9 - File Copy

Larry Osterman — Wed, 09 Feb 2005 21:14:00 GMT

Ahh, but you're missing one critical aspect of the thread pool Mike: How many threads DOES it spin up?

WT_EXECUTELONGFUNCTION forces a new thread to be created for each request, which burns a lot of time - it's not executed by the thread pool anymore.

re: What's wrong with this code, part 9 - File Copy

Mike Dimmick — Wed, 09 Feb 2005 21:45:00 GMT

"Work items are queued to I/O worker threads as asynchronous procedure calls (APC)." - http://msdn.microsoft.com/library/default.asp?url=/library/en-us/dllproc/base/thread_pooling.asp.

Nope, no clearer. However, this article might help: http://www.microsoft.com/msj/0499/pooling/pooling.aspx. So it looks like new threads will be created after a period of time or a number of requests are queued (see Figure 1 in the linked article). We queue a very large number of requests, which will block twice. OK, we won't create 500 threads for a 32MB file. The speed at which we obtain the data will presumably govern how many threads are created. The slower we get, the more threads will probably be created.

re: What's wrong with this code, part 9 - File Copy

Foolhardy — Wed, 09 Feb 2005 22:04:00 GMT

As someone else mentioned, it would be better to do unbuffered IO: it's a waste to fill up the cache with two copies of the same data, espescially with a large file which will page everything else out to make room. Explorer has a nasty tendency to do this while copying large files.

Why even bother with any seperate threads? The main thread is going to wait until the whole operation completes anyways, so why not have it create and wait on a completion port for itself? There'd be no more thread synchronization issues. I think a completion port would be a much better solution for this.

unbufcp1.c from the platform SDK examples has an example of this, although it corrupts files without modification. unbufcp2.c is the same thing, but it uses multiple threads. It's not faster. (at least not on my machine)

Also, according to the comments in unbufcp1.c, writes that cause file extention are always synchronous, so it extends the file before hand.

re: What's wrong with this code, part 9 - File Copy

Dean Harding — Wed, 09 Feb 2005 22:32:00 GMT

My understanding was that you only use WT_EXECUTEINIOTHREAD when you need your thread to wait in an alertable state, and not at other times (because non-IO threads are more "efficient"). Since the code doesn't need to wait in an alertable state, then WT_EXECUTEINIOTHREAD isn't needed.

Also, sequential access to a file is always going to be faster than random access. You're basically taking what is a sequential read and sequential write, and turning them into random reads and writes. And unless you're copying between separate physical disks, then the read/write operations will be serialized by the disk anyway so there's not point multi-threading it.

re: What's wrong with this code, part 9 - File Copy

Larry Osterman — Wed, 09 Feb 2005 23:01:00 GMT

Mike: Exactly - for normal sized files, we never generate enough load to increase the thread count to a value higher than 1. Which defeats the entire purpose of queuing these to a worker thread.

Dean: You're right. That's a bug, although not critical (I suspect that the owner of the thread pool APIs might disagree with me though :))

re: What's wrong with this code, part 9 - File Copy

B.Y. — Wed, 09 Feb 2005 23:35:00 GMT

Since the objective here is to speed up file copy, you should copy sparse files sparsely.

re: What's wrong with this code, part 9 - File Copy

Anon — Thu, 10 Feb 2005 00:19:00 GMT

Larry Osterman, the non-throwing forms of 'new' are the placement forms which accept a nothrow_t.

I understand that you might not be familiar with the engineering disciplines that exceptions require and might have reservations about the code generation with some compilers, and in fact, I'm impressed that you admit your inabilities and prejudices. Nevertheless, I consider it better to say "I've called set_new_handler" or "This is not standard C++" rather than "I don't like exceptions so I ignored them like a college student" (or whatever you came out with earlier).

re: What's wrong with this code, part 9 - File Copy

Phaeron — Thu, 10 Feb 2005 05:11:00 GMT

Hmm... I'd think that even if this code could successfully do overlapped I/O with multiple threads, there'd be a danger of hitting the I/O memory lock limit and getting spurious failures because there is no hard bound on the number of simultaneous threads (and thus pending requests).

re: What's wrong with this code, part 9 - File Copy

Tom M — Thu, 10 Feb 2005 09:33:00 GMT

Practically the entire thread function would have to be wrapped in critical sections, which would seem to defeat the object of shooting off worker threads to do the job for you. My reading of this is that part of the problem is that WriteFile will advance the file pointer by the number of bytes writen, and the next write operation will then continue from there. Because of the gap between setting the write pointer (and read pointer for that matter), and the actual read/write operation, there is a chance that reading and writing will be to and from random sections of the file, thus corrupting the file.

re: What's wrong with this code, part 9 - File Copy

Larry Osterman — Thu, 10 Feb 2005 17:52:00 GMT

Anon, actually that's not quite true. According to MSDN:
If the request is for zero bytes of storage, operator new returns a pointer to a distinct object (that is, repeated calls to operator new return different pointers). If there is insufficient memory for the allocation request, operator new returns NULL or throws an exception (see The new and delete Operators for more information).

Beginning in Visual C++ .NET 2002, the CRT's new function (in libc.lib, libcd.lib, libcmt.lib, libcmtd.lib, msvcrt.lib, and msvcrtd.lib) will continue to return NULL if memory allocation fails. However, the new function in the Standard C++ Library (in libcp.lib, libcpd.lib, libcpmt.lib, libcpmtd.lib, msvcprt.lib, and msvcprtd.lib) will support the behavior specified in the C++ standard, which is to throw a std::bad_alloc exception if the memory allocation fails.

So the operator new that's used by default doesn't throw unless you include #include <new> in your code.

And Anon, I DO understand about the engineering disciplines that exceptions require. And I've very carefully considered them.

And I feel quite strongly that is is significantly harder to generate correct code in the presence of exceptions than it is to generate correct code in the presence of deterministic failure modes. I'm not alone in this respect, there are many other senior Microsoft people (Raymond Chen, Michael Grier for example) who also feel this way.

Internally the "exceptions vs error codes" debate comes up about every two or three months, and at the end, boths sides agree to disagree - the exception people believe that the difficulty of writing correct code isn't as important as the benefits of having the ability to provide additional information about failures, and the error code people believe that it's more important to be able to write verifiably correct code.

I stand with the error code people.

Concurrency, Part 2 - Avoiding the problem

Larry Osterman's WebLog — Tue, 15 Feb 2005 23:48:00 GMT

Concurrency, Part 2 - Avoiding the problem

Larry Osterman's WebLog — Wed, 16 Feb 2005 00:50:00 GMT

Help regarding Copy File Performance | keyongtech

Help regarding Copy File Performance | keyongtech — Thu, 22 Jan 2009 05:56:54 GMT

PingBack from http://www.keyongtech.com/2548309-help-regarding-copy-file-performance

Larry Osterman s WebLog What s wrong with this code part 9 File Copy | Insomnia Cure

Tue, 09 Jun 2009 01:48:07 GMT

PingBack from http://insomniacuresite.info/story.php?id=2329