Closed Bug 249128 Opened 20 years ago Closed 19 years ago

Reduce string copying in |nsImapProtocol::HandleMessageDownLoadLine| to increase IMAP message loading performance

Categories

(MailNews Core :: Networking: IMAP, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: engel, Assigned: engel)

References

Details

(Keywords: perf)

Attachments

(1 file, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040616
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040616

When mail messages are downloaded via IMAP, every line of the message will be
copied once in |nsImapProtocol::HandleMessageDownLoadLine()|.  This can be
avoided in most cases, increasing IMAP performance.

Reproducible: Always
Steps to Reproduce:
The number of copies made for each incoming IMAP message is reduced by one.

The argument name in |HandleMessageDownLoadLine| is somewhat confusing.  I
suggest to use rename 'chunkEnd' --> 'hasMoreChunks'.

Note that the message line is only modified if |!hasMoreChunks|, thus we do not
need to strdup it if |hasMoreChunks|.
 
The message line is then passed to some of the following functions.
  + PostLineDownLoadEvent
    --> nsIImapMessageSink::ParseAdoptedMsgLine(line,  ) 
	[see nsImapMailFolder.cpp#3914]
  + nsMsgImapLineDownloadCache::CacheLine() --> nsByteArray::AppendString()
  + nsIImapHeaderInfo::CacheLine(), which is also a nsMsgImapLineDownloadCache,

    see |nsMsgImapHdrXferInfo::GetFreeHeaderInfo|
All these functions a) do not keep a pointer to the line and b) do not modify
the line (we can mark it as const).  So, we do not need to make a copy of the
line.

Further, there is a comment and some code indicating that we need to leak the
message line in some specific error cases.  This was added in revision 1.9
(1999-03-28), apparently some 4.5 code was added.  However, since the message
line is never stored, it seems that we do not need to handle these cases here. 
Along the same argument, |downLoadInfo| is never stored anywhere and can be
freed immediately.
Keywords: mail1, perf
Attachment #151977 - Flags: review?(mscott)
confirming bug
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #151977 - Attachment is obsolete: true
Attachment #151977 - Flags: review?(mscott)
Assignee: bienvenu → Hans-A.Engel
Status: NEW → ASSIGNED
Product: MailNews → Core
Hans, will you propose a new patch?
stephend: Yes.  Actually I already have a much nicer patch since 2004-07-04
(when I made the old patch obsolete), but I would like to test it somewhat more.
The current trunk code also contains the following (very minor) bug.  If
|MSG_LINEBREAK == "\n"| and the line is "x\n", the line is incorrectly changed
to "x\n\n", equivalently for "\r".

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/imap/src/nsImapProtocol.cpp&rev=1.582&root=/cvsroot#3253
Remove almost all strdups from HandleMessageDownLoadLine, substantially
increasing performance while downloading large messages from fast IMAP server.

Further, this patch
+  uses strlen only once,
+  renames argument |chunkEnd| --> |isPartialLine|,
+  uses |PR_MALLOC| and  |PL_strcpy| instead of |PR_CALLOC| and |strcpy|,
+  adds a comment about correct usage,
+  treats CRCRLF consistent on different systems,
+  formulates assumptions on |MSG_LINEBREAK| in a |NS_PRECONDITION|,
+  fixes the bug mentioned in Comment 5,
+  adds extra argument |lineCopy| to HandleMessageDownLoadLine,
+  makes the code much easier to read :)
I should have used |else if| instead of |if|, extending the patch (attachment
167049 [details] [diff] [review]) to

(...)
+          endOfLine[-1] = '\0';
+          lineLength--;
+        }
-        if ((endOfLine[-1] == nsCRT::CR) || (endOfLine[-1] == nsCRT::LF))
+        else if ((endOfLine[-1] == nsCRT::CR) || (endOfLine[-1] == nsCRT::LF))
         {
-          /* LF -> CRLF or CR -> CRLF */
-          endOfLine[-1] = MSG_LINEBREAK[0];
(...)
Comment on attachment 167049 [details] [diff] [review]
Remove almost all strdups from nsImapProtocol::HandleMessageDownLoadLine

(patch broken due to fixes on Bug 106386 and Bug 273080)
Attachment #167049 - Attachment is obsolete: true
Attachment #169402 - Flags: review?(bienvenu)
Attachment #169402 - Flags: superreview+
Attachment #169402 - Flags: review?(bienvenu)
Attachment #169402 - Flags: review+
fix checked in
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Verified FIXED using LXR and Bonsai:

1.592	bienvenu%nventure.com	2005-02-06 15:48	 	fix 249128 reduce string copying
in HandleMessageDownloadLine and 249999 Eliminate fTokenizerAdvanced from imap
parser, patches by Hans-Andreas, r/sr=bienvenu
Status: RESOLVED → VERIFIED
Blocks: 313038
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: