99146 - reduce the work we do in opt builds when we aren't logging compose performance numbers

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect)

Description

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

we are doing some work to gather compose performance numbers that we can avoid 
in the case where the user doesn't have the NSPR_LOG_* environment variables 
set (that's the common case).

take a look at nsMsgComposeService.cpp and MsgComposeCommands.js.
start cleaning that timing code up and make it so that we don't incur any cost 
in opt builds when the nspr log vars aren't set.

because the timing code lives in the compose service, you are going to need a 
new pref.  I don't want us to cross the XPconnect boundary if we don't have too.

add a readonly boolean attribute to the compose service.  on initialization of 
the compose service, get the value for a bool pref 
(maybe "mail.logComposePerformance", since we already 
have "mail.showMessengerPerformance", add it to mailnews.js defaulting to false)

in, MsgComposeCommands.js, add

var logComposePerformace = msgComposeService.logComposePerformace;

(that will be our minimal cost per compose window.) and use that bool to avoid 
doing work in the .js file.

in the C++ file, use mLogComposePerformance to avoid work as well, setting that 
in ::Init(), which you will have to add, and you'll have to fix 
nsMsgComposeFactory.cpp, too.

if quantify wasn't crashing for you, you'd see a calls to sprintf(), PR_Now(), 
other PR_* calls, GetMessageSizeFromURI(), calls across XPConnect even if we 
didn't set the env variables.

it's not a huge win, but it gives you something to clean up until we figure out 
why quantifying is crashing for you.

once you'll land we'll have to let stephend know and we'll have to update 
http://www.mozilla.org/mailnews/performance/msgcompose_loggin.html to include 
this new pref.

Comment 1

[Sheela Ravindran]

reassign since this is a code related.

Comment 2

[varada]

Attachment: Patch for cleaning up TimeStamp Logging

Comment 3

[varada]

Attachment: Pref for enabling logging in mailcompose

Comment 4

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

1) nsresult nsMsgComposeService::GetLogComposePerformance() should be 
NS_IMETHODIMP nsMsgComposeService::GetLogComposePerformance()

2) don't define kPrefServiceCID use the contract ID for the pref service.

3) can you also remove "const DEBUG = false;" from MsgComposeCommands.js, if it 
isn't needed anymore?

4) in nsMsgComposeService::nsMsgComposeService(), you should default 
mLogComposePerformance to PR_FALSE;

5) you need to add some code to wrap the work we are doing in 
nsMsgComposeService with "if (mLogComposePerformance) {}".  we are still doing 
work we don't want to be doing.

Comment 5

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

for example, we don't want do any of this code if mLogComposePerformance is 
false.

#ifdef MSGCOMP_TRACE_PERFORMANCE
      // ducarroz, properly fix this in the case of new message (not a reply)
      if (type != nsIMsgCompType::NewsPost) {
        char buff[256];
        sprintf(buff, "Start opening the window, message size = %d", GetMessageS
izeFromURI(originalMsgURI));
        TimeStamp(buff, PR_TRUE);
      }
#endif

Comment 6

[varada]

Attachment: Patch V2 - timestamp logging cleanup

Comment 7

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

1) +  NS_IMETHODIMP Init();

"nsresult Init();" was correct.

2) please do gLogComposePerformance instead of logComposePerformance, that's 
the convention we've been using for globals.

other than that, looks good. 

Comment 8

[varada]

Attachment: Patch V3 for TimeStamp logging

Comment 9

[varada]

Jean-Francois/ Seth can you give your R/SR?

Comment 10

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

Comment on attachment 49038 [details] [diff] [review]
Patch V3 for TimeStamp logging

sr=sspitzer

Comment 11

[varada]

Marking Fixed.
Stephen could you update the page to reflect the new pref required to do 
timestamp logging.

Comment 12

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

Attachment: What I committed to gila.mozilla.org

Comment 13

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

Partially verified FIXED (on 2 platforms, until I get Linux verified):

When I insert:

user_pref("mailnews.logComposePerformance", true); in prefs.js on both Mac and
Windows on the trunk builds, my normal msgcompose:5 system environment variables
(and for Mac, dragging the text file with the appropriate ARGS:) works fine.
If I switch that to false, or remove that line completely (thereby letting
mailnews.js's value of false taking place), then msgcompose:5 has no effect on
logging any output for message reply speed or window creation time.

Mac OS 9.1 - 2001-09-13-08 TRUNK
Windows 2K - 2001-09-13 (TRUNK CVS pull)

Comment 14

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

Great, finally verified.
In BASH, doing:
Set NSPR_LOG_MODULES NNTP:5 
Set NSPR_LOG_FILE /u/stephend/tmp/log.txt

produced a textfile containing the output, when I had the user_pref specified.
When I re-ran that trunk build (2001-09-13-08) and changed the pref to a value
of false, log.txt was empty.

Comment 15

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

er,...right, set NRPR_LOG_MODULES msgcompose:5 (bad copy and paste)