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

VERIFIED FIXED in mozilla0.9.1

Status

()

P3
normal
VERIFIED FIXED
18 years ago
16 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

({topembed+})

Trunk
mozilla0.9.1
x86
Linux
topembed+
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments)

4.01 KB, patch
Details | Diff | Splinter Review
8.74 KB, patch
Details | Diff | Splinter Review
8.91 KB, patch
Details | Diff | Splinter Review
9.80 KB, patch
Details | Diff | Splinter Review
1.37 KB, patch
Details | Diff | Splinter Review
5.55 KB, patch
Details | Diff | Splinter Review
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

Comment 3

18 years ago
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.
I was bitten by implicit conversion of nsLiteralCString to char* causing
infinite recursion.  Revised patch coming.

Comment 10

18 years ago
updated qa contact.
QA Contact: janc → bsharma

Comment 12

18 years ago
Patch, id=30045, looks good. r=harishd.

Comment 13

18 years ago
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

Comment 15

18 years ago
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.

Comment 16

18 years ago
Created attachment 31782 [details] [diff] [review]
hack to make nsScanner::ReadUntil() bit faster.
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.

Comment 20

18 years ago
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.

Comment 21

18 years ago
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.

Comment 23

18 years ago
What is the overhead in not using goto?

Comment 24

18 years ago
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.

Comment 25

18 years ago
ok, Tomi's patch/profiling explains it. 

Comment 26

18 years ago
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...

Comment 28

18 years ago
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.

Updated

18 years ago
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
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 34

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

Comment 35

18 years ago
Marking it verified as per above developer comments.
Status: RESOLVED → VERIFIED

Updated

16 years ago
Keywords: topembed+
You need to log in before you can comment on or make changes to this bug.