Stephen Moehle Reporter
	Description • 23 years ago

Using the 'n' key and advancing to the next unread news group is unbearably
slow.  On my 1 GHz machine it takes 15 seconds during which time Mozilla is
taking up 100% of the CPU and all Mozilla windows are completely unresponsive. 
This bug first appeared after the fix for bug 103824 was checked in.  

To reproduce:
1) Use a news server with about 40 subscribed groups.  I am using
news.mozilla.org and the mozilla groups.
2) Make all message in all groups read.
3) Go to the first group and press 'n'.
4) Mozilla freezes for about 15 seconds and takes 100% of the CPU.  All Mozilla
windows are unresponsive including all Browser windows.

Tested with Mozilla trunk build 2001101221 on Linux and 20011011?? on Win2000.

	stephend@netscape.com (gone - use stephen.donner@gmail.com instead)
	Comment 1 • 23 years ago

Confirming.  On Windows 2000 with the 10-13-2001 build, I get 100% CPU and huge 
memory usage (kills GDI resources, until I terminate Mozilla's process).

	scottputterman
	Comment 3 • 23 years ago

*** Bug 106115 has been marked as a duplicate of this bug. ***

	scottputterman
	Comment 4 • 23 years ago

*** Bug 106437 has been marked as a duplicate of this bug. ***

	scottputterman
	Comment 5 • 23 years ago

Based on the dups, it's not just newsgroups so changing summary.

In one of the dups, Neil did some research and wrote the following:

"I think I have found two related causes:
1. GetSubFoldersInFolderPaneOrder returns 175+ folders! Only 18 are real
folders, the others all appear to be copies of "Sent". N.B. Every time I start
Mozilla it appears to create a new Sent folder.
2. GetSubFoldersInFolderPaneOrder is called 1000+ times! Probably as a
side-effect of cause 1."

	Neil Pilgrim
	Comment 6 • 23 years ago

Just to note that I see this on linux 2001101808, but that if I'm in a folder
which is a sibling of Inbox/Sent/etc. then the search appears to be dramatically
faster? ~5s for Inbox->sibling-folder, whereas much faster for
sibling-folder->sibling-folder/subfolder. This could just be a consequence of
the search occurring only in the subfolders, ie. a reduction in the total number
of folders to be searched.

	(not reading, please use seth@sspitzer.org instead)
	Comment 7 • 23 years ago

accepting.

the 100+ sent folders scares me.

"Every time I start Mozilla it appears to create a new Sent folder."

that shows up on disk, or in the folder pane, or both?

	neil@parkwaycc.co.uk
	Comment 8 • 23 years ago

Seth, it shows up in msgViewNavigation.js if I add
dump(msgFolders.length + "\n");
just before the return from GetSubFoldersInFolderPaneOrder().
It does not show up on the folder pane.

	neil@parkwaycc.co.uk
	Comment 9 • 23 years ago

I've been looking to this and so far I've made one improvement: this comparator
is three times faster than the existing function(s). It also works with servers.

function compareFolderOrder(folder1, folder2) {
  if (folder1.sortOrder != folder2.sortOrder)
    return folder1.sortOrder - folder2.sortOrder;
  var folderName1 = folder1.name.toLowerCase();
  var folderName2 = folder2.name.toLowerCase();
  if (folderName1 < folderName2)
    return -1;
  else if (folderName1 > folderName2)
    return 1;
  else
    return 0;
}

	neil@parkwaycc.co.uk
	Comment 10 • 23 years ago

Actually it doesn't work for servers, but that's only done once, so no problem.

	neil@parkwaycc.co.uk
	Comment 13 • 23 years ago

Next Unread Message is useable for me with this patch, even though I'm suffering
from phantom folders. It should also be generally faster.

	scottputterman
	Comment 14 • 23 years ago

reassigning to naving.  Navin, could you help look into this problem where
folders are showing up multiple times during this operation and help with Neil's
patch?

	Navin Gupta Assignee
	Comment 15 • 23 years ago

Neil, I think all these phantom folders have corresponding phantom msf files in 
the profiles, this is what I have found out and I have a fix for that problem. 
Do you see phantom folders created in any other way ?

	neil@parkwaycc.co.uk
	Comment 16 • 23 years ago

While I agree that I have 200 phantom .msf files, which I hope your fix will 
resolve, I think that someone with a large number of folders or newsgroups 
would still want to see the speed increase that my patch has to offer.

	Navin Gupta Assignee
	Comment 17 • 23 years ago

sure, I am in the process of reviewing your patch. 

	Navin Gupta Assignee
	Comment 19 • 23 years ago

The fix for the phantom folders is to check if we already have a subfolder
with the same name in mSubFolders array. This is necessary because when we start
up, we iterate through the mail directory and create folders based on the msf
files (imap) so if there are msf duplicates (sent.msf, sent-1.msf) with the same
online/mailbox name, they were being added to mSubFolders array, so that is why
we were having phantom folders.

cc bienvenu for review. 

	Navin Gupta Assignee
	Comment 20 • 23 years ago

Comment on attachment 55219 [details]
New msgViewNavigation.js

r=naving. The newMsgNavigation is faster and nice code clean-up using good js.

	David :Bienvenu
	Comment 21 • 23 years ago

why not use GetChildWithURI ?

	Navin Gupta Assignee
	Comment 22 • 23 years ago

GetChildWithURI uses GetSubFolders and that does a bunch of stuff based on
initialized flag, things like updating summary totals that i don't need here. 
It will make thing more complicated than it ought to be.

	Navin Gupta Assignee
	Comment 24 • 23 years ago

Ok, the mInitialized flag is set as soon we create the folder, before creating
subfolders - can use GetChildWithURI. Howerver, I had to move up the local 
folder mInitialized flag to do this exactly. 

	David :Bienvenu
	Comment 25 • 23 years ago

Comment on attachment 56596 [details] [diff] [review]
patch for phantom folders, v2

looks good, as long as moving up mInitialized is OK (which I think it should be).

	(not reading, please use seth@sspitzer.org instead)
	Comment 26 • 23 years ago

Comment on attachment 56596 [details] [diff] [review]
patch for phantom folders, v2

sr=sspitzer

	(not reading, please use seth@sspitzer.org instead)
	Comment 27 • 23 years ago

I don't think the new JS is correct.

before, we'd compare the value of the collation keys 
("http://home.netscape.com/NC-rdf#Name?sort=true")

that makes it work non US-ASCII folder names, and that's how we sort in the 
folder pane.

that's not the same thing as comparing the sort order, and if the same, 
comparing the .name on the folder.

	neil@parkwaycc.co.uk
	Comment 28 • 23 years ago

Actually it is, because that's how the sort order is generated. If you look at 
line 1060 or so of nsMsgFolderDataSource.cpp the collation value is the the 
sort order 1-6 plus the name in lower case.

	scottputterman
	Comment 29 • 23 years ago

did this already get checked in?

	Navin Gupta Assignee
	Comment 30 • 23 years ago

I checked in only phantom folders part. reviews still continuing for other 
part.
 

	neil@parkwaycc.co.uk
	Comment 31 • 23 years ago

Phantom folders part works well. Now my next unread is really zippy.

	neil@parkwaycc.co.uk
	Comment 32 • 23 years ago

Should the getNumUnread(true) function work on servers? Sometimes I always get 0.

	Navin Gupta Assignee
	Comment 33 • 23 years ago

Seth, neil is correct about sorting folders based on 1-6 and lowercase name. 
Could you sr the patch ?

>Should the getNumUnread(true) function work on servers? Sometimes I always get 0.
yes, it works but sometimes it will return 0 (legal value). i think this may
happen if the folders in that account have not been selected even once - we
haven't had a chance to read from db or the folder cache. 

	(not reading, please use seth@sspitzer.org instead)
	Comment 34 • 23 years ago

> Phantom folders part works well. Now my next unread is really zippy.
excellent!

> Actually it is, because that's how the sort order is generated. 
> If you look at line 1060 or so of nsMsgFolderDataSource.cpp the collation 
> value is the the sort order 1-6 plus the name in lower case.

good point.  

the problem is that the code in nsMsgFolderDataSource.cpp is wrong.

looking at 
http://lxr.mozilla.org/mozilla/source/content/xul/templates/src/nsXULSortService
.cpp#902 and (the code that sorts the xul templates we generate)
http://lxr.mozilla.org/mozilla/source/content/xul/templates/src/nsXULOutlinerBui
lder.cpp#1819 (the code that sorts the folder pane)
 
we're going to treat the literal we generate in nsMsgFolderDataSource.cpp#1060 
as a key, which it is not.

here's what we should do:

1) add "readonly attribute wstring sortKey" to the nsIMsgFolder interface.
2) move the code from the if (sort) {) block in nsMsgFolderDataSource.cpp#1060, 
to the GetSortKey() implementation in nsMsgFolder.cpp
3) fix GetSortKey() to actually generate collation keys
4) fix nsMsgFolderDataSource.cpp#1060 to use GetSortKey()

all that will fix folder pane sorting, and menu sorting.  (which is broken, 
because we aren't generation collation keys)

5) then, you can fix the JS to use array.sort(), folder.sortKey

nsICollation isn't scriptable, so to compare the sort keys, you're going have 
to add an method to compare them to some existing, scriptable interface, and 
pass in your sort keys to it, and use nsICollation from there.

we should review how we are doing sorting of accounts.  account names can have 
non-ASCII characters, like folder names, which means sorting is broken their 
too.

this should all be in a new bug, that blocks the landing of neil's patch.

	Navin Gupta Assignee
	Comment 35 • 23 years ago

ok, you may be right we cannot just do a strcmp and therefore we need to memcmp -
that is what CompareRawSortKey does. I don't see any use of keys because
XULSortService finally compares literals. 

	neil@parkwaycc.co.uk
	Comment 36 • 23 years ago

1. I agree that nsIMsgFolder (including servers) should have some sort of
sortKey attribute which this patch and RDF can sort by, ensuring that they sort
in sync. But surely the raw sort key is the one generated by
nsMsgFolderDataSource.cpp?
2. The problem server total is always 1 less than its sole folder unread count.

	Navin Gupta Assignee
	Comment 37 • 23 years ago

The collation key is used to generate sortKey that is used for sorting
purposes.

	neil@parkwaycc.co.uk
	Comment 38 • 23 years ago

getNumUnread always returns 1 too few for me :-( at least for my IMAP servers

	Navin Gupta Assignee
	Comment 39 • 23 years ago

*** Bug 114123 has been marked as a duplicate of this bug. ***

	Navin Gupta Assignee
	Comment 40 • 23 years ago

it is the same new msgViewNavigation.js with the sort comparator changed. It
works real well, as expected.

	(not reading, please use seth@sspitzer.org instead)
	Comment 41 • 23 years ago

it's going to take me a little while to wrap my head around this js again, and 
make sure that the new code is doing the right thing.  it might be a few days 
before I can review.

can you verify that with the patch, we do the same thing that existing code did:

if there is no more unread in the current folder, find the next folder "below" 
the current folder in the folder pane with unread messages, and select it, 
skipping certain special folders.

when the "bottom" of the folder pane is hit, start at the top of the folder 
pane, making sure to stop (and avoid the infinite loop.)

note, this can cause us to "leave" an account with unread mail, and move to the 
next account, in certain scenarios.  (is that desired?).

there's been some debate (cc alecf, laurel) on how where "n" should go when 
crossing folders.

navin, can you verify that the current patch does the same thing as the current 
impl.  laurel might already have a "test" matrix.

laurel & jglick, can you verify that this is the desire behaviour?  either way, 
can you summarize the desired behaviour and put it on 
http://www.mozilla.org/mailnews/specs?

on a related note, if you have cycles, can you look into:
http://bugzilla.mozilla.org/show_bug.cgi?id=59638, which makes "n" painful to 
use.

	Navin Gupta Assignee
	Comment 42 • 23 years ago

I have verified that the current impl and the patch do the same thing. 
It always completes one acct (leaving out special folders) and then 
goes onto next. 

	Navin Gupta Assignee
	Comment 43 • 23 years ago

Removed some dumps statements and I don't see any uses of
GetTopLevelMessageForMessage (can be removed). Most of the
patch is clean-up. The sort comparator that neil measured to be three times
faster should be true now also because that is how we are creating collation
key.

I have tested and it works ok in these cases

>if there is no more unread in the current folder, find the next folder "below"

>the current folder in the folder pane with unread messages, and select it, 
>skipping certain special folders.

yes it does this. 

>when the "bottom" of the folder pane is hit, start at the top of the folder 
>pane, making sure to stop (and avoid the infinite loop.)

yes I see this behavior

>note, this can cause us to "leave" an account with unread mail, and move to
the 
>next account, in certain scenarios.  (is that desired?).

It always completes one acct before moving to next. 

seth, can you review.

	neil@parkwaycc.co.uk
	Comment 44 • 23 years ago

Just fixing some indentation including a couple of tab characters (slap wrist
:-)

	(not reading, please use seth@sspitzer.org instead)
	Comment 45 • 23 years ago

>>note, this can cause us to "leave" an account with unread mail, and move 
>>to the next account, in certain scenarios.  (is that desired?).
>
>It always completes one acct before moving to next. 

the current implementation will not complete an account first.  Meaning, it is
possible to move on to another account while the current account has unread
messages.  (for example, if Inbox has unread, but I'm reading in a folder XYZ,
below Inbox, and I hit n, I will go on to the next account.)

does your patch do that, or will it go back to Inbox?

jglick / laurel:  which behavior is desired?

	laurel
	Comment 46 • 23 years ago

*** Bug 103989 has been marked as a duplicate of this bug. ***

	Alec Flett
	Comment 47 • 23 years ago

sorry to chime in late here, but I really don't think we should 'finish an
account' before moving to the next. To me, next means the "Next folder with new
messages" not "the first folder above the current one since there are no new
messages in this account"

	laurel
	Comment 48 • 23 years ago

I vote to finish an account before going to the next account, but will bow to
Jennifer's vote or whatever is decided.  This is one of those issues that will
never please everyone.

	David :Bienvenu
	Comment 49 • 23 years ago

I agree with Alec - we should go down in strict folder pane order, and not loop
back to finish the account. But I agree with Laurel that we'll never please
everyone :-)

	Alec Flett
	Comment 50 • 23 years ago

I agree, jennifer should have the final say, but I wanted to voice my reasons :)

	Navin Gupta Assignee
	Comment 51 • 23 years ago

>the current implementation will not complete an account first.  Meaning, it is
>possible to move on to another account while the current account has unread
>messages.  (for example, if Inbox has unread, but I'm reading in a folder XYZ,
>below Inbox, and I hit n, I will go on to the next account.)

does your patch do that, or will it go back to Inbox?

It will not go back to Inbox. Sorry didn't check the above case, the patch does
the same thing as the current implementation. It follows a strict top-down
order. 

	(not reading, please use seth@sspitzer.org instead)
	Comment 52 • 23 years ago

I agree with david and alec, we should follow strict folderpane order.

I think of next as "down" in the current thread pane, and then "down" in the 
folder pane, never up, until at rock bottom.

thanks for checking that the current patch does that, navin.  I'll go finish 
reviewing, while we wait for jglick to comment.

jglick, could you add your decision to the spec, so that we can point to it 
later?

	(not reading, please use seth@sspitzer.org instead)
	Comment 53 • 23 years ago

Comment on attachment 62182 [details] [diff] [review]
fixed indentation

patch looks good, and sounds like navin has testing it well.

instead of commenting out
GetTopLevelMessageForMessage(), let's just remove it since it is not used.

if we need it when adding additional navigation, we can add it back.

once that is removed, sr=sspitzer

	Navin Gupta Assignee
	Comment 54 • 23 years ago

removed GetTopLevelMessageForMessage()

fixed.

	neil@parkwaycc.co.uk
	Comment 55 • 23 years ago

Navin, you checked in the bad indentation :-(

	Stephen Moehle
	Comment 56 • 23 years ago

Advancing to the next folder is really fast now.  This bug is fixed as far as I
am concerned.

Mozilla trunk 2001-12-20-03 on Win2000.

	jglick
	Comment 57 • 23 years ago

Sorry for delay. I also agree we should go down in folder pane order, and not 
loop back to finish an account.

	stephend@netscape.com (gone - use stephen.donner@gmail.com instead)
	Comment 58 • 23 years ago

This is now working fine for me (from a performance perspective) using builds:

Mac OS 9.1 - 2001-12-20-04
Mac OS X 10.1 - 2001-12-20-04
Windows 2K - 2001-12-20-03
RedHat 7.2 - 2001-12-20-04

I've done some additional ad-hoc testing with navigation, and noticed we
disregard (on IMAP, at least) Sent and Trash, so these are the special folders
that Navin mentions.

If there are any remaining issues with navigation, please file separate bugs,
thanks.

Verified FIXED