Closed Bug 410131 Opened 12 years ago Closed 12 years ago

DM isn't buffering writes when saving a file

Categories

(Core Graveyard :: File Handling, defect, P3)

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.9beta3

People

(Reporter: Dolske, Assigned: sdwilsh)

Details

Attachments

(1 file, 4 obsolete files)

DM is causing excessive writes when saving a file.

I downloaded a Tinderbox build/test log (5.5MB, via right click "View Full Log", Save As), and ran DTrace's rwsnoop while the download was going. [Truss output looks about the same.] A representative sample of output (filtered):

  UID    PID CMD          D   BYTES FILE
...
  101  14443 firefox-bin  W    1296 /export/home/dolske/Desktop/showlog.cgi
  101  14443 firefox-bin  W     628 /export/home/dolske/Desktop/showlog.cgi
  101  14443 firefox-bin  W      85 /export/home/dolske/Desktop/showlog.cgi
  101  14443 firefox-bin  W    1382 /export/home/dolske/Desktop/showlog.cgi
  101  14443 firefox-bin  W     599 /export/home/dolske/Desktop/showlog.cgi
  101  14443 firefox-bin  W     381 /export/home/dolske/Desktop/showlog.cgi
  101  14443 firefox-bin  W     452 /export/home/dolske/Desktop/showlog.cgi
  101  14443 firefox-bin  W   16384 /export/home/dolske/.mozilla/firefox
                                          /vr86v7k5.default/Cache/1CC4A7C8d01
...

The downloaded file is being saved to the disk cache and to my desktop...

The cache is being updated in 16K chunks, 344 total writes. The download itself is trickling down piecemeal. I see writes of many, many different sizes; from 8K down to 14 (!), the majority seem to be less than 1K. There are 6525 total writes (avg = 849 bytes per write).

The flip side of this is that we're presumably reading from the socket too often. SO_RCVLOWAT is one approach to helping that, but I don't remember if that works with non-blocking IO. Seems like if we know we're doing a bulk data transfer (*handwaving about transfer size, bandwidth, and responsiveness goes here*), we'd want to reduce overhead by only reading when big gulps are available. Probably fodder for another bug?
Flags: blocking-firefox3?
Attached patch v1.0 (obsolete) — Splinter Review
This patches uriloader/exthandler, where most downloads go through.  This doesn't hit the Save As... case though - I'm still looking at how nsWebBrowserPersists handles this stuff.

Dolske - do you think you can test this (I don't have dtrace).  I'll keep looking into the other spot of code we have to fix.
Assignee: nobody → comrade693+bmo
Status: NEW → ASSIGNED
Comment on attachment 294963 [details] [diff] [review]
v1.0

>+// Buffer file writes in 1MB chunks
>+#define BUFFERED_OUTPUT_SIZE 1048576

seems pretty big - you won't hit the disk until you've amassed that much data. reducing 6000 writes down to a few hundred seems sufficient here, so maybe a size somewhere in the range 16 - 64k would be suitable?
Attached patch v1.1 (obsolete) — Splinter Review
switch to 32kb chunks, and get the webbrowserpersists changes
Attachment #294963 - Attachment is obsolete: true
Comment on attachment 294968 [details] [diff] [review]
v1.1

>Index: embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp
>===================================================================
>+    nsCOMPtr<nsIBufferedOutputStream> bos =
>+      do_CreateInstance(NS_BUFFEREDOUTPUTSTREAM_CONTRACTID);
>+    if (bos) {
>+      rv = bos->Init(fileOutputStream, BUFFERED_OUTPUT_SIZE);
>+      if (NS_SUCCEEDED(rv))
>+        NS_ADDREF(*aOutputStream = bos);
>+      else
>+        NS_ADDREF(*aOutputStream = fileOutputStream);
>+    } else {
>+      NS_ADDREF(*aOutputStream = fileOutputStream);
>+    }

nit: if you use to do_CreateInstance(, &rv) form, you can collapse the last two NS_ADDREFs into one codepath:

   nsCOMPtr<nsIBufferedOutputStream> bos =
     do_CreateInstance(NS_BUFFEREDOUTPUTSTREAM_CONTRACTID, &rv);
   if (NS_SUCCEEDED(rv)) {
     rv = bos->Init(fileOutputStream, BUFFERED_OUTPUT_SIZE);
     if (NS_SUCCEEDED(rv))
       NS_ADDREF(*aOutputStream = bos);
   }
   if (NS_FAILED(rv))
     NS_ADDREF(*aOutputStream = fileOutputStream);
Are any changed needed for pausing/resuming a download (since now bytes written != bytes on disk). Flush() on pause, maybe? Or does the stream just end up getting closed either way?
We cancel the download on pause, and restart a connection, so I'd think that closing the nsIOutputStream (which is actually an nsIBufferedOutputStream) would do it, but I don't know a whole lot about these things (and docs seem to be lacking).  I based my patches off of how you did it with the password manager ;)
yep, if you close/destroy the stream it'll flush.
And how about if Firefox crashes while downloading -- do we attempt to resume the download? I see a curr_bytes in downloads.sqlite, could that end up being greater than the actual size of the partial crashed-while-downloading file?
When we resume, we look at the disk file size -- not the one in the database. 

Will there be any issues with the display if it only receives updates after some number of bytes?
(In reply to comment #9)
> Will there be any issues with the display if it only receives updates after
> some number of bytes?
I don't understand what you are asking exactly...
If the UI only receives updates on 32kb chunks, if it's downloading at slower than that, how often will the UI be updating? Will we be able to show 1KBps or is the minimum 4KBps?
I'm pretty sure that those notifications are done as the bits are received from the network - not as it is being written to the hard drive.
Whiteboard: [has patch][needs verification]
Target Milestone: --- → Firefox 3 M11
Comment on attachment 294968 [details] [diff] [review]
v1.1

+  nsCOMPtr<nsIBufferedOutputStream> bos =
+    do_CreateInstance(NS_BUFFEREDOUTPUTSTREAM_CONTRACTID);

just use NS_NewBufferedOutputStream

and doing the error handling consistently between wbp and exthandler would be nice too
Attachment #294968 - Flags: review+
Attachment #294968 - Flags: superreview?(bzbarsky)
Comment on attachment 294968 [details] [diff] [review]
v1.1

Actually, let me address comments before I request sr...
Attachment #294968 - Attachment is obsolete: true
Attachment #294968 - Flags: superreview?(bzbarsky)
Patch WFM, for the original steps...

  UID    PID CMD          D   BYTES FILE
...
  101  21740 firefox-bin  W   32768 /export/home/dolske/Desktop/showlog.cgi
  101  21740 firefox-bin  W   32768 /export/home/dolske/Desktop/showlog.cgi
  101  21740 firefox-bin  W   32768 /export/home/dolske/Desktop/showlog.cgi
  101  21740 firefox-bin  W   32768 /export/home/dolske/Desktop/showlog.cgi
...

A total of 220 32K writes and one final 10K write for the 7.2MB file.

So, the right-click "Save As" case is fixed, but it seems the normal click-to-download case isn't... If I go to mozilla.com and click to download a Firefox release, I see writes of varying sizes. [Mostly 3204 and 4096 bytes, but a few of different sizes.]

Oddly, I don't get the download mananger window when this happens, so something else might be broken?
Attached patch v2.0 (obsolete) — Splinter Review
This gets the other case that I missed last time, and addresses review comments.
Attachment #295150 - Flags: superreview?(bzbarsky)
Attachment #295150 - Flags: review?(cbiesinger)
(In reply to comment #15)
> Oddly, I don't get the download mananger window when this happens, so something
> else might be broken?
I'm oddly seeing that now too.  I'm also getting a dialog that I don't normally get, so maybe some recent checkin has changed something?
Whiteboard: [has patch][needs verification] → [has patch][needs r biesi][needs sr bz]
Attachment #295150 - Flags: review?(cbiesinger) → review+
Component: Download Manager → File Handling
Flags: blocking-firefox3?
Product: Firefox → Core
QA Contact: download.manager → file-handling
Target Milestone: Firefox 3 M11 → mozilla1.9 M11
Flags: blocking1.9?
Whiteboard: [has patch][needs r biesi][needs sr bz] → [has patch][has r][needs sr bz]
+'ing this with P3 priority.  Please adjust priority if necessary.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
Attached patch v3.0 (obsolete) — Splinter Review
Addresses sr comments on irc.  I changed enough that I feel it should get review again (sorry biesi).
Attachment #295150 - Attachment is obsolete: true
Attachment #295339 - Flags: superreview?(bzbarsky)
Attachment #295339 - Flags: review?(cbiesinger)
Attachment #295150 - Flags: superreview?(bzbarsky)
Whiteboard: [has patch][has r][needs sr bz] → [has patch][needs r biesi][needs sr bz]
Comment on attachment 295339 [details] [diff] [review]
v3.0

>Index: uriloader/exthandler/nsExternalHelperAppService.cpp
>+// Buffer file writes in 32kb chunks
>+#define BUFFERED_OUTPUT_SIZE 32768

(1 << 15) might be clearer...  Or (1024 * 32) or something.

>Index: netwerk/base/public/nsNetUtil.h
>+ * passed in output stream.

passed-in

>+ * @param result
>+ *        The resulting output stream.

How come this isn't the return value?  What's the point of an nsresult return value if you always return NS_OK?  Just make this return already_AddRefed<nsIOutputStream> and be done with it?

>+    if (NS_SUCCEEDED(rv))
>+        NS_ADDREF(*result = bos);

Then this could become:

  return bos.forget();

>+    else
>+        NS_ADDREF(*result = aOutputStream);

While this (after eliminating the |else|) becomes:

  NS_ADDREF(aOutputStream);
  return aOutputStream;
Attachment #295339 - Flags: superreview?(bzbarsky) → superreview-
Whiteboard: [has patch][needs r biesi][needs sr bz] → [needs new patch]
Attached patch v3.1Splinter Review
(In reply to comment #20)
> (1 << 15) might be clearer...  Or (1024 * 32) or something.
done

> >Index: netwerk/base/public/nsNetUtil.h
> >+ * passed in output stream.
> 
> passed-in
done

> How come this isn't the return value?  What's the point of an nsresult return
> value if you always return NS_OK?  Just make this return
> already_AddRefed<nsIOutputStream> and be done with it?
XPCOM familiarism.  Not an excuse though, I know.  Fixed!

> >+    if (NS_SUCCEEDED(rv))
> >+        NS_ADDREF(*result = bos);
> 
> Then this could become:
> 
>   return bos.forget();
I love getting reviews from you!  I learn something new every time!  Fixed.
Attachment #295339 - Attachment is obsolete: true
Attachment #295378 - Flags: superreview?(bzbarsky)
Attachment #295378 - Flags: review?(cbiesinger)
Attachment #295339 - Flags: review?(cbiesinger)
Whiteboard: [needs new patch] → [has patch][needs r biesi][needs sr bz]
Attachment #295378 - Flags: review?(cbiesinger) → review+
Whiteboard: [has patch][needs r biesi][needs sr bz] → [has patch][has r][needs sr bz]
Comment on attachment 295378 [details] [diff] [review]
v3.1

Beautiful.  Thank you!
Attachment #295378 - Flags: superreview?(bzbarsky) → superreview+
Checking in uriloader/exthandler/nsExternalHelperAppService.cpp;
new revision: 1.359; previous revision: 1.358
Checking in embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp;
new revision: 1.132; previous revision: 1.131
Checking in netwerk/base/public/nsNetUtil.h;
new revision: 1.110; previous revision: 1.109
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Flags: in-litmus?
Resolution: --- → FIXED
Whiteboard: [has patch][has r][needs sr bz]
Verified. dtruss on OSX shows it making 32K writes, for both test cases.
Status: RESOLVED → VERIFIED
Flags: in-litmus? → in-litmus-
OS: OpenSolaris → All
Hardware: Sun → All
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.