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

Status: RESOLVED FIXED

(SeaMonkey :: Download & File Handling, defect)

Description

[Philip Chee]

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

Comment 1

[Philip Chee]

Attachment: Patch v1.0

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

Comment 2

[Jens Hatlak (:InvisibleSmiley)]

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.

Comment 3

[[e-mail]]

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";
}

Comment 4

[Philip Chee]

> 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.

Comment 5

[Jens Hatlak (:InvisibleSmiley)]

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 6

[Robert Kaiser]

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.

Comment 7

[Robert Kaiser]

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.

Comment 8

[Jens Hatlak (:InvisibleSmiley)]

(In reply to Robert Kaiser (:[e-mail]) 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."

Comment 9

[Philip Chee]

I only asked Jens to test the test.

Comment 10

[Philip Chee]

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 11

[[e-mail]]

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.

Comment 12

[[e-mail]]

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.

Comment 13

[Philip Chee]

Attachment: Patch v1.1

> [e-mail]      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.

Comment 14

[[e-mail]]

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.

Comment 15

[Philip Chee]

Attachment: Patch v1.3

>>   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.

Comment 16

[[e-mail]]

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.

Comment 17

[Philip Chee]

Attachment: Patch v1.4 (as checked in) r=InvisibleSmiley r=KaiRo r=Neil

[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.

Comment 18

[Philip Chee]

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/9696f60a3def

Comment 19

[Philip Chee]

Attachment: Patch v2.0 Followup fix: Progressbar does not say "Finished".

<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;

Comment 20

[Philip Chee]

Attachment: Screenshot of progressbar stuck at full.

Attaching http://www.cs.iptcom.net/debug/downloadu.png to this bug for permanent reference.

Comment 21

[[e-mail]]

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.

Comment 22

[Philip Chee]

Attachment: Patch v2.1 Followup fix: Progressbar does not say "Finished"

> It wasn't obvious but I deliberately didn't quote this code above ;-)
> Restoring it should fix your problem.
Fixed. I think.

Comment 23

[[e-mail]]

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? ;-)

Comment 24

[Philip Chee]

Pushed:
http://hg.mozilla.org/comm-central/rev/6b15b03415a4