There are some function scope static constructors in nsHTMLTokens.cpp. I'll attach a patch that fixes them by using NS_LITERAL_STRING and some other new string stuff. I'm need to figure out whether that's what we want to do here... See, e.g., bug 63014 for why static constructors are bad...

Meant to assign to myself.

David, aTerminalSet.BeginReading() and aTerminalSet.EndReading() can live outside the while loop. Can't they?

I'm not sure about the BeginReading/EndReading. Anyway, that patch won't compile with the good implementation of NS_LITERAL_STRING anyway...

The reason I put the calls to BeginReading and EndReading in the loop was because FindCharInReadable adjusts its start parameter to the position at which the character was found or the end of the string (if not found). I guess I could skip the EndReading, though, although I would be a bit more comfortable with that if that parameter were |const| in the definition of FindCharInReadable.

Hmmm. These changes might be causing a crash on http://www.hcs.harvard.edu/~hgc/schedule.html

I was bitten by implicit conversion of nsLiteralCString to char* causing infinite recursion. Revised patch coming.

updated qa contact.

Patch, id=30045, looks good. r=harishd.

sr=vidur. Are all the variants of nsScanner::ReadUntil still used? I removed some dead code while I was last in the parser, but I believe that a significant amount still remains.

Moving to 0.9.1 for dealing with the remaining issues.

When i was profiling 87000 line <pre> document i find out that ReadUntil() takes 3sec out of 27 sec layout time. I hacked ReadUntil a bit and got 1sec off from that 3sec. I'll attach my hackings.

If you're going to do that you should probably change ReadUntil to require that aTerminalSet is an nsAFlatString (which didn't exist when I wrote the patch). Then you can just use |get|. You can't assume what you did will work for any old nsAString.

A quick jprof profile shows my patch is a significant improvement.

I did profiling with eazel profiler: Witout patches ReadUntil() takes 16sec, with myt patch 4sec and dbarons 1sec. count %f f %f+c f+c 87841 14.001 7064 32.476 16385 87841 8.359 3038 13.376 4862 87841 3.075 969 3.469 1093 f=time in function f+c= time in function + time in called subfunctions Profiled with 87000 line <pre> document (~7MB), times in ms. Without profiling, whole page load is 44sec and with patch 42sec.

I'd prefer to not use goto.

Do you think compilers will optimize it properly if we don't? It's being used as a |break|, except to break out of two loops instead of one.

What is the overhead in not using goto?

I think that goto is ok here, it is really intensive loop and any unneeded testing is bad. Code is also pretty clear with goto, with 2 breaks its harder to follow (like my code is). Overhead would be one extra test for every byte read from file.

ok, Tomi's patch/profiling explains it.

Convinced. r=harishd

FWIW, I just tested with gcc 2.91 and -O2 (which is how we'll be compiling our Linux builds as of next week), and it doesn't optimize Tomi's way of doing it -- we end up with an extra testl and je for a typical iteration of the inner loop. I also think there's something it's not putting into a register with the boolean-and-break method, but staring at two slighly different bits of generated assembly that do the same thing is confusing me enough that I can't tell anymore...

There's strong precedent for second-guessing the optimizer and using goto to break out of highly nested control structures in performance-sensitive code. sr=vidur.

Do you know if the compiler optimizes away the temporary needed for iter++ instead of ++iter for the String iterators? http://lxr.mozilla.org/seamonkey/source/string/public/nsStringIterator.h#116

With gcc-2.96-76 -O2, ++current is definitely better than current++. I think this is something we should file a separate bug on, since it's an issue more than just here.

C and C++ lack break to label or multilevel break, so goto is an appropriate remedy. In general, gotos that emulate break and continue to label are ok in my book (and I will push them in Mozilla code, over Pascal-esque redundant flags and tests). I see no harm in using prefix ++ for iterators, second-guess optimizer again. /be

Checked in the fix here at 2001-04-27 19:03 PDT. Marking this bug fixed -- it's already covered two issues and I'd rather open a new bug for the third. Filed bug 78032 on the |++iter| vs. |iter++| issue.

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

Marking it verified as per above developer comments.