Last Comment Bug 474626 - Optimize invalidation on entry updates in new download manager
: Optimize invalidation on entry updates in new download manager
Status: RESOLVED FIXED
: fixed-seamonkey2.0
Product: SeaMonkey
Classification: Client Software
Component: Download & File Handling (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
Depends on: 472001
Blocks: 523351 506731
  Show dependency treegraph
 
Reported: 2009-01-21 09:56 PST by Robert Kaiser (not working on stability any more)
Modified: 2009-10-20 07:06 PDT (History)
6 users (show)
neil: wanted‑seamonkey2.0+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed patch (4.30 KB, patch)
2009-10-04 11:54 PDT, neil@parkwaycc.co.uk
iann_bugzilla: review+
iann_bugzilla: approval‑seamonkey2.0+
Details | Diff | Review

Description Robert Kaiser (not working on stability any more) 2009-01-21 09:56:47 PST
In the new download manager tree, we're resorting the whole tree when any entry updates but we should probably be more intelligent on that, see bug 472001 comment #19 and some comments following it.
Comment 1 neil@parkwaycc.co.uk 2009-08-07 02:52:57 PDT
(Originally bug 506731 comment #2)

I've come up with two possible tests we can do to avoid a full sort. The test
goes in sortView just after the definition of compfunc. The first is this:

var sorted = true;
this._dlList.reduce(function(previous, current) {
  if (compfunc(previous, current) <= 0)
    sorted = false;
  return current;
}
if (sorted)
  return;

The second assumes you provide the index of the row you updated like this:

if (aRow >= 0 &&
    (aRow == 0 ||
     compfunc(this._dlList[aRow - 1], this._dlList[aRow]) > 0) &&
    (aRow + 2 <= this._dlList.length ||
     compfunc(this._dlList[aRow], this._dlList[aRow + 1]) > 0))
  return;

So it's more efficient but less readable. Note that neither of these options
was the one I mentioned in bug 472001 where I suggest only invalidating the
area between the row's original and target position, although the second option
could be adapted to do so by virtue of saving the item originally at aRow and
subsequently checking its new position.
Comment 2 neil@parkwaycc.co.uk 2009-09-29 06:25:30 PDT
Here's another alternative, again assuming the row is passed in:

this._cacheSelection();
var dl = this._dlList[aRow];
this._dlList.sort(compfunc);
if (this._dlList[aRow] == dl)
  this._selectionCache = null;
else {
  this._tree.invalidate();
  this._restoreSelection();
}
Comment 3 neil@parkwaycc.co.uk 2009-09-29 06:29:38 PDT
this._cacheSelection();
var dl = this._dlList[aRow];
this._dlList.sort(compfunc);
var row = this._dlList.indexOf(dl);
if (row == aRow)
  this._selectionCache = null;
else if (row < aRow)
  this._tree.invalidateRange(row, aRow);
else
  this._tree.invalidateRange(aRow, row);
this._restoreSelection();
Comment 4 neil@parkwaycc.co.uk 2009-09-29 06:31:47 PDT
Marking wanted as this blocks a wanted bug. (And then maybe someone will notice and suggest which version of code they'd agree to review!)
Comment 5 Robert Kaiser (not working on stability any more) 2009-09-29 11:49:47 PDT
If I'm supposed to review, I need a diff with context, I don't have the time right now to figure out all sorts of things myself.
Comment 6 Ian Neal 2009-09-29 14:50:47 PDT
I think the one in comment#3 looks the most readable, doesn't mean it's the best though!
Comment 7 neil@parkwaycc.co.uk 2009-10-04 11:54:57 PDT
Created attachment 404527 [details] [diff] [review]
Proposed patch

* Used a local variable in updateDownload rather than repeating _dlList[row]
* Passed extra arguments to sortView to indicate if only one row was updated
* Also handled the case when (re-)initialising the view and there is no row.
Comment 8 neil@parkwaycc.co.uk 2009-10-04 11:56:06 PDT
(In reply to comment #5)
> If I'm supposed to review, I need a diff with context, I don't have the time
> right now to figure out all sorts of things myself.
I wasn't asking for a review, only a decision ;-)
[But now I have that decision, you can still do the review if you like!]
Comment 9 Ian Neal 2009-10-05 15:52:42 PDT
(In reply to comment #7)
> Created an attachment (id=404527) [details]
> Proposed patch
> 
> * Used a local variable in updateDownload rather than repeating _dlList[row]
> * Passed extra arguments to sortView to indicate if only one row was updated
> * Also handled the case when (re-)initialising the view and there is no row.

How does the revised sortView deal with http://mxr.mozilla.org/comm-central/source/suite/common/downloads/treeView.js#466 ? Does it still do what is expected?
Comment 10 neil@parkwaycc.co.uk 2009-10-06 02:31:47 PDT
(In reply to comment #9)
>How does the revised sortView deal with
>http://mxr.mozilla.org/comm-central/source/suite/common/downloads/treeView.js#466
>? Does it still do what is expected?
As it can't find a download then row is -1 and it just invalidates the tree.
Comment 11 neil@parkwaycc.co.uk 2009-10-06 07:50:32 PDT
Pushed changeset b5919a3c9730 to comm-central.
Comment 12 Robert Kaiser (not working on stability any more) 2009-10-17 11:09:31 PDT
Hmm, from all I see, this only changes what we invalidate in the tree view, but not the sorting itself. Do we need another bug to optimize when/what we sort in the array or is that action really fast anyhow?
Comment 13 neil@parkwaycc.co.uk 2009-10-20 07:06:50 PDT
Updating summary to reflect what I actually ended up doing.

Note You need to log in before you can comment on or make changes to this bug.