21203 - mozTXTToHTMLConv is performance bottleneck

Status: VERIFIED INVALID

(MailNews Core :: MIME, defect, P2)

Description

[Scott MacGregor]

Simon first noticed this. Displaying messages has gotten noticebly slower on all
platforms recenently but particularly on the Mac where it is almost painful.

Simon did some investigating and noticed some big performance issues with the
mozTXTToHTMLConv stuff.

For starters, we are spending lots of time in
nsAutoString constructors. And
methods like SmilyHit are really expensive the
way they are written now.

In irc benB also mentioned that these methods are
getting called on characters
in the message as often as 12 times!

This bug is
tracking two things:
1) getting a preference for the txt to html converter in
place so we can turn
this off in the short term until the performance problems
are worked out. I
beleive shaver is running point on that.
2) Ben volunteered to
take a look at improving the performance of some of these
methods including
using char *'s in the method parameters instead of
nsAutoString and other memory
optimizations.

Adding rhp to the cc list since I think he helped review and get
this code in.
Maybe he has some ideas too.

Comment 1

[Ben Bucksch (:BenB)]

Note: We're talking about additional 1-2 seconds for a 300K msg here (source:
smfr)
I never said, I'll use char*. Note bug #18410. It also would be a headache, as
the string can inpredictably grow.
This problem was really caused by a hotfix. I'll look at it now.

Comment 2

[Ben Bucksch (:BenB)]

Attachment: Fix for iterating calls to SmilyHit

Comment 3

[Ben Bucksch (:BenB)]

This should eliminate the regression.

Comment 4

[leger]

Putting on PDT- radar.

Comment 5

[Ben Bucksch (:BenB)]

Forgot to mark fixed.

Comment 6

[Scott MacGregor]

Hey Ben, we can't mark it fixed until something is checked in. I'm going to
re-open this. We need to get PDT permission to take the fix.
When you mark something fixed QA, tries to verify it and if it isn't checked in
they get mad and beat us on the head....

Comment 7

[lchiang]

clearing resolution.
So, is your head sore, scott? :-)

Comment 8

[Simon Fraser [no longer active]]

We need this for dogfood. Opening a 7k plain text message takes 6 seconds on a
fast Mac, during which time the UI is totally unresponsive. Request that this
gets PDT+ status back.

Comment 9

[Ben Bucksch (:BenB)]

Why not just a=chofmann or so? There already *is* a fix...

Comment 10

[Ben Bucksch (:BenB)]

BTW: A better way than using |char*|s would be to rewrite the
|mozTXTToHTMLConv::Right|. If somebody wants to volunteer... :-)

Comment 11

[Simon Fraser [no longer active]]

Attachment: Diff for mozTXTToHTMLConv.cpp

Comment 12

[Simon Fraser [no longer active]]

Attachment: Diff for mozTXTToHTMLConv.h

Comment 13

[Simon Fraser [no longer active]]

I attach diffs that I believe are a better solution for the problem. I changed a
number of methods to take char*s instead of nsAutoStrings, so that the cost of
creating nsStrings is postponed until after a match is found. This makes the
display of text messages a lot faster

Comment 14

[Scott MacGregor]

Simon, thanks for taking care of this. Your changes look good to me if you'd
like to use me as the reviewer.

Comment 15

[Simon Fraser [no longer active]]

Hey, Ben, how do those changes look to you? Apologies for being rather curt on
IRC today (I was trying to do multiple things at once), and let me know if you
have any questions.

Comment 16

[Simon Fraser [no longer active]]

Actually, we need  little more here; this code is still showing up as a
bottleneck for large plain text messages. We're still constructing heap-based
strings in the call to mozTXTToHTMLConv::Right() from
mozTXTToHTMLConv::ItMatchesDelimited() [and maybe elsewhere].

Comment 17

[Simon Fraser [no longer active]]

mozTXTToHTMLConv::ScanTXT() and mozTXTToHTMLConv::ScanHTML() also contain the
following bug:
  return _retval ? NS_OK : NS_ERROR_OUT_OF_MEMORY;
should read
  return *_retval ? NS_OK : NS_ERROR_OUT_OF_MEMORY;

Comment 18

[rhp (gone)]

I have a fix for this problem. Chris, can I check this in?

Index: mozilla/mailnews/mime/emitters/src/nsMimeBaseEmitter.cpp
===================================================================
RCS file: /cvsroot/mozilla/mailnews/mime/emitters/src/nsMimeBaseEmitter.cpp,v
retrieving revision 1.6
diff -p -r1.6 nsMimeBaseEmitter.cpp
*** nsMimeBaseEmitter.cpp	1999/12/08 03:33:23	1.6
--- nsMimeBaseEmitter.cpp	1999/12/14 00:30:27
*************** nsMimeBaseEmitter::Complete()
*** 122,128 ****
    // to flush it...if we try and fail, we should probably return
    // an error!
    PRUint32      written;
!   if ( (mBufferMgr) && (mBufferMgr->GetSize() > 0))
      Write("", 0, &written);

    if (mOutListener)
--- 122,128 ----
    // to flush it...if we try and fail, we should probably return
    // an error!
    PRUint32      written;
!   while ( (mBufferMgr) && (mBufferMgr->GetSize() > 0))
      Write("", 0, &written);

    if (mOutListener)

Comment 19

[Ben Bucksch (:BenB)]

> Hey, Ben, how do those changes look to you?

From a social or technical standpoint?
Technically: Performance is OK with my patch. Your patch should be in the
microseconds area for normal messages.
Socially: I expressed earlier, that I don't want |char*|s in that class.

You're right with the |*_retval|. Thanks.

Comment 20

[Simon Fraser [no longer active]]

I don't want go get into a language war here, but use of ns[Auto]String is hiding
lots of allocations and copying that would have to be very explicit if you used
char* (or PRUnichar*).

I think this code needs to satisfy two objectives:
1. In the event that no emoticon is found, the code should do *no* additional
   heap alloctions other than that used to copy the string in ScanTXT/ScanHTML.
2. The code should be O(n), not O(n^2) as it is now.

The basic loop in mozTXTToHTMLConv::Scan[HTML,TXT] is

  for(PRUint32 i = 0; PRInt32(i) < text.Length();)
  {
     if (GlyphHit(Right(text, i), i == 0, glyphHTML, glyphTextLen))
     {
     }
   }

Right() returns an nsAutoString (which will involve allocation for almost all
iterations). So for *each* char in the msg, we're allocating a new block n-i big,
and copying into it, even if no smilies are matched. Down in
ItMatchesDelimited(), an additional call to Right() will also involve allocation.

So you need to avoid additional allocations resulting from calling Right(). I
suggest that you need to pass around offsets into 'text', so that you avoid
allocation and copying by always looking at the original string using an offset.

Comment 21

[Ben Bucksch (:BenB)]

See bug #21649 for mscott's "1)" (prefs).

Comment 22

[Ben Bucksch (:BenB)]

smfr,
Yes, Right is allocating a new string. That's one of the two reasons, why I
created it: It's easy to tune that way.

Note, that the functions are *not* "O(n^2)" in the way, that time increases
exponentially with the number of lines in the msg.

> I suggest that you need to pass around offsets into 'text'

I don't think, you can suggest my needs.
Passing offsets here that would mean changing half of the class.

Comment 23

[rhp (gone)]

I'm working with Ben to get some runtime numbers and review some of the issues
here. We'll get it done.

- rhp

Comment 24

[rhp (gone)]

I have changes in hand for this. I would like to get these into the tree. Here
is a full explanation of the updates:


Hello all,
Ben and I spent some time looking into the performance of the URL/emoticon
recognition code last night and today. In general, what is
running in the tree  right now is pretty slow and mostly resulted from a
temporary fix that was not as efficient as some new code we
have now. Last night, I updated the TXT to HTML stream converter to include
timing information so we can have some real numbers
when looking at performance.

Basically, there were 3 areas that have been changed to help performance. These
include:

      libmime creation of stream converter
      libmime was going to the factory and creating a stream converter for
every line of the file. Obviously inefficient. I've changed this
      to only do it once per message.
      BenB's Improved logic for GlyphHit()
      Ben did some nice optimizations for this code and reduced the subsequent
calls to SmilyHit() significantly.
      Simon's optimizations for Length() method calls
      Simon put together a bunch of changes to reduce the calls to the Length()
method for ns*String strings.

So, I'm sure you want to see the numbers. Well, here they are:

Performance results of message display
System: P2 - 366Mhz - 196MB RAM
All results on DEBUG builds.
All numbers are the totals for the entire message display.

31KB Plain Text Message
WITH THE CURRENT TREE CONTENTS:
mozTXTToHTMLConv::ScanTXT(): Real time 0:0:9, CP time 8.152
mozTXTToHTMLConv::GlyphHit(): Real time 0:0:7, CP time 7.371
mozTXTToHTMLConv::Right(): Real time 0:0:0, CP time 0.601

AFTER rhp's & BenB's OPTIMIZATIONS:
mozTXTToHTMLConv::ScanTXT(): Real time 0:0:1, CP time 1.722
mozTXTToHTMLConv::GlyphHit(): Real time 0:0:0, CP time 0.681
mozTXTToHTMLConv::Right(): Real time 0:0:0, CP time 0.451

AFTER Simon's OPTIMIZATIONS:
mozTXTToHTMLConv::ScanTXT(): Real time 0:0:1, CP time 1.572
mozTXTToHTMLConv::GlyphHit(): Real time 0:0:0, CP time 0.551
mozTXTToHTMLConv::Right(): Real time 0:0:0, CP time 0.441

73KB Plain Text Message after all optimizations
NOTE: This email contained 5,420 colons

mozTXTToHTMLConv::ScanTXT(): Real time 0:0:4, CP time 3.926
mozTXTToHTMLConv::GlyphHit(): Real time 0:0:1, CP time 1.402
mozTXTToHTMLConv::Right(): Real time 0:0:1, CP time 0.911

Comment 25

[Simon Fraser [no longer active]]

Please get timing data for a version of ScanTXT that just returns a copy of the
string with no matching.

Comment 26

[leger]

Putting on PDT+ radar.

Comment 27

[rhp (gone)]

Organizing my fix for checkin.

- rhp

Comment 28

[rhp (gone)]

Changes should be checked in.

- rhp

Comment 29

[Simon Fraser [no longer active]]

Some performance numbers for Mac, displaying a 24k plain text message. To get
baseline data for comparison, I changed ScanTXT to just duplicate the string
and return (the data called CloneString below). The ScanTXT data are for the
version of ScanTXT that rhp just checked in.

                # calls     Ave time    Total time
CloneString       359          0.161         58 ms
ScanTXT           359          3.132       1125 ms

So there is still serious performance degradation, of the order of over a second
for this message, due to the emoticon code.

Comment 30

[Ben Bucksch (:BenB)]

Simon,
ScanTXT does a lot more than glyph recognition, e.g. URL recognition. But the
former seems to need 2/3 of the time.

Comment 31

[Ben Bucksch (:BenB)]

(I forgot: ScanTXT also does basic escaping.)

I have some optimatimations in my local tree, including a rewrite of Right(),
but I can't get the nsTimer to work on Linux.

Comment 32

[lchiang]

suresh - can you try to verify this? Thanks :-)

Comment 33

[Suresh]

Message display performance has improved a lot. 
Marking as verified.

Comment 34

[rhp (gone)]

I believe we are in a better place. :-)

- rhp

Comment 35

[Scott MacGregor]

Suresh, when you marked this bug as verified it sounds like you were talking
about the time to display messages which did improve a lot. But this code is
called when you send an outgoing messages.

Waterson just sent out email saying he replied to a message with a patch that
had a couple hundred lines in it. It took over 5 minutes to send this message on
a high end win32 machine. Repeated breakins in the debugger should that he was
always in mozTXTToHTMLConv.cpp

Looks like we need to look into this again.

Comment 36

[Scott MacGregor]

re-opening per waterson's posting to the mailnews group.

Comment 37

[Chris Waterson]

Ok, so these routines that *return* nsAutoString objects have -got- to go. 
Replace them with routines that PASS IN an |nsString&|.

http://lxr.mozilla.org/seamonkey/source/netwerk/streamconv/converters/mozTXTToH
TMLConv.cpp#40

http://lxr.mozilla.org/seamonkey/source/netwerk/streamconv/converters/mozTXTToH
TMLConv.cpp#48

http://lxr.mozilla.org/seamonkey/source/netwerk/streamconv/converters/mozTXTToH
TMLConv.cpp#64

http://lxr.mozilla.org/seamonkey/source/netwerk/streamconv/converters/mozTXTToH
TMLConv.cpp#112

These routines are causing the compiler to do evil and contorted things:

1. Before calling your routine, the compiler will generate a "temporary" 
nsAutoString to receive the results of the computation, "a".

2. Now we call the function. The compiler will create an nsAutoString "b" on 
the stack in the body of the function.

2. In most of these functions, we copy the string you want to munge into "b". 
(Note that, if this string happens to be larger than 64 bytes or so, 
nsAutoString will have to go allocate a buffer from the heap.)

3. We then munge "b". Again, if the muging causes the string's length to exceed 
the 64 byte limit, it faults, and allocates some space from the heap.

4. We now re-assign this result back to "a". If "b" is larger than 64 bytes, 
"b" will have to allocate a buffer on the heap to store the result.

So, there appears to be a *lot* of work that goes on in tight loops copying 
characters around. When I broke in the debugger, it looked like the strings 
that were being worked on were pretty darn long, and so this is probably 
causing the heap to just get slammed.

Like I said in my mail, this needs to be whacked *hard* with the hurry-up stick 
if we're really gonna do this for every single message that we read and send. 
It is rare that something is so slow that you can actually catch it in the 
debugger, so I think it's a no-brainer that this needs to be optimized. 
Nevertheless...

mscott: you basically own message display performance. we should collect hard 
numbers on how this affects things with quantify.

Comment 38

[rhp (gone)]

This is all Ben's. Not sure why this got assigned to me.

- rhp

Comment 39

[Simon Fraser [no longer active]]

I'm with waterson on this one. This is exactly what I said earlier; that Right() 
and friends, which return an nsAutoString, are evil.

Comment 40

[Ben Bucksch (:BenB)]

> ------- Additional Comments From [e-mail]  1999-12-27 00:11 -------
> I have some optimatimations in my local tree, including a rewrite of Right(),
> but I can't get the nsTimer to work on Linux.

Of somebody can help me with this, I can test my patch.

the *escape* functions are no problem, as they are called rarely (for this very
reason).

Waterson,
are you sure, it's the return of nsAutoString, that's the problem? Considerng
the huge difference between reading (plain text) mails and sending (HTML) mails,
SendHTML *could* be the reason.
However, assuming correct HTML is passed into it, I don't know, how.

Comment 41

[Chris Waterson]

I've got the email that I tried to reply to: I'll experiment a bit more and 
post some results in the bug. At this point, my only data points were that I 
broke five or six times into the debugger during the several minutes it took to 
send the message, and each time I was down in the bowels of the text conversion 
method.

Comment 42

[Scott MacGregor]

Something funky is happening that is suddenly exposing this. I just sent a
message in the debugger (which I've done many times before) which had a patch
that was not even a hundred lines long and it took me 2 minutes to send.
Breaking in the debugger showed me in the same area of code.

I wonder if something in compose changed to trigger this code getting called
recently.

Comment 43

[Phil Peterson]

Guys, let's measure this, rather than guess. Suresh, can you help us with a
Quantify run?

Comment 44

[Ben Bucksch (:BenB)]

I have problems to reproduce this. I used M13 Linux, my debug build from 1-2
weeks ago and (after an update) my debug build from today. All tests show more
or less the same:

Pasting normal text (~1000 lines, no <>&, but some URLs) into the Composer needs
~4 mins, during which mozTXTToHTMLConv isn't used, but Mozilla freezes. Pasting
the full mozTXTToHTMLConv.cpp source appearently freezes Mozilla completely (no
reaction after 15 mins). Pasting in today's build doesn't work at all. Replying
to a msg containing the mozTXTToHTMLConv.cpp source works and the reply action
itself doesn't invoke mozTXTToHTMLConv.

Sending, during which mozTXTToHTMLConv is used, in all cases doesn't need much
longer than displaying the same text.

Some interesting exceptions:
- The mozTXTToHTMLConv.cpp source contains some HTML tags in comments. The
compose window after a reply to the msg containing the source looks like, as if
the tags were evaluated. Maybe they are never escaped?
- Pasting the first 200 lines of the source worked (with the old builds); the
tags come after them. Maybe they are also the cause for the freeze.

Conclusion:
ScanTXT and ScanHTML make assumptions about the content of their parameters.
ScanTXT wants not escaped, pure unicode, while ScanHTML assumes correct HTML
with already escaped <>&, but all other chars in unicode. If these requirements
are not met, I'm not surprised, that something goes wrong. Of course, this is
just a guess.

BTW: if you define DEBUG_BenB, you should see input (=extrance) and output
(=exit) of ScanTXT and ScanHTML on the console, which of course slows things
down a lot, but helps to diagnose.

Comment 45

[Simon Fraser [no longer active]]

Don't get confused by the performance of pasting text in composer. That will 
change dramatically once the async reflow stuff is turned on by default. You can 
turn that on in your build now with the pref:
user_pref("layout.reflow.async", true);

Comment 46

[Suresh]

Ok. I have posted the Quantify data (both in text and in html format) at 
http://jazz/users/suresh/publish/bug21203/. This data is recorded from Clicking 
on the Send until the reply window goes away. And the original msg is ~100KB.

Comment 47

[Ben Bucksch (:BenB)]

This machine is behind the firewall.

For the case, this reveals the bug (i.e. takes a long time):
- I don't know quantify, but is it easily possible to measure the time spent in
the mozTXTToHTMLConv class only?
- Can you tell me the steps to reproduce (including the msg)?

Comment 48

[Scott MacGregor]

Hey Ben, I'll run some analysis on the results and will get back to you with
what I find.

Comment 49

[Scott MacGregor]

Early returns from Quantify. According to the data, 99.6% of the time it takes
from the time msgCompose::SendMsg is initiated to the time we send the message
to the server, is spent in trying to get the body from editor. And 99.68% of
this time is spent in ScanHTML.

I'm still looking at the leaves underneath scanHML but most of the time
underneath scanHTML is spent in nsAutoString related stuff.

Ben, if you don't mind, I'd like permission to play around with scanHTML and
auto string useage to see if I can bring the numbers down. It might be easier
for me to do it since I can get Suresh (or myself) to quantify the result and I
know you don't have access to quantify. Is this okay with you? Thx

Comment 50

[Ben Bucksch (:BenB)]

mscott,
tnx for the info and the offer. However, I'd prefer to look at ScanHTML by
myself, if this function is really the problem (and not bogus input).

But first, I need to reproduce the bug. Can somebody please tell me how to do
that?

Comment 51

[Scott MacGregor]

Ben, any easy test case to see this is by sending a large text message. Once
consistant one for me is to go to
netscape.public.mozilla.performance.size-matters and reply to one of the 100KB+
messages in that newsgroup. (change the newsgroup so it gets sent to you or
something first). Then hit send.

You'll see that we freeze for a good 5 to 10 minutes on a high end machine.
According to the quantify data I'm looking at. Over 98% of that time is spent in
nsAutoString related methods that live in leaves underneath ScanHTML.

Comment 52

[Ben Bucksch (:BenB)]

Yes, I can see the bug now, tnx. The input looks OK. Seems to me like most of
the time is really spent in ScanHTML (or UnescapeStr).
Starting to debug.
Again, tnx for your offer, I might come back to it, if I can't track it down.

Comment 53

[Ben Bucksch (:BenB)]

Removing old DOGFOOD.

I think, I know the problem. Starting to hack.

Comment 54

[Ben Bucksch (:BenB)]

The problem was my assumption, that only lines or small paragraphs will be
passed to ScanTXT, but the whole quote was processed at once.

My rewrite of Right fixed the problem. I just had to apply and test it.

Sending from hitting Send until completion now needs < 15s on a Linux debug
build from today.

Patches follow. I'm very tired now, so no garanties about the quality. If
somebody wants to review nevertheless, please read and follow
<file:/home/ben/cvs/Bucksch/projects/mozilla/review.html>.

Comment 55

[Ben Bucksch (:BenB)]

Sorry, wrong URL. That's the right one:
<http://www.bucksch.org/1/projects/mozilla/review.html>

Comment 56

[Ben Bucksch (:BenB)]

Attachment: Rewrite of |mozTXTToHTMLConv::Right|

Comment 57

[Ben Bucksch (:BenB)]

Attachment: Updated comments on IDL file

Comment 58

[Ben Bucksch (:BenB)]

I did some more tests and I don't see any bugs (in my code).

Comment 59

[Ben Bucksch (:BenB)]

Could somebody review the patch and check it in?

Filed bug #26915 "Improve performance of TXT->HTML converter".

Comment 60

[Phil Peterson]

I'd like rhp and/or mscott to review.

Comment 61

[Ben Bucksch (:BenB)]

There's a bug, which can never actually appear in practice. Fixed in following
patch.

Comment 62

[Ben Bucksch (:BenB)]

Attachment: Rewrite of |mozTXTToHTMLConv::Right|, 2. version

Comment 63

[Scott MacGregor]

Hey Ben, I'm not sure if subclassing nsAutoString is the right way to go here.
If we do need to add another method to nsString (not nsAutoString) to implement
Right then we should try to talk to rickg to get that method added.

But this still doesn't help with the problem that we do lots of extra string
copying by using nsAutoString as the out parameter for so many of the methods in
here. I think that's why it keeps showing up as such a hot spot on all of our
quantify logs.

See waterson's excellent description of why this is so expensive up above in
this report.

It'd be really cool if we could have just two arguments that come into
scanText/scanHTML: nsString& originalString, nsString& converteredString and
pass these around everywhere (or const char * ptrs into these buffers were
necessary).

Ideally, we would need any extra duplicate allocations of any of the strings,
we'd just be taking characters from originalString, applying transformations and
appending the results to convertedString.

Comment 64

[Ben Bucksch (:BenB)]

>But this still doesn't help with the problem that we do lots of extra string
>copying by using nsAutoString as the out parameter for so many of the methods
>in here.
Did you also check, who calls them?
- Right was the reason for the bug and has been rewritten
- The escaping functions are only called at "<", ">" and "&".
- CompleteAbbreviatedURL is a subroutine of FindURL and only called, when it
  thinks, an URL may have been found.

IIRC, all string allocations now hold only small substrings, i.e. no heap hits
(apart from input and output strings).

Comment 65

[Scott MacGregor]

Hey Ben, yeah I did check to see who calls them. And the auto string allocations
are still a problem even with your fix for scanHTML using .Right. I give an
example below.

I'm looking at another set of quantify logs for just displaying messages which
invokes scanTxt not scanHTML. Just measuring the time spent inside the mime
converter, we spend 30% of the time converting the data into UTF-8. We spend
over 68% of the time inside of scanTxt. Searching through the leaf nodes of the
quantify data shows most of that data again being spent in auto string copy and
construction routines.

I guess I keep coming back to the suggestion of passing an inString and an
outString (by reference) through all these methods instead of returning
nsAutoStrings. We could pull off all these transformations by only copying the
string at most one time into the outString.

Hmmmm.

Comment 66

[Ben Bucksch (:BenB)]

> And the auto string allocations are still a problem even with your fix for
> scanHTML using .Right

This is topic of bug #26915, not this bug. This bug is about "Send" needing 4
mins, and this is fixed.

> I'm looking at another set of quantify logs for just displaying messages
[...]
> again being spent in auto string copy and construction routines.

Did you use the patch? For the tree version, this is no surprise.

Comment 67

[Scott MacGregor]

Ben, I think you have your bugs mixed up. Bug #26915 specifically mentions send
time still taking too long not this one.

I'm using this bug to just track some generic performance bottlenecks that we
have right now running through moxTXTToHTMLConv.

I think my comments about scanTxt probably belong here and not in that bug but
let me know if you feel otherwise. In any case, removing the auto string as
return values and passing in an out string by reference would make a huge
difference here from the quantify numbers I'm seeing as I mentioned before.

Comment 68

[Ben Bucksch (:BenB)]

> Bug #26915 specifically mentions send time still taking too long not this one.

I don't understand this sentence.

> I'm using this bug to just track some generic performance bottlenecks
[...]
> I think my comments [...] probably belong here and not in that bug but
> let me know if you feel otherwise.

I do think, they belong to 26915, in order not to mix up the real bug here (Send
needing 4 mins) with general performance optimizations.

> removing the auto string as return values [...] would make a huge
> difference here from the quantify numbers I'm seeing

I used jprof, and I did not see a large portion of the time used by these
allocations. I may try removeing them, but I need hard numbers for that. As long
as bug #27335 isn't resolved, I can't work on that (bug #26915).


Is there any specific reason, why the patch shouldn't be checked in? Closure for
M14 is soon.

Comment 69

[Scott MacGregor]

I meant that the other bug specifically talks about send mail taking too long
this bug is a generic performance bottleneck bug. In your own words in the
description of Bug #2619 send mail is taking too long.

I guess I have been viewing this bug as a generic mozTXTToHtmlConv is a
performance bottleneck with its use of nsAutoString. where 26219 seems to
specificaly deal with just sending messages and scanHTML.

Although your right the proposed patch for this bug fixes the scanHTML problem
on sending a message.

I'm also now talking about scanTxt which is showing up as a hot spot in the
quantify data I'm looking at . I guess you want your own proof of this data but
I don't know if we can wait for Bug  #27355 to get fixed in order to show what
quantify is showing me now. I suppose I could try to send you the quantify log
but it is over 10MBs. It would show you the cost of scanTxt relative to the rest
of the time spent in mime.

I thought I already put some comments in here about the patch using .Right but
maybe I didn't. I really don't think subclassing nsAutoString is the right way
to solve this problem. If we really need to do it this way, then we should talk
to rickg about getting a .Right method added to nsString instead of subclassing
nsAutoString ourselves.

Comment 70

[Ben Bucksch (:BenB)]

> longwhere 26219 seems to specificaly deal with just sending messages and
> scanHTML.

From 26915 description:
> Reading a 100K plain text and sending a 100K HTML msg currently roughtly takes
> 10s on an optimized Linux build.

> It would show you the cost of scanTxt relative to the rest
> of the time spent in mime.

I already have this info from jprof. As my goal is not a percentage of mime
processing time, but an absolute time or at most a percentage of 4.x processing
time, I need to measure, how much *time* exactly is spent in the converter. I
need "exact" numbers in order to sort out various alternatives.

> I really don't think subclassing nsAutoString is the right way
> to solve this problem.

Can you explain why?
1. What is wrong with subclassing?
2. I don't think, this usage of Right is of such general nature, that it would
deserve a function (a constructor, to be precise) in nsString. But if it is,
that's fine with me, as long as it makes it in M14.

Comment 71

[Ben Bucksch (:BenB)]

So, what's up now? The tree closes in some hours and I'd like Beta1 not to ship
with this bug, especially as there's a fix.

Comment 72

[Phil Peterson]

I think any beta-stopper performance bottlenecks in message display can be
checked in with bug 22960 (assigned to mscott). Sound ok to you Scott?

Comment 73

[Scott MacGregor]

Hey Ben, I actually needed to make some more significant changes to this code. I
started with your patch and then made changes on top of your patch to get rid of
all the nsAutoString return values. I also changed many of the methods to take a
ptr into the original string buffer along with a length parameter instead of
passing an input nsString to the methods.

This allowed us to get rid of methods like Middle and it reduced the amount of
memory allocated because we didn't need to allocate pieces of the input string
into a new string in order to call several of the routines.

Old methods would have looked like:
nsAutoString Foo (nsString& inString);
now look like
void Foo (const PRUnichar * aInputString, PRInt32 aLengthOfInString, nsString&
aOuputString).

aOutputString is allocated a large chunk of memory at the beginning of
ScanHTML/ScanTxt so we don't need to allocate smaller chunks as we go. This
single string is passed around through the various methods and transformed
characers are always appended to the output string. I also changed methods like
EscapeStr/UnescapeStr to perform the operation in place on the string instead of
allocating a new string for the return value.

These patches help resolve this bug, Bug #26915 which is the PDT+ version where
ScanTXt is slow when sending a large message. My PDT+ bug for display
performance: Bug #22960.

With both our changes, running ScanTXT through on a 140KB message took well
under a second on a debug build (PII, 450, 256MB) where it used to take on the
order of minutes.

I had waterson review these changes early today and I was hoping you might be
able to take a look at them too if you have time! I'd like to check this in
tomorrow and close out these bugs.

I'll attach the patch.

Comment 74

[Scott MacGregor]

Attachment: Ben's changes + my own

Comment 75

[Scott MacGregor]

Before and After quantify snap shot after both our performance improvements just
showing the top level ScanTxt routine:
ScanTXT call before:
mozTXTToHTMLConv::ScanTXT(nsAutoString const&,UINT)
  Calls
    nsAutoString::~nsAutoString(void)
      call count      126966
      propogated time 12821889
      % of caller           0.01

    nsAutoString::nsAutoString(nsAutoString const&)
      call count      1582
      propogated time 4378736
      % of caller           0.00

    nsCString::Append(WORD)
      call count      122771
      propogated time 53364308
      % of caller           0.05

    nsCString::CharAt(UINT)const
      call count      477587
      propogated time 4298281
      % of caller           0.00

    nsAutoString::nsAutoString(void)
      call count      125926
      propogated time 7681486
      % of caller           0.01

    mozTXTToHTMLConv::FindURL(nsAutoString const&,UINT,UINT,nsAutoString&,int&,int&)
      call count      1573
      propogated time 58167529
      % of caller           0.06

    mozTXTToHTMLConv::StructPhraseHit(nsAutoString const&,int,char const*,char
const*,char const*,nsAutoString&,UINT&)
      call count      1040
      propogated time 102597915960
      % of caller          99.46

    Right
      call count      1040
      propogated time 425445821
      % of caller           0.41

ScanTxt call AFTER


ozTXTToHTMLConv::ScanTXT(WORD const*,int,UINT,nsString&)
  Calls
    nsAutoString::~nsAutoString(void)
      call count      3662
      propogated time 113522
      % of caller           0.05

    nsCString::Append(WORD)
      call count      287095
      propogated time 122182963
      % of caller          53.49

    mozTXTToHTMLConv::GlyphHit(WORD const*,int,int,nsString&,int&)
      call count      144144
      propogated time 18308739
      % of caller           8.02

    nsAutoString::nsAutoString(void)
      call count      3662
      propogated time 223382
      % of caller           0.10

    mozTXTToHTMLConv::FindURL(WORD const*,int,UINT,UINT,nsAutoString&,int&,int&)
      call count      2407
      propogated time 102614758
      % of caller          44.93

    mozTXTToHTMLConv::StructPhraseHit(WORD const*,int,int,char const*,char
const*,char const*,nsString&,UINT&)
      call count      16208
      propogated time 25811728
      % of caller          11.30

Comment 76

[Ben Bucksch (:BenB)]

I don't like this bug, because it is basically a "This is slow and I think, it
could be faster" bug. I can't work on such a "bug". I tried to make this bug
into the bug with send needing 4 mins, but I obviously failed. Because of that,
I close this bug as INVALID and opened bug #29773 for the send pref bug.

Comment 77

[Ben Bucksch (:BenB)]

mscott,
I said more than one time, that I don't want string pointers in this class. The
reasons:
- I value stability much more than performance. A send operation, that is
slightly faster, doesn't help me with lost half-written msgs or bugzilla posts
due to crashes. My version of ScanTXT/-HTML virtually *can't* crash, because it
simply doesn't use any pointer (ignoring the few pointers Simon introduced). And
it only needs ~25ms for a normal (2,5K) msg, 1s for a normal 100K msg (without
your changes). I just don't think, some ms are worth the risk.
- The danger of crashes due to pointers are especially high in this case,
because I don't know pointers very well. My experience is, that reviewers often
also don't find these bugs.
- Generally, string classes are more expressive and produce more readable code.

The class with your patch applied is unmaintainable for me. So, as the practical
owner of the class, I don't accept the patch. If somebody with a cvs account or
a module owner wants to override me, you'll have to find a new owner. Some of
the work, that is waiting for the new owner:
- Lots of bug reports (many enhancements and invalid ones)
- Transformation in a real stream converter (I'm currently talking with Jud
about that)
- Bug #23327 - Support custom (user specified) glyph substitutions

Additionally, your patch seems to be *slower* in some cases (but faster in
others). See bug #26915 for numbers (previous numbers are also there).

I suggest, my patch is checked in, and I'll look at your patch for ideas, when
we /need/ further perf improvement.

> aOutputString is allocated a large chunk of memory at the beginning of
> ScanHTML/ScanTxt so we don't need to allocate smaller chunks as we go

Yes, but this was *my* change. See
<http://bugzilla.mozilla.org/showattachment.cgi?attach_id=5308>.

I also wonder, why you didn't check in my patches (which are certainly an
improvement) for M14. They are waiting for several *weeks* now.

Sorry for sounding emotional, but this pisses me off.

Comment 78

[Ben Bucksch (:BenB)]

Did somebody disable ScanTXT for plain text quotes in HTML msgs as a "hotfix"
for this bug?
If yes, please add a comment to bug #27991 "plaintext quotation is inserted as
html source", which screws plain text quotes with a "<" completely up and is
likely to be cause by such a change.

Comment 79

[Ben Bucksch (:BenB)]

Forget my last comment, this bug existed before this one (although I though, I
didn't see it during testing for this one). Sorry.

Comment 80

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

Ben marked this INVALID.  VERIFIED.