103202 - nsLocalFileMac::MoveCopy() is very slow to move files

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Simon Fraser [no longer active]]

nsLocalFileMac::MoveCopy() turns around and calls the MoreFiles routine
FSpFileCopy() to copy files. Morefiles uses a 16K buffer if you don't give it
one. This makes copying large files (several megabytes) very slow, for example
at the end of a big download.

This would be easily fixed by passing in a bigger buffer to FSpFileCopy.

Comment 1

[Mike Pinkerton (not reading bugmail)]

shouldn't we tell the finder to do this for big files? i'm certain they've
optimized their code for doing all this.

Comment 2

[Simon Fraser [no longer active]]

AppleScripting the Finder would be ugly. I have a patch.

Comment 3

[Simon Fraser [no longer active]]

Attachment: First cut patch

Comment 4

[Simon Fraser [no longer active]]

Attachment: Patch that works

Comment 5

[Conrad Carlen (not reading bugmail)]

A 16K buffer!?!? No wonder. 2 megs seems excessive though. Did you run some
tests and find out the optimum buffer size? A few years ago, I did some tests
with I/O buffer sizes and found that the perf/buffer size curve starts
flattening out after about 256K. Even so, the gain to justify 2 megs instead of
256K would have to be huge.

Comment 6

[Simon Fraser [no longer active]]

I did no analysis. Maybe someone could come up with some JS to call this so that 
I don't have to keep downloading files  :)

Comment 7

[Jo Hermans]

Actually, this could be fixed much easier : go to
http://lxr.mozilla.org/seamonkey/source/lib/mac/MoreFiles/FileCopy.c#51 , and
change bigCopyBuffSize to 0x00040000, 256K (it's now 0x00004000, 16K)

I remember this piece of sample code from Apple back from the 80-ies. The 16K
was the magical number both for floppy drives and for (serial) LocalTalk. It's
time to bump this up to a more normal value, like 64K or 256K. 2 megs don't make
any sense at all, you'll block too long waiting for all the buffers in the OS.
My own apps (Unix-based though) use 64K btw, but that's because they works
across congested networks. 'Zilla should normally work on local hard disks or at
worst on local networks. and the OS should deal with this in the background anyway.

I can't build on my Mac, can someone test this ? And I think it's a no brainer
to include it in the 0.9.5 release. You might have trouble with a 2 MB buffer if
you're out of memory, but a 256K buffer should be fine. Anyway, when you can't
allocate, you'll use the default 16K or even the minimal 0.5K, so this should be
perfectly fine.

Comment 8

[Jo Hermans]

BTW, I've noticed that the Mac was pretty slow after downloading a large
document, with lot of (unoptimised) disk activity. I think this could be the fix.

Comment 9

[Conrad Carlen (not reading bugmail)]

I made a little console test program which called FSpFileCopy with various
buffer sizes and printed the results. The "Buffer size" field is the number
which was passed to FSpFileCopy, "0" was really 16 KB.

Buffer size = 0, KB/sec = 1330
Buffer size = 32768, KB/sec = 1734
Buffer size = 65536, KB/sec = 3192
Buffer size = 131072, KB/sec = 4198
Buffer size = 262144, KB/sec = 6474
Buffer size = 524288, KB/sec = 7444
Buffer size = 1048576, KB/sec = 8245
Buffer size = 2097152, KB/sec = 8840

You can see that, after 1 MB for the buffer, there's very little gain. I'd still
say 256 KB is fine.

Comment 10

[Simon Fraser [no longer active]]

0.9.7

Comment 11

[Simon Fraser [no longer active]]

->0.9.8

Comment 12

[Simon Fraser [no longer active]]

Attachment: Allocated a temp 512K handle for the copy buffer, and also use it for directory copies

This patch tries to allocate a 512k handle in temporary memory to act as the
copy buffer. This same buffer is used in the directory copy case as well now.
Reviews please.

Comment 13

[Steve Dagley]

Comment on attachment 64909 [details] [diff] [review]
Allocated a temp 512K handle for the copy buffer,  and also use it for directory copies

r=sdagley (mmmm, faster)

Comment 14

[(not reading, please use [e-mail] instead)]

Comment on attachment 64909 [details] [diff] [review]
Allocated a temp 512K handle for the copy buffer,  and also use it for directory copies

sr=sspitzer

Comment 15

[Steve Dagley]

oops, r= in haste, need size of buffer passed to copy func

Comment 16

[Simon Fraser [no longer active]]

Attachment: Fixed patch, pass in buffer size.

Comment 17

[Simon Fraser [no longer active]]

Attachment: 3rd time luck. If allocation fails, be sure to pass 0 as the buffer size.

Comment 18

[(not reading, please use [e-mail] instead)]

Comment on attachment 64930 [details] [diff] [review]
3rd time luck. If allocation fails, be sure to pass 0 as the buffer size.

sr=sspitzer

Comment 19

[Steve Dagley]

Comment on attachment 64930 [details] [diff] [review]
3rd time luck. If allocation fails, be sure to pass 0 as the buffer size.

r=sdagley (yep, 3rd time is the charm :-)

Comment 20

[Simon Fraser [no longer active]]

It's in! Thanks guys.