99393 - optimize CompFields2Recipients()

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect)

Description

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

I see some low hanging fruit in CompFields2Recipients(), details coming shortly.

Comment 1

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

on new mail compose, and reply, we call CompFields2Recipients() which will call 
nsMsgCompFields::SplitRecipients() and awSetInputAndPopupFromArray()

I think we call SplitRecipients() 5 times on a new compose window, and I think 
we don't have to call it all.

basically, we should not call it if the first argument (recipients) is null.  
in the case of new compose window, it is.

nsMsgCompFields::SplitRecipients(nsMsgCompFields * const 0x04c8ac90, const 
unsigned short * 0x049594f0, int 0x00000000, nsIMsgRecipientArray * * 
0x0012cd60) line 629
XPTC_InvokeByIndex(nsISupports * 0x04c8ac90, unsigned int 0x0000003b, unsigned 
int 0x00000003, nsXPTCVariant * 0x0012cd40) line 139
XPCWrappedNative::CallMethod(XPCCallContext & {...}, XPCWrappedNative::CallMode 
CALL_METHOD) line 1952 + 42 bytes
XPC_WN_CallMethod(JSContext * 0x034cec20, JSObject * 0x00e6be50, unsigned int 
0x00000002, long * 0x0359daa4, long * 0x0012cf78) line 1254 + 14 bytes
js_Invoke(JSContext * 0x034cec20, unsigned int 0x00000002, unsigned int 
0x00000000) line 807 + 23 bytes
js_Interpret(JSContext * 0x034cec20, long * 0x0012dd1c) line 2719 + 15 bytes
js_Invoke(JSContext * 0x034cec20, unsigned int 0x00000000, unsigned int 
0x00000000) line 824 + 13 bytes
js_Interpret(JSContext * 0x034cec20, long * 0x0012ea78) line 2719 + 15 bytes
js_Invoke(JSContext * 0x034cec20, unsigned int 0x00000000, unsigned int 
0x00000002) line 824 + 13 bytes
nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJSClass * const 0x04ca9fc0, 
nsXPCWrappedJS * 0x04ca9f30, unsigned short 0x0003, const nsXPTMethodInfo * 
0x035653a0, nsXPTCMiniVariant * 0x0012efc0) line 1022 + 21 bytes
nsXPCWrappedJS::CallMethod(nsXPCWrappedJS * const 0x04ca9f30, unsigned short 
0x0003, const nsXPTMethodInfo * 0x035653a0, nsXPTCMiniVariant * 0x0012efc0) 
line 427
PrepareAndDispatch(nsXPTCStubBase * 0x04ca9f30, unsigned int 0x00000003, 
unsigned int * 0x0012f070, unsigned int * 0x0012f060) line 100 + 31 bytes
SharedStub() line 124
nsMsgCompose::NotifyStateListeners(nsMsgCompose * const 0x04c8af20, 
TStateListenerNotification eComposeFieldsReady, unsigned int 0x00000000) line 
2888
nsMsgDocumentStateListener::NotifyDocumentCreated(nsMsgDocumentStateListener * 
const 0x042dade0) line 2490
nsEditor::NotifyDocumentListeners(nsEditor * const 0x04948a70, 
nsEditor::TDocumentListenerNotification eDocumentCreated) line 2525 + 23 bytes
nsEditor::PostCreate(nsEditor * const 0x00350364) line 347
nsHTMLEditor::PostCreate(nsHTMLEditor * const 0x006e4cb8) line 302 + 9 bytes
nsEditorShell::PrepareDocumentForEditing(nsIDOMWindow * 0x0012f5ec, nsIURI * 
0x77e16469) line 592 + 32 bytes
nsEditorShell::EndPageLoad(nsIDOMWindow * 0x0367dde4, nsIChannel * 0x04c878fc, 
unsigned int 0x0012f8d0) line 5164 + 27 bytes
nsEditorShell::OnStateChange(nsEditorShell * const 0x0000000c, nsIWebProgress * 
0x77ea13fd, nsIRequest * 0x0367ddf4, int 0x04c8654c, unsigned int 0x0012f91c) 
line 4990
nsDocLoaderImpl::FireOnStateChange(nsIWebProgress * 0x102117b7, nsIRequest * 
0x00000009, int 0x0012fc74, unsigned int 0x00000000) line 1095
nsDocLoaderImpl::doStopDocumentLoad(nsIRequest * 0x00000000, unsigned int 
0x00482960) line 735
nsDocLoaderImpl::DocLoaderIsEmpty() line 632
nsDocLoaderImpl::OnStopRequest(nsDocLoaderImpl * const 0x00000008, nsIRequest * 
0x01d0c440 nsWindow::DefaultWindowProc(HWND__ *, unsigned int, unsigned int, 
long), nsISupports * 0x00000000, unsigned int 0x00000001) line 563
nsLoadGroup::RemoveRequest(nsLoadGroup * const 0x0012fb08, nsIRequest * 
0x1001d6ea, nsISupports * 0x00482960, unsigned int 0x0012fb18) line 516 + 44 
bytes
nsStreamIOChannel::OnStopRequest(nsStreamIOChannel * const 0x00000002, 
nsIRequest * 0x01d4756c, nsISupports * 0x00000000, unsigned int 0x0352a130) 
line 466
nsOnStopRequestEvent::HandleEvent() line 162
nsARequestObserverEvent::HandlePLEvent(PLEvent * 0x0352a8c4) line 65
PL_HandleEvent(PLEvent * 0x0012fc74) line 590 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00490201) line 520 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x00000000, unsigned int 0x00000113, unsigned 
int 0x00003761, long 0x22f5ee30) line 1071 + 9 bytes
USER32! 77e13eb0()
USER32! 77e1401a()
USER32! 77e192da()
nsAppShellService::Run(nsAppShellService * const 0x03b90730) line 446
main1(int 0x00000004, char * * 0x00485ca0, nsISupports * 0x00000000) line 1328 
+ 32 bytes
main(int 0x00000004, char * * 0x00485ca0) line 1650 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e87903()

Comment 2

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

you should also investigate SplitRecipientsEx(), make sure the same thing isn't 
happening there.

Comment 3

[varada]

Attachment: Minimise calls to SplitRecipients

Comment 4

[Jean-Francois Ducarroz]

Comment on attachment 49245 [details] [diff] [review]
Minimise calls to SplitRecipients

Looks good. R=ducarroz

Comment 5

[varada]

Attachment: Added Asserts in splitrecipients and changed name of global variable

Comment 6

[varada]

Scenarios tested:
1-Opening with -compose option.
2-New, Reply, Reply-All, Forward(inline/attachment) from Mail.
3-Send link, Send page, mailto: from Browser.
4-New, Reply, Reply to sender/newsgroup/all from News
Tested with various combinations of To,CC,BCC-using groups of one,two and three 
at a time.

Comment 7

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

don't return failure on null, just assert.  we still want to handle this case, 
but we do want to let the developer know it's not smart.

+  if(!recipients)
+  {
+    NS_ASSERTION(0, "The recipient list is not supposed to be null!");
+    return NS_ERROR_FAILURE;
+  }
+

something like
NS_ASSERTION(recipients, "fix your caller blah blah blah");
 

Comment 8

[varada]

Attachment: Got rid of the NS_ERROR_FAILURE return.

Comment 9

[varada]

Attachment: Some more minor fixes and style changes.

Comment 10

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

sr=sspitzer

nice work!  you're fixing these faster than I can find them. 

Comment 11

[varada]

Marking Fixed.

Comment 12

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

Verifying this will have to wait until next week's numbers, since this was 
checked in at 2pm, and the build I used was the morning build from 9/14.

Here are the numbers, to compare the final number (when I run the 9/21 build 
again on 9/25)

Build Date - Average time in seconds to reply to a 2K html message with 5 
recipients (http://www.mozilla.org/mailnews/win_performance_results.html)

8/31 - 4.16 
9/10 - 4.20
9/14 - 4.10

Comment 13

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

Windows 2K was 4.13 this week (averaged).  But still, this is fixed from a
code-standpoint (and, I couldn't get the official numbers to generate from
msgcompose:5, something's up, I'll investigate).