Closed
Bug 104610
Opened 23 years ago
Closed 23 years ago
Using 'n' and advancing to next folder is unbearably slow
Categories
(SeaMonkey :: MailNews: Message Display, defect, P1)
SeaMonkey
MailNews: Message Display
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla0.9.8
People
(Reporter: smoehle, Assigned: naving)
References
Details
(Keywords: perf)
Attachments
(2 files, 7 obsolete files)
3.26 KB,
patch
|
Details | Diff | Splinter Review | |
13.53 KB,
patch
|
sspitzer
:
superreview+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•23 years ago
|
Component: Networking - News → Mail Window Front End
Status: UNCONFIRMED → NEW
Ever confirmed: true
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).
Keywords: nsbeta1
Severity: normal → major
Hardware: PC → All
Blocks: 63759
Comment 3•23 years ago
|
||
*** Bug 106115 has been marked as a duplicate of this bug. ***
Comment 4•23 years ago
|
||
*** Bug 106437 has been marked as a duplicate of this bug. ***
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."
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.
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?
Status: NEW → ASSIGNED
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.
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;
}
Comment 10•23 years ago
|
||
Actually it doesn't work for servers, but that's only done once, so no problem.
Comment 11•23 years ago
|
||
Comment 12•23 years ago
|
||
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.
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?
Assignee: sspitzer → naving
Status: ASSIGNED → NEW
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 ?
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.
Assignee | ||
Comment 17•23 years ago
|
||
sure, I am in the process of reviewing your patch.
Assignee | ||
Comment 18•23 years ago
|
||
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.
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.
Attachment #55219 -
Flags: review+
Comment 21•23 years ago
|
||
why not use GetChildWithURI ?
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.
Assignee | ||
Comment 23•23 years ago
|
||
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.
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).
Attachment #56596 -
Flags: review+
Comment 26•23 years ago
|
||
Comment on attachment 56596 [details] [diff] [review]
patch for phantom folders, v2
sr=sspitzer
Attachment #56596 -
Flags: superreview+
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.
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.
Comment 29•23 years ago
|
||
did this already get checked in?
Assignee | ||
Comment 30•23 years ago
|
||
I checked in only phantom folders part. reviews still continuing for other
part.
Comment 31•23 years ago
|
||
Phantom folders part works well. Now my next unread is really zippy.
Comment 32•23 years ago
|
||
Should the getNumUnread(true) function work on servers? Sometimes I always get 0.
Updated•23 years ago
|
Target Milestone: mozilla0.9.6 → mozilla0.9.7
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.
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.
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.
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.
Assignee | ||
Comment 37•23 years ago
|
||
The collation key is used to generate sortKey that is used for sorting
purposes.
Assignee | ||
Updated•23 years ago
|
Attachment #57874 -
Attachment is obsolete: true
Comment 38•23 years ago
|
||
getNumUnread always returns 1 too few for me :-( at least for my IMAP servers
Updated•23 years ago
|
Priority: -- → P1
Assignee | ||
Comment 39•23 years ago
|
||
*** Bug 114123 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 40•23 years ago
|
||
it is the same new msgViewNavigation.js with the sort comparator changed. It
works real well, as expected.
Attachment #53493 -
Attachment is obsolete: true
Attachment #55218 -
Attachment is obsolete: true
Attachment #55219 -
Attachment is obsolete: true
Attachment #56596 -
Attachment is obsolete: true
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.
Assignee | ||
Updated•23 years ago
|
Target Milestone: mozilla0.9.7 → mozilla0.9.8
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.
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.
Attachment #61188 -
Attachment is obsolete: true
Comment 44•23 years ago
|
||
Just fixing some indentation including a couple of tab characters (slap wrist
:-)
Updated•23 years ago
|
Attachment #62164 -
Attachment is obsolete: true
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?
Comment 46•23 years ago
|
||
*** Bug 103989 has been marked as a duplicate of this bug. ***
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"
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.
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 :-)
Comment 50•23 years ago
|
||
I agree, jennifer should have the final say, but I wanted to voice my reasons :)
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.
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?
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
Attachment #62182 -
Flags: superreview+
Assignee | ||
Comment 54•23 years ago
|
||
removed GetTopLevelMessageForMessage()
fixed.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 55•23 years ago
|
||
Navin, you checked in the bad indentation :-(
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.
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.
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
Status: RESOLVED → VERIFIED
Updated•20 years ago
|
Product: Browser → Seamonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•