STR: 1) With a recent Nightly, set browser.library.useNewDownloadsView to new 2) Open the Library 3) Choose "Downloads" in the left tree. What happens? Browser becomes unresponsive as we do some pretty slow operations on the UI thread.

This may have significant impact, actually see http://ejohn.org/blog/dom-documentfragments/ Mike, could you please try this on your build and tell me if you see any notable improvement? Note you also need the follow up checkin I did in the original bug.

Even with this patch, I'm still getting pretty long wait times. Here's what I'm experiencing: http://www.youtube.com/watch?v=MqyhNssc5UM I demonstrate the slowness twice. Note that I move the mouse away from the "Downloads" item immediately after clicking.

Is comment 2 build already including the follow-up patch to avoid reloading the view multiple times? I still suspect a high number of downloads may be heavy to manage cause we .exists() and query 2 annos for each download. Btw this likely needs some profiling to figure what's the major culprit. We may also have to set a stricter limit to downloads history, I was already thinking to expire them more frequently than common history, since it's less likely I may want to manage a download from months ago.

It's pretty clear that most of the overhead is caused by sync IO. And that'll be fixed next. However, I would still like to know if the patch attached helps in a way you could notice (let's say from 5 sec to 3.5 sec)

Comment on attachment 693052 [details] [diff] [review] Step 1 - some obvious stuff (use a document fragment for the initial batch, cache nsIURI objects Review of attachment 693052 [details] [diff] [review]: ----------------------------------------------------------------- it's pretty clear this alone won't do much. Though shouldn't hurt. ::: browser/components/places/content/downloadsView.js @@ +602,5 @@ > + * The places node for a history download. Required if there's no data > + * item. > + * @param [optional] aNewest > + * @see onDataItemAdded. Ignored for history downlods. > + * @param [optional] aElementsToAppendFragment maybe just aDocumentFragment, the comment explains better what it does.

(In reply to Marco Bonardo [:mak] from comment #3) > Is comment 2 build already including the follow-up patch to avoid reloading > the view multiple times? > I still suspect a high number of downloads may be heavy to manage cause we > .exists() and query 2 annos for each download. Btw this likely needs some > profiling to figure what's the major culprit. > > We may also have to set a stricter limit to downloads history, I was already > thinking to expire them more frequently than common history, since it's less > likely I may want to manage a download from months ago. Yes, I was using both patches from bug 675902 (transplanted from mozilla-inbound). I'll re-check and build again, just to be sure.

After a rebuild, I'm still experiencing slow behaviour, and that's with both of those patches from bug 675902 transplanted from mozilla-inbound. With this patch, I notice a *slight* improvement. Over 4 trials, average time to switch from Bookmarks to Downloads without patch: 4.256 seconds. Average time with patch: 4.034 seconds.

Comment on attachment 693052 [details] [diff] [review] Step 1 - some obvious stuff (use a document fragment for the initial batch, cache nsIURI objects Review of attachment 693052 [details] [diff] [review]: ----------------------------------------------------------------- Just some nits - nothing major. Good work. ::: browser/components/places/content/downloadsView.js @@ +118,5 @@ > throw new Error("Unexpected download element state"); > }, > > + get _downloadURIObj() { > + if (!("__downloadURIObj" in this)) Instead of cramming in a hidden property like this, why not delete the getter once we've got the cached URI? Like so: get _downloadURIObj() { delete this._downloadURIObj; return this._downloadURIObj = NetUtil.newURI(this.downloadURI); } @@ +585,5 @@ > > + /** > + * Given a data item for a session download, or a places node for a past > + * download, updates the view as necessary. > + * 1. If the given data is a places node, we check whether there are any Nit: "There are any element" -> "There are any elements" @@ +588,5 @@ > + * download, updates the view as necessary. > + * 1. If the given data is a places node, we check whether there are any > + * element for the same download url. If there are, then we just reset > + * their places node. Otherwise we add a new download element. > + * 2. If the given data is a data item, we first check if there's an history Nit: "an history" -> "a history"

I filed bug 822710 to investigate reducing the queries load due to annotations... it will be a hack but it's the simplest thing to do atm...

.filesize and exists() are the other obvious target of optimization, these are something we should probably try to update asynchronously. I wonder if we load downloads in a sort of "idle" state, and they become active when we get back the stat info. I'm also going to file a bug to expire downloads harder than commmon history, while there are use-cases for a long past history, use-cases for downloads from 2 or 3 (or more) months ago is quite more restricted.

Another alternative would be lazy richlist contents loading (on scroll, when the last line is shown, add more items). I can't tell off-hand how hard would it be though. Mano, any thought on this?

I 'm switching to async OS.File api

Comment on attachment 693052 [details] [diff] [review] Step 1 - some obvious stuff (use a document fragment for the initial batch, cache nsIURI objects http://hg.mozilla.org/integration/mozilla-inbound/rev/46b1c88b81c2

Hrm, it seems the -1 value I used for the "unknown" download state is already in use. I guess I should use |undefined| with no state attribute at all in that case.

So, I applied my patches and your patch, unfortunately looks like we are still slow, will investigate further.. Applying this patch I see a bunch of "Error: Could not find target file path for history download " in the error console...

I think there's also another bug there, since many of the downloads here are showing an empty progress bar, while they should not

Comment on attachment 693814 [details] [diff] [review] Step 2 - async io Review of attachment 693814 [details] [diff] [review]: ----------------------------------------------------------------- clearing since looks like there is still debug work to do here (See previous comments about bugs and reportErrors) ::: browser/components/places/content/downloadsView.js @@ +105,5 @@ > if (this._placesNode != aNode) { > this._annotations = new Map(); > this._placesNode = aNode; > + > + // We don't need to update the UI if we had a data item, because s/had/have/ @@ +197,5 @@ > + this._targetFileInfoFetched = false; > + > + let path = this._targetFilePath; > + if (!path) { > + Cu.reportError("Could not find target file path for history download '" + this.downloadURI + "'. "); this happens quite frequently, should be removed and a comment should be added. this leaves the download in the initial limbo, so that should be verified "working" @@ +216,5 @@ > + this.__targetFileExists = false; > + } > + else { > + Cu.reportError("Could not fetch info for target file (reason: " + > + aReason + ")"); this case is again leaving the download in the uncertain limbo @@ +219,5 @@ > + Cu.reportError("Could not fetch info for target file (reason: " + > + aReason + ")"); > + } > + > + this._updateDownloadStatusUI(); since in both cases you want to invoke this._updateDownloadStatusUI() you can use something like promise.then(success, failure).then(finally) @@ +235,5 @@ > + get _targetFileSize() { > + if (!this._targetFileInfoFetched) > + throw new Error("Info about the target file was not yetfetched"); > + if (!this._targetFileExists) > + throw new Error("The target file does not exist"); I'm worried _targetFileExists and _targetFileSize instead of returning a default status are throwing... this may cause unwanted bailouts... @@ +257,5 @@ > + if (!this._targetFileInfoFetched) { > + // Here we cannot just throw an error until the info is fecthed because > + // we need the download element to show on the screen properly. > + // Thus return the "Unknown" state. > + return -1; we don't seem to handle -1 state anywhere... should we update something in this state? @@ +277,5 @@ > + let annosvc = PlacesUtils.annotations; > + PlacesUtils.annotations.setPageAnnotation( > + this._downloadURIObj, DOWNLOAD_STATE_ANNO, state, 0, > + PlacesUtils.annotations.EXPIRE_WITH_HISTORY); > + this._annotations.set(DOWNLOAD_STATE_ANNO, state); this looks like may be a perf hog the first time we open the view :( Is not this going to run a query for each entry? that is hundreds of writes. No, I don't have a solution off-hand. it also stored some bogus value in my DB so now at every opening I see progressbars on old items. @@ +333,5 @@ > case nsIDM.DOWNLOAD_DIRTY: > return s.stateDirty; > case nsIDM.DOWNLOAD_FINISHED:{ > + // XXXmano: if the target file info isn't fetched yet, or the targt > + // file deosn't exist, maybe we should return "Finshed"/"Completed". probably, yes @@ +475,5 @@ > + // 3.2. For past downloads, disable the command until we know the > + // file is in place. > + if (this._dataItem && !this._dataItem.openable) > + return false; > + trailing spaces @@ +488,5 @@ > + // the file exists. > + // 2.3. For session downloads, assume we can show the file > + // if the download is ongoing or completed. > + if (this._targetFileInfoFetched) > + return this._targetFileExists; in some situations only the .part file esists, but looks like you removed support for that... IIRC that happens on pause and resume, or retry (don't remember off-hand) @@ +494,5 @@ > + let state = this._state; > + return state == nsIDM.DOWNLOAD_FINISHED || > + state == nsIDM.DOWNLOAD_SCANNING || > + state == nsIDM.DOWNLOAD_PAUSED || > + state == nsIDM.DOWNLOAD_DOWNLOADING; trailing space @@ +538,5 @@ > if (this._dataItem) > this._dataItem.showLocalFile(); > else > + DownloadsCommon.showDownloadedFile( > + GetFileForFileURI(this._targetFileURI)); ditto for the .part file sometimes being the only one.

I'm looking into a profile gathered on a trybuild with my patches to reduce the overhead in Places queries (bug 822710 and bug 823156). Total time taken to to show the view (on my computer and with my 550 downloads profile) is 2s. 600ms of the time is spent in layout::Flush, more than half of this time (19% of the global time) is spent in DoReflow. So, looks like there could be space to improve richlistbox reflows, especially cause most of the rlb is not visible. 1200ms of the time is the actual code: - 50ms is the places query that fetches downloads history - 1154ms is invalidateContainer: |- 880ms directly into invalidateContainer (self) |- 212ms in _addDownload |- 132ms in creating new shells |- 62ms in getAnnotationsFor globally the database IO has been reduced to 112ms, that is 5,6% of the total time, so, apart reducing number of downloads (bug 822788), doesn't look like we can go much further there.

fwiw, in bug 747969 Neil said lazy richlistbox would not be too hard to implement if all elements have same height. While I'm not sure we can do that in time, it's worth to track that.

the "self" time in invalidateContainer is spent in: this._richlistbox.appendChild(elementsToAppendFragment); the remaining time is spent in the second loop that _addDownloadData

The _addDownloadData cost is mostly spent in the _state and _statuText getters. Actyally what is expensive is _state, but since we forgot to cache its value, _statusText is basically duping the cost by asking for _state. The cost of the _state getter is all concentrated in a single call: this._file.exists() (main-thread file stat()). Mano's patch above is properly acting on this loop by doing 2 things: 1. cache _state (Note that to properly solve this we also need to figure when to store state into an annotation, that we are not yet doing) 2. use OS.file to asynchronously get the file stats So with my Places patches and Mano's patch (properly fixed) we are basically left with the extremely slow appendChild problem, that we still lack a good attack plan.

based on experiments and discussion through IRC, this shaves off 40% of the appendChild time. 1. avoid command 2. avoid <stack> 3. avoid visibility: hidden

Marco: That last patch - attachment 694798 [details] [diff] [review] - appears to be the reverse of the patch for bug 815678 (Downloads Panel changes width when scanning completed download)... I think this patch will regress us on bug 815678... -Mike

Ach - I'm sorry, I was confused; the naming of downloads.css is so close to download.css that I thought we'd be affecting the panel. I see now that this change will *only* affect the Library View. Crisis averted. :)

Yes, we need to rename the panel files to downloadPanel or something similar, feel free to file the bug, or even do that. We didn't cause the patches were too large already.

remaining work to do in this bug: the above patch "step 2" should be splitted into these 2 parts. 1. patch to properly handle _state. We should set the anno in the backend for session downloads, and cache _state in the view after its first fetch. I'm not sure when we should also store annos for the old downloads, eventually follow-up so we measure the perf hog apart. 2. patch to convert mainthread file stat() operations (exists() and filesize) to OS.File.

With this latest set of patches, my debug build in a profile with ~500 downloads on Ubuntu now takes about 2.5 seconds to render upon clicking "Downloads". A very palpable improvement! Great work!

Comment on attachment 695236 [details] [diff] [review] Part 4 - Cache the download state so command-updating doesn't check the file size over and over Review of attachment 695236 [details] [diff] [review]: ----------------------------------------------------------------- I'm OK with this - just one suggestion (see below). ::: browser/components/downloads/content/allDownloadsViewOverlay.js @@ +215,5 @@ > > // The target's file size in bytes. If there's no target file, or If we > // cannot determine its size, 0 is returned. > get _fileSize() { > + if (!("__fileSize" in this)) { Instead of having a __fileSize property on this object, why not simply delete / replace the _fileSize getter once you've gotten the value the first time? Something like: get _fileSize() { // ... get the file size the first time - I'll assign to "gottenSize" // to illustrate. delete this._fileSize; return this._fileSize = gottenSize; } I think I'd prefer that over this current pattern.

You cannot use this technique on a "prototyped" object. The property getter is defined on the prototype, but |delete| only affects the instance, thus it would fail to remove the getter.

Try this function foo() {} foo.prototype = { get p() { delete this.p; this.p = 1; return this.p; } }; alert(new foo().p) Error: too much recursion Source File: javascript:%20function%20foo()%20{}%20foo.prototype%20=%20{%20get%20p()%20{%20delete%20this.p;%20this.p%20=%20%201;%20return%20this.p;%20}%20};%20alert(new%20foo().p) Line: 1

Comment on attachment 695236 [details] [diff] [review] Part 4 - Cache the download state so command-updating doesn't check the file size over and over http://hg.mozilla.org/integration/mozilla-inbound/rev/ad84d41d4145

While testing this today I enabled the pref and tried to open the Download Library and the system promptly went 'not responding'. Eventually it kicked a slow-script warning at which time I killed the script and the Library opened. I did have several hundred files in the Download Library and during the 'hang' "not responding" I noted that MSE (Microsoft Security Essentials) was running 25% CPU on a quad CPU so effectively 100% CPU. Once I killed the script MSE settled down to 0%CPU. Its not totally reproducible, seems maybe a close/restart of the browser may again trigger the hang. Why would opening the library even trigger MSE to scan the files ? I don't think the old way did that except a scan when downloads were completed. I do have browser.download.manager.scanWhenDone set to 'false' as I don't care for the hang caused by MSE on downloads. Any relevance here or is this another bug to check on later. I did clear all my download history and even with MSE running the Library will open instantly and MSE seems to not scan anything. For a test, I did disable MSE and found that before I deleted all download history that there was no hang with the large number of downloads in the Library.

(In reply to Marco Bonardo [:mak] (intermittently avail. until 3 Jan) from comment #24) > Created attachment 694798 [details] [diff] [review] > Part 3 - Avoid some costs in appendChild > > based on experiments and discussion through IRC, this shaves off 40% of the > appendChild time. > 1. avoid command > 2. avoid <stack> > 3. avoid visibility: hidden This patch is not submitted, but will have a positive impact on performance.

Jim, thanks a lot for the input. A couple of things: 1) The slowness discussed and covered here is cross-platform, and, if there's a problem with MSE, it's unlikely to be resolved here. In fact, both Marco and Mike were not using Windows to test the new view, as far as I know. Therefore I suggest opening a new bug for the MSE issue. 2) Our "explicit" triggering of anti-virus scan (the one controlled by browser.download.manager.scanWhenDone) sure has nothing to do with what you see. Rather, it seems that something we do with the files as the view loads (existence check, size check, mime type check and maybe more) is implicitly triggering the MSE scan. Unfortunately I know almost nothing about MSE. I don't know when does it trigger itself and what could we do to avoid that. That's also why a separate bug report is preferable here. Again, thanks for the input.

(In reply to Alfred Kayser from comment #38) > (In reply to Marco Bonardo [:mak] (intermittently avail. until 3 Jan) from > comment #24) > > Created attachment 694798 [details] [diff] [review] > > Part 3 - Avoid some costs in appendChild > > > > based on experiments and discussion through IRC, this shaves off 40% of the > > appendChild time. > > 1. avoid command > > 2. avoid <stack> > > 3. avoid visibility: hidden > > This patch is not submitted, but will have a positive impact on performance. Hrm? Here's the checkin https://hg.mozilla.org/mozilla-central/rev/9810612fbb64

Mano, we still lack setting the state annotation, maybe worth filing it apart?

(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #37) > While testing this today I enabled the pref and tried to open the Download > Library and the system promptly went 'not responding'. Eventually it kicked > a slow-script warning at which time I killed the script and the Library > opened. Likely Part 2 will help, since the AV will then work in a separate thread. one we also store the state in an annotation we could avoid some lookups. As Mano said this should be files apart so we can better check the AV impact separately.

For some reason, today's Nightly is quite usable and fast for me, the hang is down to about 1s, annoying but not totally unusable. If anyone else has positive feedback to report in this regard, we may remove the pref and enable the view for Nightly testing. Provided we'll keep working on perf that would allow to collect bug reports on its functionality.

This is an experimental patch implementing a sort of lazy loading of the richlistbox, the behavior is the common "scroll to bottom to load more results"

this also avoids invoking getDownloadState on invisible items. It's acceptably fast also on Alice's profile, we should likely go this way or something similar.

Moved the experimental unhide in chunks behavior to bug 825846. Mano, how far are we from the "async IO" and "state in backend" parts?

Just wondering if I'm seeing this bug or a different one on Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:20.0) Gecko/20130103 Firefox/20.0 ID:20130103030946 Steps: 1. Download a file. 2. Click the "Download button" (Green down pointing arrow). 3. Click "Show All Downloads" Result: A script on this page may be busy, or it may have stopped responding. You can stop the script now, or you can continue to see if the script will complete. Script: chrome://browser/content/downloads/allDownloadsViewOverlay.js:930

(In reply to alex_mayorga from comment #50) > Just wondering if I'm seeing this bug or a different one it's this one.

Comment on attachment 697987 [details] [diff] [review] Part 2 - async i/o Review of attachment 697987 [details] [diff] [review]: ----------------------------------------------------------------- nothing blocking. The empty statusText looks bad, basically the item has an icon and the filename that is not vertically centered compared to the icon. it's weird since there's so much empty space between items and everything looks unbalanced. I think you should add a localizable "Unknown" or similar status text... ::: browser/components/downloads/content/allDownloadsViewOverlay.js @@ +122,5 @@ > if (!this._dataItem && this._placesNode) { > this._wasInProgress = false; > this._wasDone = this.getDownloadState(true) == nsIDM.DOWNLOAD_FINISHED; > this._updateStatusUI(); > + this._fetchTargetFileInfo(); this is a problem since we should not fetch anything until the item is needed (that currently is immediately but with lazy loading it's not). At this point I wonder if you just did this expecting my patch to just take care of it by moving the calls to when items are actually shown (basically what I did in the experimental patch)? I will work on the lazy patch on top of yours. @@ +213,5 @@ > + return ""; > + }, > + > + _fetchTargetFileInfo: function DES__fetchTargetFileInfo() { > + this._targetFileInfoFetched = false; may we check _targetFileInfoFetched here and throw if we re-enter the method with it set to true? @@ +216,5 @@ > + _fetchTargetFileInfo: function DES__fetchTargetFileInfo() { > + this._targetFileInfoFetched = false; > + let path = this._targetFilePath; > + if (!path) { > + Cu.reportError("Could not find target file path for history download '" + this.downloadURI + "'. "); I think we discussed this in the past and since many old downloads may not have DESTINATION_FILE_URI_ANNO it's just too noisy and should be removed. rather add a comment explaining when it may happen (old downloads) @@ +254,3 @@ > * @param [optional] aForceUpdate > * Whether to force update the cached download state. Default: false. > + @ return the download state if available, |undefined| otherwise. this looks like a typo should be * @return @@ +442,5 @@ > this._wasInProgress = this._dataItem.inProgress; > > this._updateDownloadStatusUI(); > + > + if (this._element.selected) nit: remove empty line before this @@ +471,3 @@ > } > case "downloadsCmd_show": { > + // TODO: Handle part-file asynchronously. trailing whitespace... file a bug and add bug number here please, not blocking since limited to session downloads @@ +471,5 @@ > } > case "downloadsCmd_show": { > + // TODO: Handle part-file asynchronously. > + if (this._dataItem && this._dataItem.inProgress) > + return this._dataItem.partFile && this._dataItem.partFile.exists(); I'm not sure if checking .inProgress is correct, IIRC I tried to cancel a download, then retried and canceled again and after that it only had a .part file. I'm sure there may be cases where a non in-progress download has just a part file. Would it be bad to just keep the _dataItem check since this only happens for a limited number of session entries? @@ +519,5 @@ > if (this._dataItem) > this._dataItem.showLocalFile(); > else > + DownloadsCommon.showDownloadedFile( > + GetFileForFileURI(this._targetFileURI)); may not work properly in the above case where we only have a part file @@ +1122,5 @@ > // Set the state attribute so that only the appropriate items are displayed. > let contextMenu = document.getElementById("downloadsContextMenu"); > + let state = element._shell.getDownloadState(); > + if (state !== undefined) > + contextMenu.setAttribute("state", state); what does the context menu show if state is not set? everything? ::: browser/components/downloads/content/download.css @@ +40,5 @@ > > .download-state:not( [state="1"] /* Finished */) > + .downloadShow, > + > +.download-state:not([state]) button please use the > child selector

The "store download state" part moved to bug 826991, so once lazy loading and async io land this can be resolved.

From comment #2 about the size of the list of downloads, I decided to look at the list. It was rather large. I started to delete the no longer necessary items and found that selecting of all of them (ctrl+a) and then pressing the delete key did not work. I did this because the "clear list" button is missing. I added date to the columns but the date column did NOT appear. In fact, the only column that appeared was the filename. This display used to be more useful but now "Library" is used. Why this change? Deleting all the extra entries did speed things up.

(In reply to George R. Goffe from comment #55) > From comment #2 about the size of the list of downloads, I decided to look > at the list. It was rather large. I started to delete the no longer > necessary items and found that selecting of all of them (ctrl+a) and then > pressing the delete key did not work. Please file a separate bug. I did this because the "clear list" > button is missing. We are adding it in bug 822572 I added date to the columns but the date column did NOT > appear. we are hiding the Views menu in bug 822572, it's not supported. > In fact, the only column that appeared was the filename. This > display used to be more useful but now "Library" is used. Why this change? Long discussion, btw we plan to make the Library view at least as powerful as the old DM window.

Since this bug has quite some patches already I've filed a new bug to investigate the hangs and we'll handle improvements in separate bugs from now on.

Works ok in the latest Nightly (20130204030941) and Aurora (20130204042019). I wasn`t able to reproduce using the STR from comment #0 and comment #50. In my opinion this is verified fixed, do you agree?