Closed
Bug 161481
Opened 22 years ago
Closed 22 years ago
Page source rendering terribly slow
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: benjamin, Assigned: rbs)
References
()
Details
(Keywords: perf, regression)
Attachments
(4 files, 6 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
scc
:
review+
sfraser_bugs
:
superreview+
asa
:
approval+
|
Details | Diff | Splinter Review |
Since yesterday (perhaps two or three builds before...) the page source
rendering is terribly slow.
Mozilla 1.1b
Mozilla/5.0 (Macintosh; U; PPC; en-US; rv:1.1b) Gecko/2002080703
Mac8.6
Comment 1•22 years ago
|
||
Could you find out which day the regression occured on? It'd help narrow down
what broke.
I -think- these bugs go in the ViewSource component; I'm sure someone will yell
if that's wrong.
Component: Parser → ViewSource
Keywords: regression
Confirmed using Mac/2002080208/9.2.2. Viewing source of [http://www.mozilla.org/] takes 7
seconds.
Does this happen on any other platforms?
Comment 4•22 years ago
|
||
Can you give me a 12-24 hour period to work with?
As a note, for me (Linux) view-source is instantaneous on
http://www.mozilla.org/. but I have a fast machine...
Using FizzillaCFM, this bug first appears in 2002-08-02-08-trunk. It was not
apparent in 2002-08-01-08-trunk.
(There are other bugs that first showed up in that build, too, such as bug 160887.)
Comment 6•22 years ago
|
||
Confirming with 2002080717-trunk/Mac OS 9.2 on this page.
There is not problem on 20020728.
And I don't think this is only about "View Surce".
Comment 7•22 years ago
|
||
So... is this a viewsource problem only? Or general layout?
ccing rbs, since his is the only mac-specific checkin I see in that date range...
There are other _symptoms_ that appeared on Mac at about the same time, that may
or may not be related. In addition to the bug I mentioned, there are problems
with TEXTAREA and selection performance.
(The text typing problem is noted in bug 129857, comment 10, as a worsening of
the originally reported problem in recent builds.)
Assignee | ||
Comment 10•22 years ago
|
||
>ccing rbs, since his is the only mac-specific checkin I see in that date range...
Yes, that was for bug 107146.
I have this gut feeling that the perf upshot might just be something that is
computed over and over again -- rather than being cached. I am seriously impeded
by the fact that I don't have a Mac to explore this. Cc:ing shofield for input.
Comment 11•22 years ago
|
||
sampling mozilla shows that the slowness is caused by the use of the atsui fallback to
measure (seemingly all) the text. i'll be able to find out why it's using the atsui fallback
after i get my build environment updated. i'll report back in a few hours.
Comment 12•22 years ago
|
||
Related Bug 161766 ?
Comment 13•22 years ago
|
||
generic fonts ('monospace,' 'serif,' etc) were not getting initialized properly
due to an oversight in the mathml changes. this patch should fix that.
however, i am getting assertions deep inside of nsString:
###!!! ASSERTION: data in U+0100-U+FFFF will be lost: '(*first < 256)', file
bufferRoutines.h, line 317
Break: at file bufferRoutines.h, line 317
###!!! ASSERTION: data in U+0100-U+FFFF will be lost: '(*first < 256)', file
bufferRoutines.h, line 317
Break: at file bufferRoutines.h, line 317
###!!! ASSERTION: data in U+0100-U+FFFF will be lost: '(*first < 256)', file
bufferRoutines.h, line 317
Break: at file bufferRoutines.h, line 317
at this location:
Breakpoint 1, CopyChars2To1 (aDest=0xbfffda38 "Osaka.h??p¿??H¿?¿?1E?",
anDestOffset=0, aSource=0x1393c470 "", anOffset=0, aCount=8) at
bufferRoutines.h:317
317 NS_ASSERTION( (*first < 256), "data in U+0100-U+FFFF will be
lost");
(gdb) bt
#0 CopyChars2To1 (aDest=0xbfffda38 "Osaka.h??p¿??H¿?¿?1E?", anDestOffset=0,
aSource=0x1393c470 "", anOffset=0, aCount=8) at bufferRoutines.h:317
#1 0x0146f168 in nsStrPrivate::StrAppend (aDest=@0xbfffda2c,
aSource=@0xbfffd978, anOffset=0, aCount=8) at nsStr.cpp:204
#2 0x0147448c in nsCString::AssignWithConversion (this=0xbfffda28,
aString=0x1393c470, aCount=8) at nsString.cpp:640
#3 0x01474518 in nsCString::AssignWithConversion (this=0xbfffda28,
aString=@0x1393c460) at nsString.cpp:645
#4 0x07da216c in GetEncoding (aFontName=@0x1393c460, aValue=@0xbfffdbf8) at
nsMacUnicodeFontInfo.cpp:513
#5 0x07da23f4 in GetConverter (aFontName=@0x1393c460, aConverter=0xbfffdd60)
at nsMacUnicodeFontInfo.cpp:549
#6 0x07da2758 in nsMacUnicodeFontInfo::GetConverterAndCCMap
(aFontName=@0x1393c460, aConverter=0xbfffdd60, aCCMap=0xbfffdd64) at
nsMacUnicodeFontInfo.cpp:595
#7 0x07da3fd8 in nsUnicodeFontMappingMac::FontEnumCallback
(aFamily=@0xbfffdeec, aGeneric=1, aData=0xbfffdfe8) at
nsUnicodeFontMappingMac.cpp:227
roger, maybe you know what's going on since it's happening inside of the
GetConverter() function.
Assignee | ||
Comment 14•22 years ago
|
||
re: the assertions. They arise because this call
const nsString* fontName = info->GenericFontNameForScript(script,type);
is returning a double-byte fontname for certain scripts, and GetConverter()
calls GetEncoding() which first tries to convert the fontname to a single-byte,
and the assertions fire. The fix on for that side is to make GetEncoding()
double-byte safe.
re: patch, some side-notes. I am not sure what the plans are for the Mac
regarding having a list of fonts per each generic font name. What
GenericFontNameForScript() is doing is to pick a single (hardcoded) font. If a
generic font is remapped to a font list, "font1, font2", we might have to
recursively callback the font enumerator to populate the list of data (as we do
on Windows). Maybe if a list of fonts was used, it wouldn't be necessary to loop
again over the scripts. But as I said, I am not sure what are the plans here.
Cc:ing ftang so that he can keep that in these side-notes in his radar.
The patch fits with the current setup. I only had to add an "if ((BAD_SCRIPT !=
script)" to stay coherent, and make GetEncoding() double-byte safe.
Assignee | ||
Comment 15•22 years ago
|
||
Attachment #94665 -
Attachment is obsolete: true
Assignee | ||
Comment 16•22 years ago
|
||
Comment on attachment 94685 [details] [diff] [review]
patch - take2
sr=rbs (if the patch compiles :-)
Attachment #94685 -
Flags: superreview+
Assignee | ||
Comment 17•22 years ago
|
||
ftang, care to r= this perf bug. The patch should also fix bug 161766, and maybe
other duplicates.
Comment 18•22 years ago
|
||
To quantify "terribly slow", see
ftp://ftp.mozilla.org/pub/data/loadtimes/daily_loadtime.html and check out the
OSX graph.
Updated•22 years ago
|
Comment 19•22 years ago
|
||
+ // XXX need tuning to list the appropriate fonts for the Mac
+ if (fontName.Equals(NS_LITERAL_STRING("Arial")) ||
+ fontName.Equals(NS_LITERAL_STRING("Times New Roman")) ||
+ fontName.Equals(NS_LITERAL_STRING("Courier New")) )
Can we fix this before we commit the patch please?
Comment 20•22 years ago
|
||
Also, we need to get Mac pageload numbers from before the MathML fixes, and
after (with this patch) and make sure that we have not regressed. We cannot let
MathML regress Mac pageload; pageload should have been tested before the changes
were landed.
Comment 21•22 years ago
|
||
More possible relateds: bug 161878 and bug 161887.
Assignee | ||
Comment 22•22 years ago
|
||
Per comment #19, just adding a printf in nsDeviceContextMac::GetSystemFont() to
get the fonts used at startup, namely the fonts used in the 'font:' short-hand:
font : caption | icon | menu | message-box | small-caption | status-bar | etc
Need help from someone who can apply the patch and report the output produced
so that the main patch can be updated.
Assignee | ||
Comment 23•22 years ago
|
||
Attachment #94874 -
Attachment is obsolete: true
Assignee | ||
Comment 24•22 years ago
|
||
From comment 11, the perf problem is due to the fact that the ATSUI fallback is
used all the time -- this means that characters are processed *one at a time*!
So this fix might bring back the pageload graph close to where they were since
the dreaded ATSUI fallback won't be used anymore.
(Also hoping that this activity will help in fueling the drive for improving the
Mac font code at some stage. With the hindsight that has been built over time,
there are avenues for some overall improvement/re-organization of the code.)
Updated•22 years ago
|
Comment 25•22 years ago
|
||
pageload tests on OS/X for 2002-08-01-08-trunk and 2002-08-02-08-trunk:
8/01 - 1144 msec. (avg. med.)
8/02 - 1357 msec. (avg. med.)
which is about a 19% increase. I should note, though, that two of the testpages
account for the lion's share of the slowdown: lxr.mozilla.org, which went from
~1300 ms to ~6500 ms, and www.w3.org_DOML2Core, which went from ~2230 ms to
~3740 ms. (Those are also, not surprisingly, two of the most text-heavy
documents in the set of test pages). Not counting those two test pages, the
increase was about 4% in time to load pages.
[I should also note that I suck for not picking this up at the time the
regression occurred. Thanks to the people who tracked this down.]
Assignee | ||
Comment 26•22 years ago
|
||
Although informative, the tests are somewhat premature because the fix for this
bug is still out. There was an inadvertant omission in the landing, and
unfortunately it is causing one of the slowest conceivable codepath to be
executed: measuring a string one character at a time, drawing it one character
at a time, etc. I am not sure why the ATSUI fallback was implemented that way.
The pageload tests confirm indeed that it can't be used by default. I suggest
waiting for this patch to land, as it will fix the problem that is activating
this dreaded fallback codepath. Upon checking of the fix, the execution will be
back to the initial default codepath.
Assignee | ||
Comment 27•22 years ago
|
||
rob, could you figure out the fonts used at startup so that we can be done with
this bug? Are you using a fast machine where the perf decline wasn't apparent?
(Or alternatively, do you have a slow machine where Mozilla was/is just slow
anyway :-)
Comment 28•22 years ago
|
||
This is a blocker to 1.1. adding to the 1.1 dependency list. Can someone give me
a time estimate here? If we can't get this corrected in a reasonably short
period of time, I'm inclined to back out the changes that caused it on the 1.1
branch.
Blocks: 1.1
Comment 30•22 years ago
|
||
the output on startup is:
GetSystemFont: ID=3, family=Lucida Grande
GetSystemFont: ID=3, family=Lucida Grande
GetSystemFont: ID=3, family=Lucida Grande
Comment 31•22 years ago
|
||
not viewsource. This is a layout bug. fixing summary and moving to layout.
Assignee: doron → rbs
Component: ViewSource → Layout
QA Contact: pmac → petersen
Assignee | ||
Comment 32•22 years ago
|
||
Is that the same output for both skins, classic and modern?
For information, what if you printf() in the callback function itself to see?
Assignee | ||
Comment 33•22 years ago
|
||
Re: comment #28
> Can someone give me a time estimate here?
Once the fonts used at startup are known, it should be ready to go. Today or
tomorrow -- if reviews drag on.
Comment 34•22 years ago
|
||
You need to use different fonts for Mac OS 9 and Mac OS X. On Mac OS 9, the user
can choose their system font (Gadget/Chicago etc etc).
Comment 35•22 years ago
|
||
the fonts are the same for the classic and modern themes.
on my machine (classic environment) the os9 appearance control panel allows for
the "Large System Font" to be "Charcoal," "Chicago," "Capitals," "Gadget,"
"Sand," "Techo," or "Textile." it allows the "Small System Font" to be
"Geneva." on osx i think the system font can currently only be "Lucida
Grande," but i suppose that could change in the future.
is something like this patch what you had in mind?
Assignee | ||
Comment 36•22 years ago
|
||
Missing "Lucida Grande" in the check list.
Assignee | ||
Comment 37•22 years ago
|
||
Nope, it is there. It was the (in)famous Mac-binary attachment problem...
Assignee | ||
Comment 38•22 years ago
|
||
Any r= on schofield's patch to move it forward?
Comment 39•22 years ago
|
||
Confirming additional comment # 35, these are the available Large System fonts
under Mac OS 9.2.2 English-North American.
Comment 40•22 years ago
|
||
Comment 41•22 years ago
|
||
Comment on attachment 95037 [details] [diff] [review]
patch - take 3
r=jfrancis;
note: i dont know diddly about mac font issues; so get another review if you
need assurance there.
From a line-by-line of view,it looks ok. I do wonder if we have a hard and
fast guarantee about
::FontToScript(fontNum)) never returning a script that is greater than the size
of the mScriptFallbackFontIDs array. Does that [] operator do bounds checking?
Attachment #95037 -
Flags: review+
Assignee | ||
Comment 42•22 years ago
|
||
Good point. In fact, looking into details, I see that:
for(ScriptCode script = 0; script < 32 ; script++)
{ ^^^^^^^^^^^^^^^^^
const nsString* fontName = info->GenericFontNameForScript(script,type);
[...]
ScriptCode script = (converter ? BAD_SCRIPT : ::FontToScript(fontNum));
^^^^^^^^^^^^^^^^^
The running "script" was overwritten.
Updated patch coming up. Actually, since there isn't any generic font that is
mapped to a special symbolic font (as yet), the converter business isn't
necessary there.
Assignee | ||
Comment 43•22 years ago
|
||
patch to be checked in
Attachment #94685 -
Attachment is obsolete: true
Attachment #94875 -
Attachment is obsolete: true
Attachment #95037 -
Attachment is obsolete: true
Assignee | ||
Comment 44•22 years ago
|
||
Attachment #95125 -
Attachment is obsolete: true
Assignee | ||
Comment 45•22 years ago
|
||
Checked in. Just cvs update mozilla/gfx/src/mac to get this fix in your tree.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 46•22 years ago
|
||
Following bug 107146, these are the fixups that have been made so far. The
fixups include this bug and the one-liner that was made for bug 161048.
Seeking a= on the m1.1 branch.
Updated•22 years ago
|
Attachment #95146 -
Flags: approval+
Comment 47•22 years ago
|
||
Comment on attachment 95146 [details] [diff] [review]
proposed fixups against the m1.1 branch
a=asa (on behalf of drivers) for checkin to 1.1
Comment 48•22 years ago
|
||
actually, wait. Where's the super-review for this code?
Comment 49•22 years ago
|
||
Comment on attachment 95146 [details] [diff] [review]
proposed fixups against the m1.1 branch
posthumous sr=sfraser
Attachment #95146 -
Flags: superreview+
Comment 50•22 years ago
|
||
Comment on attachment 95146 [details] [diff] [review]
proposed fixups against the m1.1 branch
r=scc
Attachment #95146 -
Flags: review+
Comment 51•22 years ago
|
||
Per comment 18, anyone know why page load on OS X dropped so dramatically
following the spike up? The last data point is well below what it was before
MathML went in.
Assignee | ||
Comment 52•22 years ago
|
||
Fixed on the branch.
Comment 53•22 years ago
|
||
this seems to have caused bustage on the mac opt 1.1 build.
Comment 54•22 years ago
|
||
> this seems to have caused bustage on the mac opt 1.1 build.
my gcc build seems to work ok.
Comment 55•22 years ago
|
||
Mac builds use CodeWarrior, not gcc. However, Tinderbox seems to have cleared
itself up.
Comment 56•22 years ago
|
||
My tests suggests that we regained the performance loss. These builds aren't
from the exact day of the changes but they're close and the data seems reasonable.
1.0 final - 1780 ms (average median) 1.0 datapoint
07/31 - 1719 ms (average median) before rbs regression
1.1 08/10 - 2085 ms (average median) after rbs regression
1.1 08/14 - 1734 ms (average median) after rbs fix
I'm not sure if that means we're all the way back down but it's back to within
1% when it had deviated about 21%. I only did one run of the test and I didn't
write down the specific pages and their changes but it seems we're pretty much
back to normal. John, does this look/sound right to you?
All tests performed on my G4500 with 256MB RAM. Each test was run with a new
profile on a fresh install.
Comment 57•22 years ago
|
||
Asa, thanks. Yeah, sounds pretty good. I can't attach significance to
difference below 1%. I'll give this a spin on another OS/X box tomorrow,
but in the interim, looks fine.
You need to log in
before you can comment on or make changes to this bug.
Description
•