66847 - delete message performance optimization: don't call getElementById() [make the variable gNextMessageAfterDelete an element, not an id]

Status: VERIFIED WONTFIX

(SeaMonkey :: MailNews: Message Display, defect, P2)

Description

[(not reading, please use [e-mail] instead)]

calling getElementById() is very expensive.  (see bug #14001)

given the complexity of our document, we don't want to be calling it any more
than we have to.

when we delete a message, we eventually get into 
HandleDeleteOrMoveMsgCompleted() in msgMail3PaneWindow.js

from my jprof log, I could see that we were spending a lot of time in there.

I've rewritten the code and remove two unnecessary calls to getAttribute('id')
and one to getElementById()

patch coming soon.

Comment 1

[(not reading, please use [e-mail] instead)]

accepting.  the patch I'm about to attach includes the batch delete patch.  I'll
get a clean patch tomorrow or monday.

Comment 2

[(not reading, please use [e-mail] instead)]

Attachment: patch, cleaner patch coming soon.

Comment 3

[scottputterman]

Seth, make sure to test threaded mode.

Suppose you have:

a
  b

And you delete 'a'.  This means that 'b' will be the next message.

I don't know if we've changed this since I last looked at it, but the way delete
in threads used to work is that b would get removed from a and then added back
in.  This cause a different tree node to get created and therefore the one you
are hanging onto is not valid.  That was the reason for using uri's since they
will always be valid.  I can't think of other example where this might also be
true at the moment.

Comment 4

[(not reading, please use [e-mail] instead)]

after reading 14001, it looks like getElementById() isn't so slow for xul
elements.  but if we can avoid calling it, we should.  

putterman is right, my patch breaks in threaded mode.  I'll investigate how
expensive getElementById() is, and if it is worth it, I'll try to fix the delete
when in threaded mode problem.

here's the exception I get with my patch:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE)
[nsITreeBoxObject.getIndexOfItem]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)" 
location: "JS frame ::
chrome://global/content/treeBindings.xml#tree.ensureElementIsVisible() ::
ensureElementIsVisible :: line 1"  data: no]

Comment 5

[(not reading, please use [e-mail] instead)]

moving to 0.9 

Comment 6

[Håkan Waara]

So when will the "clean patch" come?  Shall we add the "patch" keyword to this bug?

Comment 7

[David :Bienvenu]

this bug will be invalid when the performance branch lands - ignore it.

Comment 8

[scottputterman]

I'm marking WONTFIX because this bug won't exist anymore when the perf branch lands.

Comment 9

[[e-mail] (gone - use [e-mail] instead)]

amen, verified.