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)
MailNews Core
Networking: IMAP
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: engel, Assigned: engel)
References
Details
(Keywords: perf)
Attachments
(1 file, 2 obsolete files)
15.30 KB,
patch
|
Bienvenu
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
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:
Assignee | ||
Comment 1•20 years ago
|
||
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.
Assignee | ||
Updated•20 years ago
|
Assignee | ||
Updated•20 years ago
|
Attachment #151977 -
Flags: review?(mscott)
Assignee | ||
Updated•20 years ago
|
Attachment #151977 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #151977 -
Flags: review?(mscott)
Assignee | ||
Updated•20 years ago
|
Assignee: bienvenu → Hans-A.Engel
Assignee | ||
Updated•20 years ago
|
Status: NEW → ASSIGNED
Updated•20 years ago
|
Product: MailNews → Core
Comment 3•20 years ago
|
||
Hans, will you propose a new patch?
Assignee | ||
Comment 4•20 years ago
|
||
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.
Assignee | ||
Comment 5•20 years ago
|
||
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
Assignee | ||
Comment 6•20 years ago
|
||
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 :)
Assignee | ||
Comment 7•20 years ago
|
||
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]; (...)
Assignee | ||
Comment 8•20 years ago
|
||
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
Assignee | ||
Updated•20 years ago
|
Attachment #169402 -
Flags: review?(bienvenu)
Updated•19 years ago
|
Attachment #169402 -
Flags: superreview+
Attachment #169402 -
Flags: review?(bienvenu)
Attachment #169402 -
Flags: review+
Comment 10•19 years ago
|
||
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
Updated•15 years ago
|
Product: Core → MailNews Core
You need to log in
before you can comment on or make changes to this bug.
Description
•