From 42b50d5d59c86161b28b6d0ef051c3ad218f8700 Mon Sep 17 00:00:00 2001 From: Felix Fung Date: Thu, 3 Nov 2011 15:25:55 -0700 Subject: [PATCH] Bug 397424 - Downloads cause high CPU usage. r=gavin This addresses an issue with the download manager that can cause high CPU usage when there is an active download. The underlying issue is the frequency of updates that the download progress listener receives. Things changed: - reduced the number of null checks in DownloadUtils.jsm's getDownloadStatus function by one (down to two from three). - obtain and format strings from the nsIStringBundle. This removes all the calls to String.replace in DownloadUtils.jsm. - modifies the download manager back-end to update the percentComplete and size property on downloads before dispatching a state changed notification for downloads entering the DOWNLOAD_DOWNLOADING state. This saves us two calls to setAttribute on downloads that we know how big they are, and saves us the same two calls to setAttribute for indeterminate downloads as well as not dispatching a ValueChange event on the progressmeter every time onProgressChange is called on the DownloadProgressListener. - has nsDownload implement nsIClassInfo so we do not need to QueryInterface when going through the list of active downloads in both the download manager's UI and the browser's taskbar UI. --- browser/base/content/browser.js | 2 +- .../downloads/nsDownloadManager.cpp | 16 +- .../mozapps/downloads/downloads.properties | 40 ++-- toolkit/mozapps/downloads/DownloadUtils.jsm | 195 ++++++------------ .../content/DownloadProgressListener.js | 40 ++-- .../mozapps/downloads/content/downloads.js | 2 +- 6 files changed, 130 insertions(+), 165 deletions(-) diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js index 286d46faac15..13a8c8103a04 100644 --- a/browser/base/content/browser.js +++ b/browser/base/content/browser.js @@ -8190,7 +8190,7 @@ let DownloadMonitorPanel = { let maxTime = -Infinity; let dls = gDownloadMgr.activeDownloads; while (dls.hasMoreElements()) { - let dl = dls.getNext().QueryInterface(Ci.nsIDownload); + let dl = dls.getNext(); if (dl.state == gDownloadMgr.DOWNLOAD_DOWNLOADING) { // Figure out if this download takes longer if (dl.speed > 0 && dl.size > 0) diff --git a/toolkit/components/downloads/nsDownloadManager.cpp b/toolkit/components/downloads/nsDownloadManager.cpp index 179768ad0f39..b41a2469a1f4 100644 --- a/toolkit/components/downloads/nsDownloadManager.cpp +++ b/toolkit/components/downloads/nsDownloadManager.cpp @@ -46,6 +46,7 @@ #include "mozIStorageService.h" #include "nsIAlertsService.h" +#include "nsIClassInfoImpl.h" #include "nsIDOMWindow.h" #include "nsIDownloadHistory.h" #include "nsIDownloadManagerUI.h" @@ -2130,8 +2131,14 @@ nsDownloadManager::ConfirmCancelDownloads(PRInt32 aCount, //////////////////////////////////////////////////////////////////////////////// //// nsDownload -NS_IMPL_ISUPPORTS4(nsDownload, nsIDownload, nsITransfer, nsIWebProgressListener, - nsIWebProgressListener2) +NS_IMPL_CLASSINFO(nsDownload, NULL, 0, NS_DOWNLOAD_CID); +NS_IMPL_ISUPPORTS4_CI( + nsDownload + , nsIDownload + , nsITransfer + , nsIWebProgressListener + , nsIWebProgressListener2 +) nsDownload::nsDownload() : mDownloadState(nsIDownloadManager::DOWNLOAD_NOTSTARTED), mID(0), @@ -2399,6 +2406,11 @@ nsDownload::OnProgressChange64(nsIWebProgress *aWebProgress, if (resumableChannel) (void)resumableChannel->GetEntityID(mEntityID); + // Before we update the state and dispatch state notifications, we want to + // ensure that we have the correct state for this download with regards to + // its percent completion and size. + SetProgressBytes(0, aMaxTotalProgress); + // Update the state and the database rv = SetState(nsIDownloadManager::DOWNLOAD_DOWNLOADING); NS_ENSURE_SUCCESS(rv, rv); diff --git a/toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties b/toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties index c04dd51c13c3..920fcde7b64f 100644 --- a/toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties +++ b/toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties @@ -42,31 +42,37 @@ dontLeavePrivateBrowsingButton=Stay in Private Browsing Mode downloadsCompleteTitle=Downloads Complete downloadsCompleteMsg=All files have finished downloading. -# LOCALIZATION NOTE (statusFormat2): — is the "em dash" (long dash) -# #1 transfer progress; #2 rate number; #3 rate unit; #4 time left +# LOCALIZATION NOTE (statusFormat3): — is the "em dash" (long dash) +# %1$S transfer progress; %2$S rate number; %3$S rate unit; %4$S time left # example: 4 minutes left — 1.1 of 11.1 GB (2.2 MB/sec) -statusFormat2=#4 — #1 (#2 #3/sec) +statusFormat3=%4$S — %1$S (%2$S %3$S/sec) bytes=bytes kilobyte=KB megabyte=MB gigabyte=GB -# LOCALIZATION NOTE (transferSameUnits, transferDiffUnits, transferNoTotal): -# #1 progress number; #2 progress unit; #3 total number; #4 total unit -# examples: 1.1 of 333 MB; 11.1 MB of 3.3 GB; 111 KB -transferSameUnits=#1 of #3 #4 -transferDiffUnits=#1 #2 of #3 #4 -transferNoTotal=#1 #2 +# LOCALIZATION NOTE (transferSameUnits2): +# %1$S progress number; %2$S total number; %3$S total unit +# example: 1.1 of 333 MB +transferSameUnits2=%1$S of %2$S %3$S +# LOCALIZATION NOTE (transferDiffUnits2): +# %1$S progress number; %2$S progress unit; %3$S total number; %4$S total unit +# example: 11.1 MB of 3.3 GB +transferDiffUnits2=%1$S %2$S of %3$S %4$S +# LOCALIZATION NOTE (transferNoTotal2): +# %1$S progress number; %2$S unit +# example: 111 KB +transferNoTotal2=%1$S %2$S -# LOCALIZATION NOTE (timePair): #1 time number; #2 time unit +# LOCALIZATION NOTE (timePair2): %1$S time number; %2$S time unit # example: 1 minute; 11 hours -timePair=#1 #2 -# LOCALIZATION NOTE (timeLeftSingle): #1 time left +timePair2=%1$S %2$S +# LOCALIZATION NOTE (timeLeftSingle2): %1$S time left # example: 1 minute remaining; 11 hours remaining -timeLeftSingle=#1 remaining -# LOCALIZATION NOTE (timeLeftDouble): #1 time left; #2 time left sub units +timeLeftSingle2=%1$S remaining +# LOCALIZATION NOTE (timeLeftDouble2): %1$S time left; %2$S time left sub units # example: 11 hours, 2 minutes remaining; 1 day, 22 hours remaining -timeLeftDouble=#1, #2 remaining +timeLeftDouble2=%1$S, %2$S remaining timeFewSeconds=A few seconds remaining timeUnknown=Unknown time remaining @@ -79,7 +85,7 @@ doneStatus=#1 — #2 doneSize=#1 #2 doneSizeUnknown=Unknown size # LOCALIZATION NOTE (doneScheme): #1 URI scheme like data: jar: about: -doneScheme=#1 resource +doneScheme2=%1$S resource # LOCALIZATION NOTE (doneFileScheme): Special case of doneScheme for file: # This is used as an eTLD replacement for local files, so make it lower case doneFileScheme=local file @@ -95,7 +101,7 @@ stateDirty=Blocked: Download may contain a virus or spyware # LOCALIZATION NOTE (yesterday): Displayed time for files finished yesterday yesterday=Yesterday # LOCALIZATION NOTE (monthDate): #1 month name; #2 date number; e.g., January 22 -monthDate=#1 #2 +monthDate2=%1$S %2$S fileDoesNotExistOpenTitle=Cannot Open %S fileDoesNotExistShowTitle=Cannot Show %S diff --git a/toolkit/mozapps/downloads/DownloadUtils.jsm b/toolkit/mozapps/downloads/DownloadUtils.jsm index 39d925872bac..81b682f5921d 100644 --- a/toolkit/mozapps/downloads/DownloadUtils.jsm +++ b/toolkit/mozapps/downloads/DownloadUtils.jsm @@ -1,4 +1,5 @@ -/* ***** BEGIN LICENSE BLOCK ***** +/* vim: sw=2 ts=2 sts=2 expandtab filetype=javascript + * ***** BEGIN LICENSE BLOCK ***** * Version: MPL 1.1/GPL 2.0/LGPL 2.1 * * The contents of this file are subject to the Mozilla Public License Version @@ -84,76 +85,32 @@ __defineGetter__("gDecimalSymbol", function() { const kDownloadProperties = "chrome://mozapps/locale/downloads/downloads.properties"; -// These strings will be converted to the corresponding ones from the string -// bundle on use -let kStrings = { - statusFormat: "statusFormat2", - transferSameUnits: "transferSameUnits", - transferDiffUnits: "transferDiffUnits", - transferNoTotal: "transferNoTotal", - timePair: "timePair", - timeLeftSingle: "timeLeftSingle", - timeLeftDouble: "timeLeftDouble", +let gStr = { + statusFormat: "statusFormat3", + transferSameUnits: "transferSameUnits2", + transferDiffUnits: "transferDiffUnits2", + transferNoTotal: "transferNoTotal2", + timePair: "timePair2", + timeLeftSingle: "timeLeftSingle2", + timeLeftDouble: "timeLeftDouble2", timeFewSeconds: "timeFewSeconds", timeUnknown: "timeUnknown", - monthDate: "monthDate", + monthDate: "monthDate2", yesterday: "yesterday", - doneScheme: "doneScheme", + doneScheme: "doneScheme2", doneFileScheme: "doneFileScheme", units: ["bytes", "kilobyte", "megabyte", "gigabyte"], // Update timeSize in convertTimeUnits if changing the length of this array timeUnits: ["seconds", "minutes", "hours", "days"], }; -// This object will lazily load the strings defined in kStrings -let gStr = { - /** - * Initialize lazy string getters - */ - _init: function() - { - // Make each "name" a lazy-loading string that knows how to load itself. We - // need to locally scope name and value to keep them around for the getter. - for (let [name, value] in Iterator(kStrings)) - let ([n, v] = [name, value]) - gStr.__defineGetter__(n, function() gStr._getStr(n, v)); - }, - - /** - * Convert strings to those in the string bundle. This lazily loads the - * string bundle *once* only when used the first time. - */ - get _getStr() - { - // Delete the getter to be overwritten - delete gStr._getStr; - - // Lazily load the bundle into the closure on first call to _getStr - let getStr = Cc["@mozilla.org/intl/stringbundle;1"]. - getService(Ci.nsIStringBundleService). - createBundle(kDownloadProperties). - GetStringFromName; - - // _getStr is a function that sets string "name" to stringbundle's "value" - return gStr._getStr = function(name, value) { - // Delete the getter to be overwritten - delete gStr[name]; - - try { - // "name" is a string or array of the stringbundle-loaded "value" - return gStr[name] = typeof value == "string" ? - getStr(value) : - value.map(getStr); - } catch (e) { - log(["Couldn't get string '", name, "' from property '", value, "'"]); - // Don't return anything (undefined), and because we deleted ourselves, - // future accesses will also be undefined - } - }; - }, -}; -// Initialize the lazy string getters! -gStr._init(); +// This lazily initializes the string bundle upon first use. +__defineGetter__("gBundle", function() { + delete gBundle; + return this.gBundle = Cc["@mozilla.org/intl/stringbundle;1"]. + getService(Ci.nsIStringBundleService). + createBundle(kDownloadProperties); +}); // Keep track of at most this many second/lastSec pairs so that multiple calls // to getTimeLeft produce the same time left @@ -189,28 +146,14 @@ let DownloadUtils = { let seconds = (aSpeed > 0) && (aMaxBytes > 0) ? (aMaxBytes - aCurrBytes) / aSpeed : -1; - // Update the bytes transferred and bytes total - let status; - let (transfer = DownloadUtils.getTransferTotal(aCurrBytes, aMaxBytes)) { - // Insert 1 is the download progress - status = replaceInsert(gStr.statusFormat, 1, transfer); - } + let transfer = DownloadUtils.getTransferTotal(aCurrBytes, aMaxBytes); + let [rate, unit] = DownloadUtils.convertByteUnits(aSpeed); + let [timeLeft, newLast] = DownloadUtils.getTimeLeft(seconds, aLastSec); - // Update the download rate - let ([rate, unit] = DownloadUtils.convertByteUnits(aSpeed)) { - // Insert 2 is the download rate - status = replaceInsert(status, 2, rate); - // Insert 3 is the |unit|/sec - status = replaceInsert(status, 3, unit); - } - - // Update time remaining - let ([timeLeft, newLast] = DownloadUtils.getTimeLeft(seconds, aLastSec)) { - // Insert 4 is the time remaining - status = replaceInsert(status, 4, timeLeft); - - return [status, newLast]; - } + let params = [transfer, rate, unit, timeLeft]; + let status = gBundle.formatStringFromName(gStr.statusFormat, params, + params.length); + return [status, newLast]; }, /** @@ -233,20 +176,31 @@ let DownloadUtils = { let [total, totalUnits] = DownloadUtils.convertByteUnits(aMaxBytes); // Figure out which byte progress string to display - let transfer; - if (aMaxBytes < 0) - transfer = gStr.transferNoTotal; - else if (progressUnits == totalUnits) - transfer = gStr.transferSameUnits; - else - transfer = gStr.transferDiffUnits; + let name, values; + if (aMaxBytes < 0) { + name = gStr.transferNoTotal; + values = [ + progress, + progressUnits, + ]; + } else if (progressUnits == totalUnits) { + name = gStr.transferSameUnits; + values = [ + progress, + total, + totalUnits, + ]; + } else { + name = gStr.transferDiffUnits; + values = [ + progress, + progressUnits, + total, + totalUnits, + ]; + } - transfer = replaceInsert(transfer, 1, progress); - transfer = replaceInsert(transfer, 2, progressUnits); - transfer = replaceInsert(transfer, 3, total); - transfer = replaceInsert(transfer, 4, totalUnits); - - return transfer; + return gBundle.formatStringFromName(name, values, values.length); }, /** @@ -267,7 +221,7 @@ let DownloadUtils = { aLastSec = Infinity; if (aSeconds < 0) - return [gStr.timeUnknown, aLastSec]; + return [gBundle.GetStringFromName(gStr.timeUnknown), aLastSec]; // Try to find a cached lastSec for the given second aLastSec = gCachedLast.reduce(function(aResult, aItem) @@ -300,25 +254,26 @@ let DownloadUtils = { let timeLeft; if (aSeconds < 4) { // Be friendly in the last few seconds - timeLeft = gStr.timeFewSeconds; + timeLeft = gBundle.GetStringFromName(gStr.timeFewSeconds); } else { // Convert the seconds into its two largest units to display let [time1, unit1, time2, unit2] = DownloadUtils.convertTimeUnits(aSeconds); - let pair1 = replaceInsert(gStr.timePair, 1, time1); - pair1 = replaceInsert(pair1, 2, unit1); - let pair2 = replaceInsert(gStr.timePair, 1, time2); - pair2 = replaceInsert(pair2, 2, unit2); + let pair1 = + gBundle.formatStringFromName(gStr.timePair, [time1, unit1], 2); + let pair2 = + gBundle.formatStringFromName(gStr.timePair, [time2, unit2], 2); // Only show minutes for under 1 hour unless there's a few minutes left; // or the second pair is 0. if ((aSeconds < 3600 && time1 >= 4) || time2 == 0) { - timeLeft = replaceInsert(gStr.timeLeftSingle, 1, pair1); + timeLeft = gBundle.formatStringFromName(gStr.timeLeftSingle, + [pair1], 1); } else { // We've got 2 pairs of times to display - timeLeft = replaceInsert(gStr.timeLeftDouble, 1, pair1); - timeLeft = replaceInsert(timeLeft, 2, pair2); + timeLeft = gBundle.formatStringFromName(gStr.timeLeftDouble, + [pair1, pair2], 2); } } @@ -364,7 +319,7 @@ let DownloadUtils = { 0); } else if (today - aDate < (24 * 60 * 60 * 1000)) { // After yesterday started, show yesterday - dateTimeCompact = gStr.yesterday; + dateTimeCompact = gBundle.GetStringFromName(gStr.yesterday); } else if (today - aDate < (6 * 24 * 60 * 60 * 1000)) { // After last week started, show day of week dateTimeCompact = aDate.toLocaleFormat("%A"); @@ -373,8 +328,7 @@ let DownloadUtils = { let month = aDate.toLocaleFormat("%B"); // Remove leading 0 by converting the date string to a number let date = Number(aDate.toLocaleFormat("%d")); - dateTimeCompact = replaceInsert(gStr.monthDate, 1, month); - dateTimeCompact = replaceInsert(dateTimeCompact, 2, date); + dateTimeCompact = gBundle.formatStringFromName(gStr.monthDate, [month, date], 2); } let dateTimeFull = dts.FormatDateTime("", @@ -437,11 +391,12 @@ let DownloadUtils = { // Check if we need to show something else for the host if (uri.scheme == "file") { // Display special text for file protocol - displayHost = gStr.doneFileScheme; + displayHost = gBundle.GetStringFromName(gStr.doneFileScheme); fullHost = displayHost; } else if (displayHost.length == 0) { // Got nothing; show the scheme (data: about: moz-icon:) - displayHost = replaceInsert(gStr.doneScheme, 1, uri.scheme); + displayHost = + gBundle.formatStringFromName(gStr.doneScheme, [uri.scheme], 1); fullHost = displayHost; } else if (uri.port != -1) { // Tack on the port if it's not the default port @@ -479,7 +434,7 @@ let DownloadUtils = { if (gDecimalSymbol != ".") aBytes = aBytes.replace(".", gDecimalSymbol); - return [aBytes, gStr.units[unitIndex]]; + return [aBytes, gBundle.GetStringFromName(gStr.units[unitIndex])]; }, /** @@ -552,23 +507,7 @@ function convertTimeUnitsUnits(aTime, aIndex) if (aIndex < 0) return ""; - return PluralForm.get(aTime, gStr.timeUnits[aIndex]); -} - -/** - * Private helper function to replace a placeholder string with a real string - * - * @param aText - * Source text containing placeholder (e.g., #1) - * @param aIndex - * Index number of placeholder to replace - * @param aValue - * New string to put in place of placeholder - * @return The string with placeholder replaced with the new string - */ -function replaceInsert(aText, aIndex, aValue) -{ - return aText.replace("#" + aIndex, aValue); + return PluralForm.get(aTime, gBundle.GetStringFromName(gStr.timeUnits[aIndex])); } /** diff --git a/toolkit/mozapps/downloads/content/DownloadProgressListener.js b/toolkit/mozapps/downloads/content/DownloadProgressListener.js index d1ae908f2949..b345b36ac387 100644 --- a/toolkit/mozapps/downloads/content/DownloadProgressListener.js +++ b/toolkit/mozapps/downloads/content/DownloadProgressListener.js @@ -63,6 +63,7 @@ DownloadProgressListener.prototype = { // Update window title in-case we don't get all progress notifications onUpdateProgress(); + let dl; let state = aDownload.state; switch (state) { case nsIDM.DOWNLOAD_QUEUED: @@ -82,16 +83,26 @@ DownloadProgressListener.prototype = { if (state == nsIDM.DOWNLOAD_FINISHED) autoRemoveAndClose(aDownload); break; + case nsIDM.DOWNLOAD_DOWNLOADING: { + dl = getDownload(aDownload.id); + + // At this point, we know if we are an indeterminate download or not + dl.setAttribute("progressmode", aDownload.percentComplete == -1 ? + "undetermined" : "normal"); + + // As well as knowing the referrer + let referrer = aDownload.referrer; + if (referrer) + dl.setAttribute("referrer", referrer.spec); + + break; + } } // autoRemoveAndClose could have already closed our window... try { - let dl = getDownload(aDownload.id); - - // We should eventually know the referrer at some point - let referrer = aDownload.referrer; - if (referrer) - dl.setAttribute("referrer", referrer.spec); + if (!dl) + dl = getDownload(aDownload.id); // Update to the new state dl.setAttribute("state", state); @@ -112,18 +123,15 @@ DownloadProgressListener.prototype = { var download = getDownload(aDownload.id); // Update this download's progressmeter - if (aDownload.percentComplete == -1) { - download.setAttribute("progressmode", "undetermined"); - } else { - download.setAttribute("progressmode", "normal"); + if (aDownload.percentComplete != -1) { download.setAttribute("progress", aDownload.percentComplete); - } - // Dispatch ValueChange for a11y - var event = document.createEvent("Events"); - event.initEvent("ValueChange", true, true); - document.getAnonymousElementByAttribute(download, "anonid", "progressmeter") - .dispatchEvent(event); + // Dispatch ValueChange for a11y + let event = document.createEvent("Events"); + event.initEvent("ValueChange", true, true); + document.getAnonymousElementByAttribute(download, "anonid", "progressmeter") + .dispatchEvent(event); + } // Update the progress so the status can be correctly updated download.setAttribute("currBytes", aDownload.amountTransferred); diff --git a/toolkit/mozapps/downloads/content/downloads.js b/toolkit/mozapps/downloads/content/downloads.js index 481594d486cf..7bd7ec8109c8 100644 --- a/toolkit/mozapps/downloads/content/downloads.js +++ b/toolkit/mozapps/downloads/content/downloads.js @@ -411,7 +411,7 @@ function onUpdateProgress() var base = 0; var dls = gDownloadManager.activeDownloads; while (dls.hasMoreElements()) { - let dl = dls.getNext().QueryInterface(Ci.nsIDownload); + let dl = dls.getNext(); if (dl.percentComplete < 100 && dl.size > 0) { mean += dl.amountTransferred; base += dl.size;