Closed Bug 701051 Opened 8 years ago Closed 8 years ago

Reduce CPU load in the Download Manager UI during downloads (Port Bug 397424).

Categories

(SeaMonkey :: Download & File Handling, defect)

defect
Not set

Tracking

(seamonkey2.8 fixed)

RESOLVED FIXED
seamonkey2.8
Tracking Status
seamonkey2.8 --- fixed

People

(Reporter: philip.chee, Assigned: philip.chee)

References

Details

Attachments

(3 files, 4 obsolete files)

Relevant bugs:
  FX Bug 397424 - Downloads cause high CPU usage.
  SM Bug 506731 - Download manager uses a lot of CPU when active with multiple items
Attached patch Patch v1.0 (obsolete) — Splinter Review
Jens, can you review the test changes? Blame says you wrote/adapted this test.

Ported the following changes to get test_drag.xul working:
Bug 367393 - Add a packed MochiKit that contains only SimpleTest dependencies- chrome.
Bug 677626 - Add a profiling test suite for mochitest.

$ TEST_PATH=suite/common/downloads/tests make -C ../objdir-sm/ mochitest-browser-chrome

INFO TEST-START | Shutdown
Browser Chrome Test Summary
        Passed: 2
        Failed: 0
        Todo: 0

*** End BrowserChrome Test Results ***

$ TEST_PATH=suite/common/downloads/tests make -C ../objdir-sm/ mochitest-chrome

227 INFO TEST-START | Shutdown
228 INFO Passed: 137
229 INFO Failed: 0
230 INFO Todo:   0
231 INFO SimpleTest FINISHED
232 INFO TEST-INFO | Ran 0 Loops
233 INFO SimpleTest FINISHED
Attachment #573224 - Flags: review?(kairo)
Attachment #573224 - Flags: review?(jh)
Blocks: 506731
Comment on attachment 573224 [details] [diff] [review]
Patch v1.0

(In reply to Philip Chee from comment #1)
> Jens, can you review the test changes? Blame says you wrote/adapted this
> test.

Sure (for something as simple as this).

> $ TEST_PATH=suite/common/downloads/tests make -C ../objdir-sm/
> mochitest-chrome
> 
> 227 INFO TEST-START | Shutdown
> 228 INFO Passed: 137
> 229 INFO Failed: 0
> 230 INFO Todo:   0
> 231 INFO SimpleTest FINISHED
> 232 INFO TEST-INFO | Ran 0 Loops
> 233 INFO SimpleTest FINISHED

Without the patch, this test suite hangs for me on Linux at test_drag.xul. With the patch, the test suite finishes with 136 Passed and 0 Failed/Todo. Can you explain the discrepancy (137 vs. 136)? Maybe you ran the tests on Windows? Or had uncommitted changes locally? Or is the mistake on my side?

Anyway, r=me for the test changes, provided that they'll still all pass in case code review requires you to make changes elsewhere. Thanks!

BTW the code changes look good, too, but I didn't really test (heh) them.
Attachment #573224 - Flags: review?(jh) → review+
Comment on attachment 573224 [details] [diff] [review]
Patch v1.0

>+      // At this point, we know if we are an indeterminate download or not
>+      if (gDownload.progress != -1) {
>+        // if it was undetermined before, unhide text and switch mode
>+        if (gProgressText.hidden) {
>+          gProgressText.hidden = false;
>+          gProgressMeter.mode = "determined";
>+        }
>+      }
>+      else {
>+        gProgressText.hidden = true;
>+        gProgressMeter.mode = "undetermined";
>+      }
This is a little clumsy. Try:
if (gDownload.progress == -1) {
  gProgressText.hidden = true;
  gProgressMeter.mode = "undetermined";
}
else if (gProgressText.hidden) {
  // if it was undetermined before, unhide text and switch mode
  gProgressText.hidden = false;
  gProgressMeter.mode = "determined";
}
> Without the patch, this test suite hangs for me on Linux at test_drag.xul. With the
> patch, the test suite finishes with 136 Passed and 0 Failed/Todo. Can you explain the
> discrepancy (137 vs. 136)? Maybe you ran the tests on Windows?
My notebook runs Windows7. Probably that explains the difference.

> Or had uncommitted changes locally? Or is the mistake on my side?
Nope, everything is managed with Mercurial Queues, push/pop/reorder etc.
While running the DM chrome tests I found that test_multi_select.xul reproducibly hung for me on Windows (Win7 x64, if that matters), even with the patch from here applied. Filed bug 701540 for that.

BTW: On Windows, I only have 134 passing tests (with the patches from bug 701540 and this one applied; 0 failed/TODO). Strange.
Comment on attachment 573224 [details] [diff] [review]
Patch v1.0

I take it that Jens has tested it, so I only did code inspection, and that looks good to me. Please address that nit from Neil though, his variant looks a bit nicer.
Attachment #573224 - Flags: review?(kairo) → review+
Comment on attachment 573224 [details] [diff] [review]
Patch v1.0

Review of attachment 573224 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for putting this into two pieces, but I just now also did the same patch for my download manager add-on and tripped over this. That patch there is at http://hg.mozilla.org/users/kairo_kairo.at/dlman/rev/2a6994627ba5 FYI.

::: suite/common/downloads/progressDialog.js
@@ +139,5 @@
>      case nsIDownloadManager.DOWNLOAD_NOTSTARTED:
>      case nsIDownloadManager.DOWNLOAD_DOWNLOADING:
> +      // At this point, we know if we are an indeterminate download or not
> +      if (gDownload.progress != -1) {
> +        // if it was undetermined before, unhide text and switch mode

Oh, just noticed that, please change that to start with a capital I and end with a dot. It's a full sentence, so should also be formatted as one.

::: suite/common/downloads/treeView.js
@@ +287,5 @@
>      };
>      switch (attrs.state) {
>        case nsIDownloadManager.DOWNLOAD_NOTSTARTED:
>        case nsIDownloadManager.DOWNLOAD_DOWNLOADING:
> +        // At this point, we know if we are an indeterminate download or not

Also end with a dot.

@@ +291,5 @@
> +        // At this point, we know if we are an indeterminate download or not
> +        attrs.progressMode = attrs.progress == -1 ?
> +                                               nsITreeView.PROGRESS_UNDETERMINED :
> +                                               nsITreeView.PROGRESS_NORMAL;
> +        // As well as knowing the referrer

And that is not a sentence and also doesn't really fit the one before. Can we make it proper English? I propose:
// We also might now know the referrer.

@@ +345,5 @@
> +          // At this point, we know if we are an indeterminate download or not
> +          dl.progressMode = dl.progress == -1 ?
> +                                           nsITreeView.PROGRESS_UNDETERMINED :
> +                                           nsITreeView.PROGRESS_NORMAL;
> +          // As well as knowing the referrer

Same with the two comments here.
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #6)
> I take it that Jens has tested it

I did *not* test anything but the test! Verbatim:

"BTW the code changes look good, too, but I didn't really test (heh) them."
I only asked Jens to test the test.
Checked in:
http://hg.mozilla.org/comm-central/rev/af611683d127
Then backed out:
http://hg.mozilla.org/comm-central/rev/030e1a66b572

Problem.

Make sure that the download manager is closed.
Go download something large from http://ftp.mozilla.org/pub/mozilla.org/seamonkey/nightly/latest-comm-central-trunk/

Actual results:
The download manager opens. The Progress column shows a changing number showing the percent downloaded instead of a progress bar.

Expected results:
A progress bar appears.
Comment on attachment 573224 [details] [diff] [review]
Patch v1.0

>+      if (gDownload.progress != -1) {
Warning: reference to undefined property gDownload.progress

Did you mean to use size?

Not sure what's causing the downloadmanager problem yet though.
OK, so the reason it doesn't work for existing downloads is that they are populated in initTree() which doesn't set the progressMode property.

There seems to be a bug in that the code only sets the progress mode for downloading downloads, although in fact we need to set it for active downloads (which is what getProgressMode checks for). You could go one step further and reset it for inactive downloads which would simplify getProgressMode.
Attached patch Patch v1.1 (obsolete) — Splinter Review
> neil@parkwaycc.co.uk      2011-11-17 11:00:56 PST
> 
>>+      if (gDownload.progress != -1) {
> Warning: reference to undefined property gDownload.progress
> 
> Did you mean to use size?

Oops I meant percentComplete

> OK, so the reason it doesn't work for existing downloads is that they are
> populated in initTree() which doesn't set the progressMode property.

Fixed.

> There seems to be a bug in that the code only sets the progress mode for
> downloading downloads, although in fact we need to set it for active downloads
>> (which is what getProgressMode checks for). You could go one step further
>> and reset it for inactive downloads which would simplify getProgressMode.

Fixed.
Attachment #573224 - Attachment is obsolete: true
Attachment #575620 - Flags: review?(neil)
Comment on attachment 575620 [details] [diff] [review]
Patch v1.1

>   getProgressMode: function(aRow, aColumn) {
>     if (aColumn.id == "Progress") {
>       var dl = this._dlList[aRow];
>       if (dl.isActive)
>-        return (dl.maxBytes >= 0) ? nsITreeView.PROGRESS_NORMAL :
>-                                    nsITreeView.PROGRESS_UNDETERMINED;
>+        return dl.progressMode;
>     }
My point is that getProgressMode assumes progressMode is accurate if isActive is true. Having ensured that progressMode is always accurate, you can write:
if (aColumn.id == "Progress")
  return this._dlList[aRow].progressMode;

>     switch (attrs.state) {
>       case nsIDownloadManager.DOWNLOAD_NOTSTARTED:
>       case nsIDownloadManager.DOWNLOAD_DOWNLOADING:
>+        // At this point, we know if we are an indeterminate download or not.
>+        attrs.progressMode = attrs.progress == -1 ?
>+                                               nsITreeView.PROGRESS_UNDETERMINED :
>+                                               nsITreeView.PROGRESS_NORMAL;
>+        // We also know the referrer at this point.
>+        var referrer = aDownload.referrer;
>+        if (referrer)
>+            attrs.referrer = referrer.spec;
>       case nsIDownloadManager.DOWNLOAD_PAUSED:
>       case nsIDownloadManager.DOWNLOAD_QUEUED:
>       case nsIDownloadManager.DOWNLOAD_SCANNING:
>         attrs.isActive = 1;
The problem here is that we don't set the progress mode, yet getProgressMode thinks we did! (Similarly for the other places where we check the state.) So we should ensure we always set an appropriate progress mode here too.
Attached patch Patch v1.3 (obsolete) — Splinter Review
>>   getProgressMode: function(aRow, aColumn) {
>>     if (aColumn.id == "Progress") {
>>       var dl = this._dlList[aRow];
>>       if (dl.isActive)
>>-        return (dl.maxBytes >= 0) ? nsITreeView.PROGRESS_NORMAL :
>>-                                    nsITreeView.PROGRESS_UNDETERMINED;
>>+        return dl.progressMode;
>>     }
> My point is that getProgressMode assumes progressMode is accurate if isActive
> is true. Having ensured that progressMode is always accurate, you can write:
> if (aColumn.id == "Progress")
>   return this._dlList[aRow].progressMode;
Fixed.

>>     switch (attrs.state) {
>>       case nsIDownloadManager.DOWNLOAD_NOTSTARTED:
>>       case nsIDownloadManager.DOWNLOAD_DOWNLOADING:
>>+        // At this point, we know if we are an indeterminate download or not.
>>+        attrs.progressMode = attrs.progress == -1 ?
>>+                                               nsITreeView.PROGRESS_UNDETERMINED :
>>+                                               nsITreeView.PROGRESS_NORMAL;
>>+        // We also know the referrer at this point.
>>+        var referrer = aDownload.referrer;
>>+        if (referrer)
>>+            attrs.referrer = referrer.spec;
>>       case nsIDownloadManager.DOWNLOAD_PAUSED:
>>       case nsIDownloadManager.DOWNLOAD_QUEUED:
>>       case nsIDownloadManager.DOWNLOAD_SCANNING:
>>         attrs.isActive = 1;
> The problem here is that we don't set the progress mode, yet getProgressMode
> thinks we did! (Similarly for the other places where we check the state.) So
> we should ensure we always set an appropriate progress mode here too.
Err. OK.
Attachment #575620 - Attachment is obsolete: true
Attachment #575620 - Flags: review?(neil)
Attachment #576567 - Flags: review?(neil)
Comment on attachment 576567 [details] [diff] [review]
Patch v1.3

>     };
>     switch (attrs.state) {
>+      case nsIDownloadManager.DOWNLOAD_DOWNLOADING:
>+        // At this point, we know if we are an indeterminate download or not.
>+        attrs.progressMode = attrs.progress == -1 ?
>+                                               nsITreeView.PROGRESS_UNDETERMINED :
>+                                               nsITreeView.PROGRESS_NORMAL;
>+        // We also know the referrer at this point.
>+        var referrer = aDownload.referrer;
>+        if (referrer)
>+            attrs.referrer = referrer.spec;
>       case nsIDownloadManager.DOWNLOAD_NOTSTARTED:
>-      case nsIDownloadManager.DOWNLOAD_DOWNLOADING:
>       case nsIDownloadManager.DOWNLOAD_PAUSED:
>       case nsIDownloadManager.DOWNLOAD_QUEUED:
>       case nsIDownloadManager.DOWNLOAD_SCANNING:
>         attrs.isActive = 1;
>+        if (!attrs.progressMode)
>+          attrs.progressMode = nsITreeView.PROGRESS_NONE;
>         break;
>       default:
>         attrs.isActive = 0;
>+        attrs.progressMode = nsITreeView.PROGRESS_NONE;
You can simplify this code by adding a default value for progressMode to the definition of the attrs object above (but still overriding when downloading) although that doesn't really help you in the initTree version.

>       switch (dl.state) {
>+        case nsIDownloadManager.DOWNLOAD_DOWNLOADING:
>+          // At this point, we know if we are an indeterminate download or not.
>+          dl.progressMode = dl.progress == -1 ?
>+                                           nsITreeView.PROGRESS_UNDETERMINED :
>+                                           nsITreeView.PROGRESS_NORMAL;
>+          // We also know the referrer at this point.
>+          var referrer = aDownload.referrer;
>+          if (referrer)
>+            dl.referrer = referrer.spec;
>         case nsIDownloadManager.DOWNLOAD_NOTSTARTED:
>-        case nsIDownloadManager.DOWNLOAD_DOWNLOADING:
>         case nsIDownloadManager.DOWNLOAD_PAUSED:
>         case nsIDownloadManager.DOWNLOAD_QUEUED:
>         case nsIDownloadManager.DOWNLOAD_SCANNING:
>           dl.isActive = 1;
>+          if (!dl.progressMode)
>+            dl.progressMode = nsITreeView.PROGRESS_NONE;
As it happens, in this version the progress mode will already have been set when the download was loaded or added, so you don't actually need to check.
Attachment #576567 - Flags: review?(neil) → review+
[addDownload]
>>       case nsIDownloadManager.DOWNLOAD_SCANNING:
>>         attrs.isActive = 1;
>>+        if (!attrs.progressMode)
>>+          attrs.progressMode = nsITreeView.PROGRESS_NONE;
>>         break;
>>       default:
>>         attrs.isActive = 0;
>>+        attrs.progressMode = nsITreeView.PROGRESS_NONE;
> You can simplify this code by adding a default value for progressMode to the
> definition of the attrs object above (but still overriding when downloading)
> although that doesn't really help you in the initTree version.
Fixed.

[updateDownload]
>>         case nsIDownloadManager.DOWNLOAD_SCANNING:
>>           dl.isActive = 1;
>>+          if (!dl.progressMode)
>>+            dl.progressMode = nsITreeView.PROGRESS_NONE;
> As it happens, in this version the progress mode will already have been set
> when the download was loaded or added, so you don't actually need to check.
Fixed.
Attachment #576567 - Attachment is obsolete: true
Attachment #576702 - Flags: review+
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/9696f60a3def
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.8
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
<Px>	Oops, on latest nightly on windows download progress bar doesn't change to "Finished", just stays full, is it intended behavior, or bug?
<Px>	http://www.cs.iptcom.net/debug/downloadu.png
<RattyAway>	Px: I changed something download related yesterday http://hg.mozilla.org/comm-central/rev/9696f60a3def
<RattyAway>	Px: ok I've got a fix.

Too much optimization due to:

> My point is that getProgressMode assumes progressMode is accurate if isActive
> is true. Having ensured that progressMode is always accurate, you can write:

> if (aColumn.id == "Progress")

>   return this._dlList[aRow].progressMode;
Attachment #576936 - Flags: review?(neil)
Attaching http://www.cs.iptcom.net/debug/downloadu.png to this bug for permanent reference.
Comment on attachment 576567 [details] [diff] [review]
Patch v1.3

>         default:
>           dl.isActive = 0;
>+          dl.progressMode = nsITreeView.PROGRESS_NONE;
>           gDMUI.getAttention();
>           break;
It wasn't obvious but I deliberately didn't quote this code above ;-)
Restoring it should fix your problem.
Attachment #576936 - Flags: review?(neil) → review-
> It wasn't obvious but I deliberately didn't quote this code above ;-)
> Restoring it should fix your problem.
Fixed. I think.
Attachment #576936 - Attachment is obsolete: true
Attachment #576946 - Flags: review?(neil)
Comment on attachment 576946 [details] [diff] [review]
Patch v2.1 Followup fix: Progressbar does not say "Finished"

Surely my r+ from attachment 576936 [details] [diff] [review] covers this? ;-)
Attachment #576946 - Flags: review?(neil) → review+
Pushed:
http://hg.mozilla.org/comm-central/rev/6b15b03415a4
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.