This bug fell out of some of my analysis work for Bug #28783. Background: I tried replying to one of the 130K news articles that are currently in netscape.public.mozilla.performance.size-matters. I found that it took over 4 minutes before the news body was inserted into the editor instance of the compose window. This was on a PII-450MHZ machine with 256 MB of ram. So a pretty beefy machine =). Bug #28783 tracks the first bottleneck in the operation which was nsTextEditRules::WillInsertText. This routine was taking about 3.5 of the 4 minutes. Of the reamining 30 seconds, most of that time was spent inside of nsHTMLToTXTSinkStream::Write. This method is getting passed a nsString that is 130K in size. The sits in a tight loop extracting a single word at a time (space is one of the delimeters it uses), copying that to a new nsstring and eventually writing that string out to a new output string. A couple things to look at: 1) The following call: aString.FindCharInSet(" \t\n\r", bol); on aString is expensive becaue the string is 130K. I'm not sure what we can do here 2) reading out one word at a time and copying that to a new string is hurting a lot too. Here's where we can try to get the most win. If we really need to read only one word at a time out, we should try to pass ptrs into the original buffer instead of copying the word into a new string. We can then pass a ptr into teh buffer (with a length field) into AddToLine I've had an email thread going with Daniel and he said he'd be willing to help take a look at this (thanks Daniel!!). And I'm willing to help too.

adding perf and beta1 keywords. Nominating for beta1. I've mentioned in the report above the amount of time we spend in here that we'd gain by optimizing the code.

1) We could limit the look ahead. The main reason for the search is to remove multiple whitespace. This could probably be done in a more effective way. 2) To pass references to a part of a string seems to be the right way, but it will spread to a lot of the code, including the charset converter. I don't know if we will have to make a copy there anyway.

1) The nsString::CompressSet function seems to do the right thing. It should be faster (I hope) but will probably need some thinking to do it right.

gh.

Drat, bugzilla dropped my last round of comments on the floor!! Let's try this again. I'm attaching a patch which removes the extra string copying for each string we extract from aString in the Write method. Now we pass a ptr into aString's buffer and a length field into AddToLine instead of a new nsString. Note: this code is a bit rough on my part. I just threw it together right now and it needs more debugging and cleanup. Feel free to play around with it. Also, I only optimized the AddToLine case (which was what my mail test case was triggering). The mCacheline cases still copy into an extra nsString. Using this patch, we reduced the overheard of this method by about 50%. We spend a little under 10 seconds in here now. I think most of that time is spent looking for the next space, line return, etc. If we can look at extracting larger chunks (instead of each word) like you suggested then we should be able to save more time. Lemme know if you have any questions about this patch or anything else. Good luck! (p.s. this patch still hasn't been fully tested I just did the one measurement).

Great. I will try it tomorrow (it's past midnight here now). The CompressSet function was no big win. I got the same time as before but that might be because I still had to copy the string. That's because the input to write was "const nsString&" and CompressSet is destructive and won't work on a variable declared const. I might try to change the chain of calls to use a non const if it's possible, but that will have to wait till tomorrow. By the way. Is there any trivial way to time a part of a function to make comparisons?

Putting on PDT+ radar for beta1.

Yeah there are some offset bugs in the code I wrote. I'll try to fix those and post another patch.

A question: Does strncasecmp work on PRUnichar?

oops..somehow the PDT+ field got dropped for this bug during the last update. Yeah, nsCRT::strncasecmp works with unicode strings.

I've applied your patch (with some corrections) but I still have problems measuring the impact. I tried to use Quantify in a trial version but it only crashes and eats too much memory (died at 800 MB last time).

I found the problem! We (my fault) are looking for both CR and LF to end a line, but since there are no CRs the whole string is searched. A nice O(n^2) algorithm. It's only necessary to look for one of CR and LF and then check if it's a CRLF. I hope to have a patch tonight when I get home, that is your morning. By the way, why are the html converted to text before displaying it in the composer window (as html, I bet)? Wouldn't it be enough to wrap it in a <blockquote>-tag?

BTW: we don't need to look for CR at all. The parser strips out all CR characters, so they should never make it into the document; the DOM linebreak character is LF with no CR. On quoting replies: if we're in an html compose window, we wrap the quoted text in a blockquote; if we're in plaintext, we wrap it in a pre (though that isn't working right now, I'm working on bug 28279) so it won't get wrapped along with the new text. But libmime is sometimes doing some sort of additional processing on the replied text, e.g. looping through and converting all the spaces to nbsp, and I don't know why it does that (I'm planning on filing a bug as soon as I have time to investigate).

Ah, then here is the patch for review and checkin (akkana or mscott?). I like it when I can remove 15 lines and make it so much faster and better. I guess we should also do the string passing optimization, but I think this is what was killing us the most. Index: src/nsHTMLToTXTSinkStream.cpp =================================================================== RCS file: /cvsroot/mozilla/htmlparser/src/nsHTMLToTXTSinkStream.cpp,v retrieving revision 3.53 diff -u -r3.53 nsHTMLToTXTSinkStream.cpp --- src/nsHTMLToTXTSinkStream.cpp 2000/02/23 01:22:20 3.53 +++ src/nsHTMLToTXTSinkStream.cpp 2000/02/24 18:59:07 @@ -1151,27 +1151,12 @@ // Put the mail quote "> " chars in, if appropriate. // Have to put it in before every line. - PRInt32 newCR, newLF; while(bol<totLen) { if(0 == mColPos) WriteQuotesAndIndent(); + + newline = aString.FindChar('\n',PR_FALSE,bol); - newCR = aString.FindCharInSet("\r",bol); - newLF = aString.FindCharInSet("\n",bol); - if(newCR>=0) { - if(newLF==newCR+1) { - // Found CRLF - newline=newLF; - } else if(newLF>=0 && newLF<newCR) { - // Found single LF - newline=newLF; - } else { - // Single CR - newline=newCR; - } - } else { - newline=newLF; - } if(newline < 0) { // No new lines. nsAutoString stringpart;

The patch looks great. I'll run with it in my tree for a while this morning to make sure I don't see any weird problems, but I don't see why there should be any. We should check this in. Scott, should I, or do you want to?

This is great Daniel. The search for the \r was definetly expensive. But I was still seeing a lot of time in the string copying too. Can we check in my patch (or a correct variant there of) which passes ptrs into the original string instead of copying each word into a new string? For the mailnew case, this was still the one that hurt us more than the search.

Sure do. I have that in my tree and forgot about it. With both the performance for the sink should be acceptable for beta 1. I'm still fighting with Quantify which give very good information when it works but it doesn't very often. If I see something interesting I will try to optimize further.

I haven't noticed any problems from Daniel's patch alone, but when I apply Scott's, I get massive horkage in the automated output tests (run TestOutSinks from dist/bin; it's currently broken on windows, I have a fix which I have approval to check in if the tree ever opens), lots of lines saying: WARNING: Error: possible unintended null in string, file nsString2.cpp, line 1123 Also, I get a warning because of the redeclaration of whitestring on line 1237.

Akkana, I think I may have mentioned to Daniel earlier in the bug report that my patch is NOT something that can be checked in with work. It's just something I worked on to help him get started. There are several offset bugs and one problem with a big comment saying fix me in it. Please don't check in what I have =). I was hoping Daniel was going to pick it up from there =).

Sure I did, but my memory is working worse than an 80 year old. :-/ I will attach the combined patch. I think it's only two or three changes to Scott's patch.

When I apply this patch, six of the automated output tests fail. I did check in that fix to make the tests work on Windows, so you should be able to run them. Update/build in htmlparser/tests/outsinks and then from dist/bin run TestOutSinks with no arguments.

Asch. I rushed that patch before going to CeBIT and it got very bad. I had it working but managed to delete it when trying to make a diff with just the right changes. Too many bug fixes in a single file... I will make a new look at what I missed.

I found the problem. I had changed the order of two lines; it's very bad to use the value of a variable _after_ you've changed it to the value it should have the next iteration. Additionally, thanks to the tests (thank you!) I avoided another little problem. Anyway, with good performance and tests passed, an attachement is coming.

The fix looks okay to me, and does indeed pass the tests. Have requested approval to check in. Scott, have you verified that this addresses the performance issues?

Hi Akkana, this fixes part of the problem. The big problem (the one that still takes over 4 minutes) is an editor performance problem when inserting text into an editor instance. Smfr and kin are looking at that now (Bug #28783) But this fix does fix the other half of the problem.

The fix is in.

mscott/akkana, please verify this one...I can't verify it....thanks!

updating status whiteboard. QA can't verify this; developer doesn't have time to verify

marking verified....