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

VERIFIED WONTFIX

Status

P2
normal
VERIFIED WONTFIX
18 years ago
14 years ago

People

(Reporter: sspitzer, Assigned: sspitzer)

Tracking

({perf})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
QA Contact: esther → stephend
Hardware: PC → All
accepting.  the patch I'm about to attach includes the batch delete patch.  I'll
get a clean patch tomorrow or monday.
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla0.8
Created attachment 23669 [details] [diff] [review]
patch, cleaner patch coming soon.

Comment 3

18 years ago
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.
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]
moving to 0.9 
Target Milestone: mozilla0.8 → mozilla0.9

Comment 6

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

Comment 7

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

Comment 8

18 years ago
I'm marking WONTFIX because this bug won't exist anymore when the perf branch lands.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → WONTFIX
Target Milestone: mozilla0.9 → ---
amen, verified.
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.