162929 - Reduce number of proxied xpcom calls made during imap header download

Status: VERIFIED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[David :Bienvenu]

We make 3 proxied xpcom calls for each imap header we download. This seems to
take about 20-30% of the time to download headers. It will be fairly easy to
reduce to 1 per header, which will give us a nice speedup. It's possible we can
do less than one proxied xpcom per header if we implement a class that holds
onto multiple headers (say 10) and then proxies over an instance of that class
over to the ui thread. I'm going to make it so we only make one proxied call per
header, and see how much that helps. I'm not sure how of the proxy overhead is
just the events, and how much is the copying of data that happens currently
(I've experimented with making our proxied calls async where possible - it seems
to help a little, but it ends up copying lots of data because we're passing
around const char *'s now - if I make it so we pass an interface pointer over to
the ui thread, we can perhaps have the best of both worlds - async proxied
calls, w/o data copying.

Comment 1

[David :Bienvenu]

I'm about to attach a rough patch which reduces the number of proxied xpcom
calls by a factor of 3, which seems to give a roughly 7% speedup (it's hard to
measure these times precisely, and factor in server overhead, etc). We can
improve this by making the proxied interface asynchronous, which reduces the
xpcom proxy overhead because it doesn't have to post an event back to the
original thread to notify it of the event completion. That means that we would
have to use a locking mechanism on the header info object we pass back and forth
between the imap thread and the ui thread so the imap thread can know when the
ui thread is done with the data, and the imap thread can re-use the header info
object buffer w/o reallocating it. We should also make the header info object
have a set of buffers that it can cycle through - this way, we can also send
multiple headers objects across in one proxied call.

The other possibilities I talked to dougt about were using plain old PL_Events,
and moving the parsing code into the imap thread - I'm not sure the former will
have a big performance improvement, and the latter would require the db to be
thread-safe, and that's a daunting task.

Comment 2

[David :Bienvenu]

Attachment: rough patch

Comment 3

[David :Bienvenu]

Attachment: nsIImapHeaderInfo.idl

this is the idl file for the new interface.

Comment 4

[David :Bienvenu]

Attachment: proposed fix

This patch speeds up imap hdr downloading substantially (depending on your imap
server speed and network connection speed and processor speed...I'd guess
roughly around 30% on average for clicking on a large folder for the first
time). This patch gets rid of over 90% of the xpcom proxied calls. Instead of 3
proxied calls per hdr, I do one proxied call for every 10 headers (this is
tuneable in the code now). I also got rid of 2 more proxied calls per header
that were not needed. I removed some dead code (the tunnelling code)and
converted the imap line buffer to use the nsMsgLineBuffer. I also reduced the
frequency of status updating to every 3/4 second instead of 1/4, commented out
some monitor stuff that was never getting invoked (we were waiting on data
available monitor, but it only got notified as a result of processing events,
so in essence, it was a 50 millisecond sleep).

Comment 5

[Scott MacGregor]

1)Does the ref counting for a nsMsgImapHdrXfer work out okay? I notice in the
constructor we set the refcount to 1. 

2) In GetFreeHeaderInfo, are we holding two references to the nsIImapHeaderInfo,
aResult? Looks like it gets referenced here:
m_hdrInfos->QueryElementAt(m_nextFreeHdrInfo++, NS_GET_IID(nsIImapHeaderInfo),
(void **) aResult);
then again later in the routine:
NS_ADDREF(*aResult);

3)Do we have to worry about the thread safety of a the imap header cache object?
Is their a scenario where the imap thread would be putting header data into the
cache while we are reading it out? I'm guessing not, since it looks like we push
data into the header cache and then when it's full (or we are done), we push the
header cache across to the UI thread for parsing.


This looks like a great improvement David!

Comment 6

[David :Bienvenu]

1) is an issue, and I woke up planning on fixing that.
2) looks like a problem
3) Since we're still making synchronous proxied calls, thread safety isn't
currently an issue.

I'll attach a new patch for 1 and 2.

Comment 7

[David :Bienvenu]

2) is not an issue - we only addref the hdr info if we didn't get one from
QueryElementAt, i.e., if we're creating a new one.

I will run this through purify to make sure it doesn't introduce any new leaks.

Comment 8

[David :Bienvenu]

Attachment: cleanup ref-counting for xfer info, cache redirector type

I changed the ref counting for the hdr info and hdrxferinfo, so that the
protocol object AddRefs's the two objects that it has embedded. I also changed
the imap incoming server to cache the redirector type. This is just the diff
for nsImapProtocol.cpp and nsImapIncomingServer.cpp,h - the rest of the
previous diff still applies.

Comment 9

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

I reviewed at the last two patches, just some minor nits:

1)

+  nsresult rv = NS_OK;
+  PRUint32 numHdrs;
+  nsCOMPtr <nsIImapHeaderInfo> headerInfo;
+
+  rv = aHdrXferInfo->GetNumHeaders(&numHdrs);

why not do:

+  PRUint32 numHdrs;
+  nsCOMPtr <nsIImapHeaderInfo> headerInfo;
+
+  nsresult rv = aHdrXferInfo->GetNumHeaders(&numHdrs);

2)

+    *aMsgHdrs = GetBuffer();
+    return NS_OK;

should we check *aMsgHdrs for null, and return NS_ERROR_OUT_OF_MEMORY if
 
3)

+      *aResult = lineCache;
+      m_hdrInfos->AppendElement(lineCache);
+      NS_ADDREF(*aResult);

why not:

+      NS_ADDREF(*aResult = lineCache);
+      m_hdrInfos->AppendElement(lineCache);

4)

+  void ParseMsgHdrs(in nsIImapProtocol aProtocol, in nsIImapHeaderXferInfo 
aHdrXferInfo);

interCaps, so

+  void parseMsgHdrs(in nsIImapProtocol aProtocol, in nsIImapHeaderXferInfo 
aHdrXferInfo);

5)

+    char *msgHdrs;
+    headerInfo->GetMsgSize(&msgSize);
+    nsresult rv = SetupHeaderParseStream(msgSize, nsnull, nsnull);
+    NS_ENSURE_SUCCESS(rv, rv);
+    headerInfo->GetMsgUid(&msgKey);
+    headerInfo->GetMsgHdrs(&msgHdrs);

msgHdrs should be a const char *, right?

Comment 10

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

6)

+nsMsgImapHdrXferInfo::nsMsgImapHdrXferInfo()
+{
+  NS_INIT_ISUPPORTS();
+  NS_NewISupportsArray(getter_AddRefs(m_hdrInfos));
+  m_nextFreeHdrInfo = 0;
+  mRefCnt = 1; // just for now - we're embedded in nsImapProtocol object
+
+}

the "mRefCnt = 1;" seems weird to me.

You are doing this because this object is an attribute of nsImapProtocol

nsMsgImapHdrXferInfo  m_hdrDownloadCache;

why use XPCOM object at all?

Comment 11

[David :Bienvenu]

I used an xpcom object so we could use xpcom proxies.

msgHdrs is a char * because GetMsgHdrs() takes a char ** and I didn't see a way
to get the idl to generate a const char ** - there's probably some sort of
string foo I could use to avoid a copy here...

Comment 12

[David :Bienvenu]

Oh, and GetBuffer() doesn't do an allocation (it returns a pointer to an already
allocated buffer), so it might not be accurate to return out of memory there.

Comment 13

[David :Bienvenu]

Attachment: whole, proposed fix

this addresses the comments of Seth's that I could address. I also added code
to init the size of the various buffers to our desired initial size.

Comment 14

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

I think the string foo we want is:

readonly attribute ACString msgHdrs;

Comment 15

[David :Bienvenu]

Attachment: "final" patch, make msg hdrs const

OK, you have to use a non-scriptable method, but you can generate idl to make
the output const.

Comment 16

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

Comment on attachment 96221 [details] [diff] [review]
"final" patch, make msg hdrs const

david said he changed GetMsgHdrs -> getMsgHdrs locally.  sr=sspitzer, once you
get blessing from mscott.

Comment 17

[Scott MacGregor]

Comment on attachment 96221 [details] [diff] [review]
"final" patch, make msg hdrs const

r=mscott

Comment 18

[David :Bienvenu]

fix checked in.

Comment 19

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

Machine/Account Setup:

The OS X box is a G4, 500mhz with 384MB of RAM.
Both Linux and Windows 2000 are Dell Precision 220s.
The network was a 10/100 switched Ethernet, with an IMAP account of 4,055
headers.  I used a completely new profile for each build on each platform.

Linux:
--------------------------------
8-18 build:        10.73 seconds
8-29 build:         9.27 seconds

OS X:
--------------------------------
8-18 build:        16.10 seconds
8-30 build:        10.75 seconds

Windows 2000:
--------------------------------
8-18 build:         8.15 seconds
9-01 build:         6.14 seconds

Verified FIXED based on the results above.