:aceman Assignee
	Description • 12 years ago

After bug 450302 has landed, the rebuildFilterList function is called more times as the list toggles between filtered and unfiltered view.
Even in the unfiltered view this has performance impact if the list is very long.

I benchmarked on a 3Ghz machine with 10000 filters.
In this scenario, Move to Top takes 8 seconds even in unfiltered view.
In filtered view, Move Up takes 20 seconds.
Also setting the search string to a value that matches few filters takes on the order of 50 seconds.

I'll attach a patch shortly.

	:aceman Assignee
	Comment 1 • 12 years ago

So after applying this patch I get these times:
- 1 second
- 2 seconds
- 2-5 seconds (it depends a bit on how many elements were shown in the list before the new search term is written)

But I see some caching effects in that first time I do an operation it takes longer but all subsequent runs are very fast (the times written above).

The meat of the patch is this:
- optimize rebuildFilterList() to reuse existing listitems (instead of tearing all down and creating new ones) and then add/remove the tail as needed.
- remove many places where rebuildFilterList() is called and then the search is applied to it by removing non-matching rows. This was merged into rebuildFilterList() to only pass through the XUL rows once and only render those that match.

There are also some small optimizations, like removal of updateSearchBox() in some places (as it is called in rebuildFilteList already). And fixes to selection in moveFilter.

Axel please see if I didn't break any of your search logic anywhere.

I think there must be a way to also remove one rebuild when msgMoveMotion.Up and msgMoveMotion.Down. But I must think about that.
Also I want to shuffle the arguments to rebuildFilterList and document them, but I attach this to start discussion.

	Kent James (:rkent)
	Comment 2 • 12 years ago

Comment on attachment 649128 [details] [diff] [review]
WIP patch

So I looked over the patch, but did not try to apply it.

It has been several years since I worked in this code, so it would reasonably take me an hour or two to really review this carefully, which I am not going to to. Just looking at it briefly there were no obvious problems though, and it is worthwhile to fix some of these performance issues.

One note though: Code churn can break extensions. I found several extensions that use rebuildFilterList (though 2 involve Axel, the other active one is Filter of Filters by xzer) I'm not sure how your changes would affect those extensions, or whether the patch breaks them anyway. But if you could avoid changing the calling sequence to rebuildFilterList, it would be nice.

	:aceman Assignee
	Comment 3 • 12 years ago

Thanks a lot. rebuildFilterList after the patch still has the same functionality but I change the meaning of the argument. And the argument the extensions probably pass (gCurrentFilterList) will actually break the function after the patch.
I think Axel can remove the usage of rebuildFilterList (maybe all overlaying of TB filterlist dialog) in his QuickFolders extension (because the functionality is now merged in bug 450302). I can't say for the other extensions.

I changed the argument because it seemed basically unused (the passed in gCurrentFilterList was always the same current list, never something else).
I can keep the argument and just ignore it and add another one for my needs.
What are the proposals here?

	neil@parkwaycc.co.uk
	Comment 4 • 12 years ago

Well, I find the relationship between onFindFilter and rebuildFilterList to be quite confusing so I think you should split rebuildFilterList into two methods so that onFindFilter doesn't have to call it.

	Axel Grude [:realRaven]
	Comment 5 • 12 years ago

(In reply to :aceman from comment #3)
> Thanks a lot. rebuildFilterList after the patch still has the same
> functionality but I change the meaning of the argument. And the argument the
> extensions probably pass (gCurrentFilterList) will actually break the
> function after the patch.

I think Filter of Filters will be obsolete after the landing patch for bug 450302 - otherwise there will be multiple search boxes. Both quickFilters and QuickPasswords guard against that patch and omit the overlay in a patched Thunderbird, so there is no problem here either. They will only overload onTop, onBottom, onUp, onDown and onFindFilter  (and use rebuildFilterList(gCurrentFilterList)) when there is no searchBox element in the dialog.

> I changed the argument because it seemed basically unused (the passed in
> gCurrentFilterList was always the same current list, never something else).
> I can keep the argument and just ignore it and add another one for my needs.
> What are the proposals here?

I am not sure that it is safe to overwrite the global gCurrentFilterList  during setFolders(), need to apply your patch to see.

	:aceman Assignee
	Comment 6 • 12 years ago

Good idea Neil. But I could actually leave it at one function and just not call onFindFilter directly anymore. I moved the focusing stuff to rebuildFilterList and that allowed to strip onFindFilter a lot. Axel?

	:aceman Assignee
	Comment 7 • 12 years ago

(In reply to Axel Grude [:realRaven] from comment #5)
> I think Filter of Filters will be obsolete after the landing patch for bug
> 450302 - otherwise there will be multiple search boxes. Both quickFilters
> and QuickPasswords guard against that patch and omit the overlay in a
> patched Thunderbird, so there is no problem here either. They will only
> overload onTop, onBottom, onUp, onDown and onFindFilter  (and use
> rebuildFilterList(gCurrentFilterList)) when there is no searchBox element in
> the dialog.
That would be cool. I CCed a user xzer, I hope it is the author of the Filter of Filters extension.

(I have CCed Seamonkey guys by mistake, I always forget this one FilterListDialog.js file is TB-only. Sorry about that.)

> I am not sure that it is safe to overwrite the global gCurrentFilterList 
> during setFolders(), need to apply your patch to see.
I think previously seFolders was calling rebuildFilterList that also overwrote gCurrentFilterList so I moved this to setFolders directly, thinking it is equivalent. Please check it.

	xzer
	Comment 8 • 12 years ago

> That would be cool. I CCed a user xzer, I hope it is the author of the
> Filter of Filters extension.
> 

Thank you for cc me, aceman. I will be glad to see it becomes a built-in
function of TB. Since my extention will become obsolete after the bug 450302,
I do not think there is anything that should worry about.

Only one thing that I want to know which release version will include the
patch on bug 450302? I think I can add some version check to avoid misuse.

	xzer
	Comment 9 • 12 years ago

One more thing, I think the WIP patch here would be released with the patch of bug 450302 together, is there any misunderstanding?

If WIP patch will be released before bug 450302, I think it actually causes a problem to me.

	:aceman Assignee
	Comment 10 • 12 years ago

Bug 450302 is already fixed and marked as being released in Thunderbird 17.

The bug here is only an optional speed improvement for the filter list dialog, on top of bug 450302. I doubt it will still make it into TB17. It is true that only this bug here will break your extension, but you should actually disable your functionality when fix for bug 450302 is present so starting at TB17.

	xzer
	Comment 11 • 12 years ago

(In reply to :aceman from comment #10)
> Bug 450302 is already fixed and marked as being released in Thunderbird 17.
> 
> The bug here is only an optional speed improvement for the filter list
> dialog, on top of bug 450302. I doubt it will still make it into TB17. It is
> true that only this bug here will break your extension, but you should
> actually disable your functionality when fix for bug 450302 is present so
> starting at TB17.

It's good. I will kill my extension when TB17 being released.

	:aceman Assignee
	Comment 12 • 12 years ago

OK, I think I can still optimize moveFilter() to not rebuild the list twice (once generating full list and then the new filtered list). Then I can drop the new second argument to rebuildFilterList.

	:aceman Assignee
	Comment 13 • 12 years ago

CCing John David Galt as he is interested in the performance of the dialog with 1000s of filters (bug 450302 comment 49).

For testers, there is a filter generator at https://bugzilla.mozilla.org/attachment.cgi?id=514339 that you can use to create any number of filters.

	:aceman Assignee
	Comment 14 • 12 years ago

Optimization from comment 12. I could even remove both arguments to rebuildFilterList.
With this version, Move Up/Down takes 0.5s in filtered view (with 10000 out of 10003 filters shown as in comment 0).

	Axel Grude [:realRaven]
	Comment 15 • 12 years ago

Sorry I am still fighting with my c-c build, which appears to be broken after the latest tree checkout. Once I can build again, I will be able to give feedback.

	Magnus Melin [:mkmelin]
	Comment 16 • 12 years ago

Comment on attachment 652547 [details] [diff] [review]
patch v3

Review of attachment 652547 [details] [diff] [review]:
-----------------------------------------------------------------

Thx for the patch, just a few nits. r=mkmelin

::: mail/base/content/FilterListDialog.js
@@ +240,4 @@
>    if (selectedFilter) {
>      // Get the position in the unfiltered list.
>      // - this is where the new filter should be inserted!
> +    for (let i = 0; i < gCurrentFilterList.filterCount; i++) {

should cache gCurrentFilterList.filterCount

@@ +259,5 @@
>  
>      // Select the new filter, it is at the position of previous selection.
>      list.clearSelection();
> +    list.addItemToSelection(list.getItemAtIndex(position));
> +    updateViewPosition(position + 1);

you could add a comment that +1 is for the list header

@@ +519,5 @@
> +  while (activeElement != null) {
> +    if (activeElement == searchBox) {
> +      searchBoxFocus = true;
> +      break;
> +    } else if (activeElement.id) {

I'd prefer else if on a new line

@@ +557,5 @@
> +    if (aTempFilterList && aTempFilterList[listitemIndex] != i)
> +      continue;
> +
> +    if (listitemCount > listitemIndex) {
> +      // If there is free existing listitems, reuse it.

are .. them

@@ +562,5 @@
> +      // Use .children[] instead of .getItemAtIndex() as it is much faster.
> +      listitem = list.children[listitemIndex + 1];
> +      nameCell = listitem.childNodes[0];
> +      enabledCell = listitem.childNodes[1];
> +    } else {

else on new line

@@ +572,5 @@
> +      listitem.appendChild(nameCell);
> +      listitem.appendChild(enabledCell);
> +      list.appendChild(listitem);
> +      // We have to attach this listener to the listitem, even though we only care
> +      // about clicks on the enabledCell.  However, attaching to that item doesn't

multiple spaces before However

@@ +591,4 @@
>    }
> +  // Remove any superfluos listitems, if the number of filters shrunk.
> +  for (let i = listitemCount - 1; i >= listitemIndex; i--)
> +    list.removeChild(list.lastChild);

i think for clauses should always have braces

	:aceman Assignee
	Comment 17 • 12 years ago

(In reply to Magnus Melin from comment #16)
> @@ +259,5 @@
> >  
> >      // Select the new filter, it is at the position of previous selection.
> >      list.clearSelection();
> > +    list.addItemToSelection(list.getItemAtIndex(position));
> > +    updateViewPosition(position + 1);
> 
> you could add a comment that +1 is for the list header
updateViewPosition should operate on real listitems, that do not contain the headers. So I do not know what the +1 is for (I added that code in the past when I understood this list code a lot less). I just remove that line now as it is not needed (the currently selected line should not change after the new filter is inserted, so no need to update the view.

	:aceman Assignee
	Comment 18 • 12 years ago

While this is ready for checkin, I'd still like to hear the opinion from RealRaven, as it quite reworks his onFindFilter function he only recently added and completely reworks the way the list is redrawn.

Let's hope he can check it before TB17 is branched (I think Monday 27th)...

	Axel Grude [:realRaven]
	Comment 19 • 12 years ago

Comment on attachment 655317 [details] [diff] [review]
patch v4

feedback+ from me! Works as expected, nice patch. I only did smoke testing

	:aceman Assignee
	Comment 20 • 12 years ago

Comment on attachment 655317 [details] [diff] [review]
patch v4

Thanks.

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 12 years ago

https://hg.mozilla.org/comm-central/rev/83616ca185bb

	:aceman Assignee
	Comment 22 • 12 years ago

Comment on attachment 655317 [details] [diff] [review]
patch v4

This has baked on trunk for 10 days now. I haven't seen any bad reports about it.
Let's try to move it into the same release as bug 450302.

[Approval Request Comment]
Regression caused by (bug #): 450302
User impact if declined: unnecessary performance penalty when using the new features from bug 450302.

	Mark Banner (:standard8)
	Comment 23 • 12 years ago

https://hg.mozilla.org/releases/comm-aurora/rev/526e2bab1310