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

RESOLVED FIXED in mozilla0.9.8

Status

()

RESOLVED FIXED
17 years ago
17 years ago

People

(Reporter: sfraser_bugs, Assigned: sfraser_bugs)

Tracking

({perf})

Trunk
mozilla0.9.8
PowerPC
Mac System 8.5
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

17 years ago
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.
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
Keywords: perf
shouldn't we tell the finder to do this for big files? i'm certain they've
optimized their code for doing all this.
(Assignee)

Comment 2

17 years ago
AppleScripting the Finder would be ugly. I have a patch.
(Assignee)

Comment 3

17 years ago
Created attachment 52100 [details] [diff] [review]
First cut patch
(Assignee)

Updated

17 years ago
Attachment #52100 - Attachment is obsolete: true
(Assignee)

Comment 4

17 years ago
Created attachment 52119 [details] [diff] [review]
Patch that works
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.
(Assignee)

Comment 6

17 years ago
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

17 years ago
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

17 years ago
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.
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.

Updated

17 years ago
Blocks: 71668
(Assignee)

Comment 10

17 years ago
0.9.7
Target Milestone: --- → mozilla0.9.7
(Assignee)

Comment 11

17 years ago
->0.9.8
Target Milestone: mozilla0.9.7 → mozilla0.9.8
(Assignee)

Comment 12

17 years ago
Created attachment 64909 [details] [diff] [review]
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.
Attachment #52119 - Attachment is obsolete: true

Comment 13

17 years ago
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)
Attachment #64909 - Flags: review+
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
Attachment #64909 - Flags: superreview+

Comment 15

17 years ago
oops, r= in haste, need size of buffer passed to copy func
(Assignee)

Comment 16

17 years ago
Created attachment 64928 [details] [diff] [review]
Fixed patch, pass in buffer size.
Attachment #64909 - Attachment is obsolete: true
(Assignee)

Comment 17

17 years ago
Created attachment 64930 [details] [diff] [review]
3rd time luck. If allocation fails, be sure to pass 0 as the buffer size.
Attachment #64928 - Attachment is obsolete: true
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

17 years ago
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 :-)
Attachment #64930 - Flags: review+
(Assignee)

Comment 20

17 years ago
It's in! Thanks guys.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.