The default bug view has changed. See this FAQ.

Optimize invalidation on entry updates in new download manager

RESOLVED FIXED

Status

SeaMonkey
Download & File Handling
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Robert Kaiser, Assigned: neil@parkwaycc.co.uk)

Tracking

(Blocks: 1 bug, {fixed-seamonkey2.0})

Trunk
fixed-seamonkey2.0
Dependency tree / graph
Bug Flags:
wanted-seamonkey2.0 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

8 years ago
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.
(Assignee)

Updated

8 years ago
Blocks: 506731
(Assignee)

Comment 1

8 years ago
(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.
(Assignee)

Comment 2

8 years ago
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();
}
(Assignee)

Comment 3

8 years ago
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();
(Assignee)

Comment 4

8 years ago
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+
(Reporter)

Comment 5

8 years ago
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

8 years ago
I think the one in comment#3 looks the most readable, doesn't mean it's the best though!
(Assignee)

Comment 7

8 years ago
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.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #404527 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 8

8 years ago
(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

8 years ago
(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?
(Assignee)

Comment 10

8 years ago
(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.

Updated

8 years ago
Attachment #404527 - Flags: review?(iann_bugzilla)
Attachment #404527 - Flags: review+
Attachment #404527 - Flags: approval-seamonkey2.0+
(Assignee)

Comment 11

8 years ago
Pushed changeset b5919a3c9730 to comm-central.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: fixed-seamonkey2.0
Resolution: --- → FIXED
(Reporter)

Comment 12

8 years ago
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?
(Reporter)

Updated

8 years ago
Blocks: 523351
(Assignee)

Comment 13

8 years ago
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.