Closed Bug 65431 Opened 24 years ago Closed 23 years ago

[static ctor]function scope static ctors in nsHTMLTokens.cpp

Categories

(Core :: DOM: HTML Parser, defect, P3)

Product:
Core
Component:
DOM: HTML Parser
Platform:
x86
Linux
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.1

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

(Keywords: topembed+)

Attachments

(7 files)

patch
4.01 KB, patch
Details | Diff | Splinter Review
revised patch (not yet tested)
8.74 KB, patch
Details | Diff | Splinter Review
revised again (fixes crash)
8.91 KB, patch
Details | Diff | Splinter Review
patch, revised to avoid performance hit for platforms with 4-byte wchar_t
9.80 KB, patch
Details | Diff | Splinter Review
hack to make nsScanner::ReadUntil() bit faster.
1.37 KB, patch
Details | Diff | Splinter Review
proposed performance improvement
5.55 KB, patch
Details | Diff | Splinter Review
differences between prefix and postfix ++ with gcc-2.96-76 -O2
2.57 KB, text/plain
Details 
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.
Assignee: harishd → dbaron
Blocks: 60709
Priority: -- → P3
Target Milestone: --- → mozilla0.9
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.
Status: NEW → ASSIGNED
I was bitten by implicit conversion of nsLiteralCString to char* causing
infinite recursion.  Revised patch coming.
updated qa contact.
QA Contact: janc → bsharma
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.
Target Milestone: mozilla0.9 → mozilla0.9.1
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.
Blocks: 77959
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.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
*** Bug 77959 has been marked as a duplicate of this bug. ***
Marking it verified as per above developer comments.
Status: RESOLVED → VERIFIED
Keywords: topembed+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: