Closed Bug 22960 Opened 25 years ago Closed 17 years ago

Improve message display performance

Categories

(MailNews Core :: Backend, defect, P1)

Product:
MailNews Core
Component:
Backend
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla1.2alpha

People

(Reporter: mscott, Assigned: mscott)

References

(Depends on 1 open bug)

Details

(Keywords: perf)

Attachments

(2 files, 3 obsolete files)

don't spend time converting us-ascii to utf-8 for plain text messages
931 bytes, patch
Details | Diff | Splinter Review
Added a check to make sure the data is really ASCII.
3.82 KB, patch
Details | Diff | Splinter Review 
I need a tracking bug to land the changes for message display. I'm just filing
this bug as a place holder.
Status: NEW → ASSIGNED
Priority: P3 → P2
Target Milestone: M13
Blocks: 22215
Status: ASSIGNED → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED
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.
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
Status: RESOLVED → REOPENED
Keywords: beta1, perf
Resolution: FIXED → ---
Summary: [PERFORMANCE] Improve message display performance → Improve message display performance
We'll start doing some quantify analysis to figure out where the hot spots are
(mime, layout, protocol) etc. More info soon.
Status: REOPENED → ASSIGNED
Target Milestone: M13 → M14
Putting on PDT+ radar for beta1 until we get down to 2x.
Whiteboard: [PDT+]
Display perf has regressed according to suresh's data. P1 for M14.
Priority: P2 → P1
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.
Whiteboard: [PDT+] → [PDT+] expected fix: post 2/15 for it to meet marketing target
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
QA Contact: lchiang → suresh
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.

mozTXTToHTMLConv perf improvement is bug #26915.
Depends on: 26915
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.

I
have
some
ideas
butdon't
know
if
they
will
be
enough.
Whiteboard: [PDT+] expected fix: post 2/15 for it to meet marketing target → [PDT+] UNKNOWN
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.


> 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?
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.

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?
Yeah, we would only need to run the display message cases and not the whole
suite of timings. The numbers are looking good!

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            
Wow!
Whiteboard: [PDT+] UNKNOWN → [PDT+] Proposed fixes in review now
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

(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.
Whiteboard: [PDT+] Proposed fixes in review now → [PDT+] will be done by 3/3
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.
Keywords: beta1
Whiteboard: [PDT+] will be done by 3/3
Target Milestone: M14 → M15
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.
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.

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

- rhp
mscott, you're right. It was incredible stupidity on my side.
rhp is referring to a patch I made for bug #27911.
triaging since M15 is tonight. Not an M15 stoppper.
Target Milestone: M15 → M16
Not M16 stopper.  Marking M17.
Target Milestone: M16 → M17
moving to M18 and nominating for beta3
Keywords: nsbeta3
Target Milestone: M17 → M18
Mail triage marking [nsbeta3+]
Whiteboard: [nsbeta3+]
adding dependencies on bugs on our i18n useage.
Depends on: 47542, 48496
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. 

Scott: do you know if David's changes made it in?
It sure did!!!
PDT would like to wrap it up on this bug. Making P5; can we mark it fixed?
Priority: P1 → P5
Whiteboard: [nsbeta3+] → [nsbeta3+][PDTP5]
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.
Whiteboard: [nsbeta3+][PDTP5] → [nsbeta3-][cut 8/31][PDTP5]
QA Contact: suresh → stephend
I've created bug 63759 which is a performance tracker bug for mail/news.
Target Milestone: M18 → ---
nom catfood, when outliner lands this will stand out as one of the top areas to
address (along with reply/compose.)
Keywords: nsbeta3 → nsCatFood
Whiteboard: [nsbeta3-][cut 8/31][PDTP5]
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?
Getting this back on the radar for investigation in 0.9.4
Priority: P5 → P1
Target Milestone: --- → mozilla0.9.4
mscott, are we going to be able to nail this one for 0.94?
we won't be checking anything in for this in .9.4 but we are working on it.
Target Milestone: mozilla0.9.4 → mozilla0.9.5
triaging.
Target Milestone: mozilla0.9.5 → mozilla0.9.6
*** Bug 104636 has been marked as a duplicate of this bug. ***
this will continue to be worked on in 0.9.7
Target Milestone: mozilla0.9.6 → mozilla0.9.7
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.
Attachment #5938 - Attachment is obsolete: true
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.
Attachment #56620 - Attachment is obsolete: true
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 =). 
+      headerNames[numHeadersAdded] = headerInfo->name;
should you use a strdup here?

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.
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.
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).

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.
Depends on: 108635
Comment on attachment 56624 [details] [diff] [review]
also incorporates Seth's patch in 104519 (selectively broadcast headers)

R=ducarroz
Attachment #56624 - Flags: review+
Comment on attachment 56624 [details] [diff] [review]
also incorporates Seth's patch in 104519 (selectively broadcast headers)

sr=sspitzer
Attachment #56624 - Flags: superreview+
*** Bug 104519 has been marked as a duplicate of this bug. ***
Depends on: 108761
Keywords: nsbeta1+
Depends on: 108960
Depends on: 108961
Depends on: 108963
removing some obsolete dependencies.
No longer depends on: 26915, 47542, 48496
Depends on: 108965
Depends on: 109107
Depends on: 109134
*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. 
the patch to selectively broadcast headers and to broadcast all the headers at
once is now checked into the trunk. 
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.
Attachment #56624 - Attachment is obsolete: true
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.
Depends on: 109929
Depends on: 109931
Depends on: 109979
Depends on: 64764
Depends on: 110905
Depends on: 112026
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.
Depends on: 112061
Depends on: 112220
OS: other → All
Hardware: PC → All
Depends on: 113334
Depends on: 113384
Depends on: 113528
continuing in 0.9.8
Target Milestone: mozilla0.9.7 → mozilla0.9.8
Depends on: 114532
Individual bugs might get fixed, but heavy concentration on this will won't
occur for a while so moving out.
Target Milestone: mozilla0.9.8 → mozilla1.2
Blocks: 122274
Keywords: nsbeta1+ → nsbeta1-
Product: MailNews → Core
closing per mscott
Status: ASSIGNED → RESOLVED
Closed: 25 years ago → 17 years ago
Resolution: --- → FIXED
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: