22960 - Improve message display performance

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect, P1)

Description

[Scott MacGregor]

I need a tracking bug to land the changes for message display. I'm just filing
this bug as a place holder.

Comment 1

[Scott MacGregor]

I just landed the new message display stuff tonight. You should see the
performance impact in tomorrow's builds. I'll post a larger summary of what I
did in the mailnews group. Look there for more details.

Suresh is going to help me generate new performance data using his suite of
tests for message display.

Comment 2

[Phil Peterson]

Message display performance did improve a lot, but the beta1 goal is to be
within 2x of 4.7, and we're not there yet. Reopening

Comment 3

[Scott MacGregor]

We'll start doing some quantify analysis to figure out where the hot spots are
(mime, layout, protocol) etc. More info soon.

Comment 4

[leger]

Putting on PDT+ radar for beta1 until we get down to 2x.

Comment 5

[Phil Peterson]

Display perf has regressed according to suresh's data. P1 for M14.

Comment 6

[Scott MacGregor]

I've done some quanitfy analysis and the two big bottlenecks for pop (I'm only
talking about mailnews stuff not layout) are:

For each on Data Available call given to the mime converter, we spend some 65%
of the time in the moxTxtToHTML stuff. And close to 30% in I18N related
conversion stuff. Most of the problems with the I18N stuff is that mime doesn't
hang on to the converters and we have to re-initialize them every time we parse
another line of data. I'm working on changing that right now. But the big low
hanging fruit is still ScanTxt and related methods in moxTXTToHTML.

I don't know if optimizing these two problems will be enough to make us only
twice as slow as 4.5.....I won't know till I get there.

Comment 7

[rhp (gone)]

Scott,
Let me know what is going on in libmime so we don't collide. I have a fair 
amount of work going on in there right now.

- rhp

Comment 8

[Scott MacGregor]

evaughan's box landing which dramatically reduced the number of reflows in the
message pane header area has helped improve the display time. Not enough to
bring us within our target but it has helped.

Comment 9

[Ben Bucksch (:BenB)]

mozTXTToHTMLConv perf improvement is bug #26915.

Comment 10

[Scott MacGregor]

The mime emitter was trying to load a style sheet: mailheader.css whenever we
displayed a message body. I assume this is needed in order to display headers in
forwarded messages.

Unfortunately, the way this style sheet was declared to be linked in, was
causing it to be loaded asynchronously. When layout was told we were done
loading the style sheet it would then re-layout the message body a second time,
applying the styles rules.

This accounted for the "double" load effect you see when you view messages. I
changed how this style sheet gets linked in by listing it as a IMPORTANT style
sheet in nsMimeHtmlEmitter.cpp. By doing this, layout waits until the style
sheet is loaded before it begins to layout the message body.

As a result, we only layout the document once instead of twice now. So there is
no more double load effect.

This change + the box optimizations evaughan landed last week which reduced some
reflows in the header pane helped shave 10seconds off the time it takes to
display 5 messages on imap and pop according to Suresh's latest data.

So the good news is we shaved almost 2 seconds per message. The bad news is we
still have to shave a lot more off in order to only be twice as slow as 4.5.

Comment 11

[Scott MacGregor]

I
have
some
ideas
butdon't
know
if
they
will
be
enough.

Comment 12

[Scott MacGregor]

There are at least three more pieces of low hanging fruit that show up in the
quantify logs that still need addressed. I have changes for one of them in my
tree now. As for the other two:

2) Every time we load a mail message, the mime emitter is loading a style sheet
called mailheader.css in the iframe that contains the message body. This causes
us to hit the disk to read in the file, parse the file and build up a style
sheet table. Since the contents of the iframe are torn down every time you load
another message body into it, we have to redo this every time.

The only reason we need to include the style sheet is in case we are displaying
a forwarded message that has headers in the body. If we hard coded the look and
feel of forwarded headers instead of making them skinnable via the style sheet,
we could get out of this pickle. This would be a big win

3) The other hotspot in the quantify logs is character set conversion time. For
each line of data, mime converts it into UTF-8. But mime is creating a new
characters set converter, initializing the converter for each line! We can save
a lot of time by caching the initialized converter instead of doing it every
time we go to convert a chunk of data. The quantify data I've been looking at
backs this up.

Comment 13

[Ben Bucksch (:BenB)]

> If we hard coded the look and feel of forwarded headers instead of making them
> skinnable via the style sheet,
> we could get out of this pickle.

If you're at the hard-coding track anywhere, can't you just look, if there's a
forwarded msg attached and load the stylesheet only in this case? Or are there
implementation issues?

Comment 14

[Scott MacGregor]

There are 3 big performance improvements ready to hit us (actually one already has).
1) MOZ_PERF is no longer defined in the release builds. This results in the
biggest time savings for displaying messages!
2) I have attached a patch to Bug #21203 which includes changes Ben and I have
made to the mozTXTToHTMLConverter to improve performance
3) I found a couple routines in mime which were making 4 extra string copies of
each line of data fed through the mime parser for the message body. I was able
to reduce this to just one extra copy in each of these two routines. I'm
attaching a fix for this problem to this bug report.

I'll get rich to review (3) and check that in soon. Hopefully (2) will happen
tomorrow.

Then we'll take some new measurements and probably declare victory on this bug
for beta.

The next step is to look at removing the style sheet we need to load with the
message body which forces us to hit the disk every time we display a message.

Comment 15

[Scott MacGregor]

Attachment: reduce # allocations of each line of data parsed by these 2 routines

Comment 16

[lchiang]

suresh just ran his performance tests on Win32 for this week.  Can we wait until 
next week to time again or just run the message display cases again with your 
fix for comparison?

Comment 17

[Scott MacGregor]

Yeah, we would only need to run the display message cases and not the whole
suite of timings. The numbers are looking good!

Comment 18

[Suresh]

Performance improvements in displaying messages after MOZ_PERF is removed from 
the release builds. (from 
http://www.mozilla.org/mailnews/win_performance_results.html)
                  
                                            Before            After
                                            (02/23/00)        (02/29/00)

POP: Display 5 msgs each of size 10 KB       37.98             14.298
IMAP:Display 5 msgs each of size 10 KB       39.3              15.154
IMAP:Display 2 msgs each of size 100 KB      221.83            24.014            

Comment 19

[lchiang]

Wow!

Comment 20

[Jim Roskind]

Can you update the status whiteboard with the expected landing dates? This looks
like a real nice performance win... but we need it in RSN.   The train will be
leaving....
Thanks,
Jim

Comment 21

[Scott MacGregor]

(1) and (3) are taken care of. Hopefully (2) will be resolved by tomorrow and
then regardless of the numbers, we will declare victory on this tomorrow.

Comment 22

[Scott MacGregor]

Okay I just landed optimizations for mozTXTToHTMLConv.cpp that should help.

Time to move this bug off the beta radar. We still have a ways to go and I still
have several meaty optimizations we can still make. But we are good enough for
beta1.

Moving the rest of the work to M15.

Comment 23

[Ben Bucksch (:BenB)]

mscott,
you made some good changes to mimetpla.cpp, but you missed mimetpfl.cpp, which
contains nearly the same code (did I say I hate redundancy?).

mimetpfl.cpp is used for format=flowed msgs, which Mozilla sends out by default.

Comment 24

[Scott MacGregor]

Hey Ben, I checked in the same set of changes to mimetpfl.cpp the same night I
checked in the plain text stuff. Let me know if you still think I missed a spot
in that file though.

Comment 25

[rhp (gone)]

This may explain why the mimeplfl.cpp patch is not working for me.

- rhp

Comment 26

[Ben Bucksch (:BenB)]

mscott, you're right. It was incredible stupidity on my side.
rhp is referring to a patch I made for bug #27911.

Comment 27

[Scott MacGregor]

triaging since M15 is tonight. Not an M15 stoppper.

Comment 28

[selmer (gone)]

Not M16 stopper.  Marking M17.

Comment 29

[scottputterman]

moving to M18 and nominating for beta3

Comment 30

[scottputterman]

Mail triage marking [nsbeta3+]

Comment 31

[David :Bienvenu]

adding dependencies on bugs on our i18n useage.

Comment 32

[Scott MacGregor]

I just checked in a rewrite of my email address XBL widget that makes it more
light weight and I changed a bunch of inefficient style rules in
msgHdrViewOverlay.css. Both of these changes improve message display a little
bit but bienvenu's working with caching the charset converters will make the
biggest difference this week. 

Comment 33

[lchiang]

Scott: do you know if David's changes made it in?

Comment 34

[Scott MacGregor]

It sure did!!!

Comment 35

[Phil Peterson]

PDT would like to wrap it up on this bug. Making P5; can we mark it fixed?

Comment 36

[lchiang]

second pass: - per mail triage. I'm sure that there are always more improvements 
that can be done, but this is good for first release.

Comment 37

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

I've created bug 63759 which is a performance tracker bug for mail/news.

Comment 38

[selmer (gone)]

nom catfood, when outliner lands this will stand out as one of the top areas to
address (along with reply/compose.)

Comment 39

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

Seth, Scott; isn't bug 13653 just really a DUP of this or do we want to keep it
seperate?  I realize it's a news issue, but shouldn't news and IMAP message
display performance be the same, since they both rely on pulling the message
from a server?

Comment 40

[scottputterman]

Getting this back on the radar for investigation in 0.9.4

Comment 41

[Cathleen]

mscott, are we going to be able to nail this one for 0.94?

Comment 42

[Scott MacGregor]

we won't be checking anything in for this in .9.4 but we are working on it. 

Comment 43

[Scott MacGregor]

triaging.

Comment 44

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

*** Bug 104636 has been marked as a duplicate of this bug. ***

Comment 45

[scottputterman]

this will continue to be worked on in 0.9.7

Comment 46

[Scott MacGregor]

Comment on attachment 5938 [details] [diff] [review]
reduce # allocations of each line of data parsed by these 2 routines

obsoleting. This patch was checked in a very long time ago.

Comment 47

[Scott MacGregor]

Attachment: Broadcast the headers all at once into the listening JS instead of calling out once per header

Comment 48

[Scott MacGregor]

According to quantify, this patch should help msg display a little bit. Instead
of calling out to JS and broadcasting the presence of each individual header, we
wrap them up into a set of arrays and make a single call. This also allowed us
to remove OnStartHeaders and OnEndHeaders (2 more calls out to JS).

Now libmime only calls into JS once if there aren't any attachments.

Comment 49

[Scott MacGregor]

Attachment: also incorporates Seth's patch in 104519 (selectively broadcast headers)

Comment 50

[Scott MacGregor]

Quantify results:
Before: 29.75 milliseconds were spent making calls out to JS (on start, on end
and handle header). After these optmiziations to not call out to JS so much and
to reduce the data we pass out to JS, we spend 27.42 milliseconds in JS.

small but it's going to be a battle of small wins =). 

Comment 51

[Jean-Francois Ducarroz]

+      headerNames[numHeadersAdded] = headerInfo->name;
should you use a strdup here?

Comment 52

[Scott MacGregor]

good question. Actually that particular array is an array of const char *'s
which are sent to the JS engine. So no, I don't want to strdup that string. The
lifetime of the string is owned by the html emitter class which outlasts the
lifetime of the call to the JS engine. 
The only reason the header value array isn't also an array of const strings is
because I had to convert those values from utf-8 to unicode. o.t. they also
would be const.

Comment 53

[Scott MacGregor]

Attachment: don't spend time converting us-ascii to utf-8 for plain text messages

Comment 54

[Scott MacGregor]

Here's an interesting one, at least for plain text messages, we were converting
every line of data into UTF-8 regardless of the initial character set. this
patch simply checks for an input charset of us-ascii and skips the charset
conversion step. 

Before:
MimeInlineText_convert_and_parse_line took 1.38 milliseconds over the course of
99 calls. 73% of that time was spent doing charset conversion when we didn't
need to. After this fix: MimeInlineText_convert_and_parse_line took .44
milliseconds over 99 calls with all of it going into actual mime parsing work.

Comment 55

[nhottanscp]

There are messages which have "us-ascii" labels but contains "ISO-8859-1" text.
They are not correct messages but I have seem bugs filed for those cases (e.g.
webmials).
There is a check in MIME_get_unicode_decoder to map "us-ascii" to "ISO-8859-1".
http://lxr.mozilla.org/seamonkey/source/mailnews/mime/src/comi18n.cpp#1677

So, before skipping the conversion, you should check if the data is really 7 bit
only (e.g. by nsCRT::IsAscii).

Comment 56

[Scott MacGregor]

Another interesting tidbit. We spend significant cycles parsing and using global
history in the message pane. We don't want global & session history in the
message pane. I just filed 108635 to track making the browser tag allow the
creator to disable session & global history. I'm posting a patch to that bug too. 

Comment 57

[Jean-Francois Ducarroz]

Comment on attachment 56624 [details] [diff] [review]
also incorporates Seth's patch in 104519 (selectively broadcast headers)

R=ducarroz

Comment 58

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

Comment on attachment 56624 [details] [diff] [review]
also incorporates Seth's patch in 104519 (selectively broadcast headers)

sr=sspitzer

Comment 59

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

*** Bug 104519 has been marked as a duplicate of this bug. ***

Comment 60

[nhottanscp]

Attachment: Added a check to make sure the data is really ASCII.

Comment 61

[Scott MacGregor]

removing some obsolete dependencies.

Comment 62

[Scott MacGregor]

*sigh* turns out my xul cache was disabled!!! So most of my quantify work this
week is bogus. I just marked a bunch of the bugs I've been investigating as
invalid since they all went away once I realized it was diabled. 

Comment 63

[Scott MacGregor]

the patch to selectively broadcast headers and to broadcast all the headers at
once is now checked into the trunk. 

Comment 64

[Scott MacGregor]

Comment on attachment 56624 [details] [diff] [review]
also incorporates Seth's patch in 104519 (selectively broadcast headers)

obsoleting since this patch is now checked into the trunk. In the future I'll
be tracking things like this in  separate bugs.

Comment 65

[Scott MacGregor]

Looking at the quantify logs for message display performance, I feel I've killed
off all of the low hanging fruit. The top 4 time sinks left are:
1) time spent in gecko parsing the html message body
2) time spent painting
3) time spent in js_interpret executing the JS in msgHdrViewOverlay.js
4) time spent doing frame construction

The only thing I really have control over is 3 and 4 so those are the next 2
areas I'm going to start focusing on.

Comment 66

[Scott MacGregor]

I did just notice a strange regression in performance for
CSSFrameConstructor::ConstructRootFrame over previous quantify runs. I just
filed bug #112026 to track this regression. 

Comment 67

[scottputterman]

continuing in 0.9.8

Comment 68

[scottputterman]

Individual bugs might get fixed, but heavy concentration on this will won't
occur for a while so moving out.

Comment 69

[Wayne Mery (:wsmwk)]

closing per mscott