Closed Bug 474626 Opened 11 years ago Closed 11 years ago

Optimize invalidation on entry updates in new download manager

Categories

(SeaMonkey :: Download & File Handling, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: kairo, Assigned: neil)

References

(Blocks 1 open bug)

Details

(Keywords: fixed-seamonkey2.0)

Attachments

(1 file)

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.
Blocks: 506731
(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.
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();
}
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();
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!)
Flags: wanted-seamonkey2.0+
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 think the one in comment#3 looks the most readable, doesn't mean it's the best though!
Attached patch Proposed patchSplinter Review
* 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.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #404527 - Flags: review?(iann_bugzilla)
(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!]
(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?
(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.
Attachment #404527 - Flags: review?(iann_bugzilla)
Attachment #404527 - Flags: review+
Attachment #404527 - Flags: approval-seamonkey2.0+
Pushed changeset b5919a3c9730 to comm-central.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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?
Blocks: 523351
Updating summary to reflect what I actually ended up doing.
Summary: Optimize sorting on entry updates in new download manager → Optimize invalidation on entry updates in new download manager
You need to log in before you can comment on or make changes to this bug.