Last Comment Bug 124570 - 1.5% increase in Ts time, --enable-mathml ?
: 1.5% increase in Ts time, --enable-mathml ?
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.0
Assigned To: rbs
: Doron Rosenberg (IBM)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2002-02-09 01:31 PST by Chris McAfee
Modified: 2005-11-22 15:25 PST (History)
32 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
startup increase graph (5.21 KB, image/png)
2002-02-09 01:32 PST, Chris McAfee
no flags Details
Size diffs of >100 Bytes per file between 2002-02-08-{08,21} (2.62 KB, text/plain)
2002-02-10 07:33 PST, Andreas Franke (gone)
no flags Details
timeline output - ucvmath doesn't show up at startup (59.33 KB, text/plain)
2002-02-10 10:33 PST, rbs
no flags Details
output during style processing while mathml.css is there (426.58 KB, text/plain)
2002-02-10 13:01 PST, rbs
no flags Details
patch to load mathml.css on demand (first solution) (2.70 KB, patch)
2002-02-11 21:26 PST, rbs
no flags Details | Diff | Splinter Review
second solution (allow to load catalog data on demand) (22.43 KB, patch)
2002-02-12 22:54 PST, rbs
no flags Details | Diff | Splinter Review
updated patch in sync with the tip (22.49 KB, patch)
2002-02-21 04:22 PST, rbs
no flags Details | Diff | Splinter Review
third solution (attach catalog sheets to the document) (35.74 KB, patch)
2002-03-05 19:31 PST, rbs
no flags Details | Diff | Splinter Review
updated path - sync:ing with the tip (36.96 KB, patch)
2002-03-10 13:19 PST, rbs
harishd: review+
jst: superreview+
asa: approval+
Details | Diff | Splinter Review

Description Chris McAfee 2002-02-09 01:31:21 PST
Sometime around 6pm tonight, startup time Ts
increased across the 6 tinderboxes running Ts test
about 1.5% or so.
Comment 1 Chris McAfee 2002-02-09 01:32:02 PST
Created attachment 68706 [details]
startup increase graph
Comment 2 Chris McAfee 2002-02-09 02:42:30 PST
Started looking here, 3:30 checkin list.

http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&date=explicit&mindate=1013210820&maxdate=1013215799

Hmm, --enable-mathml ?  Maybe this got turned on correctly and is
taking up startup time now?
Comment 3 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2002-02-09 07:35:48 PST
None of the jumps on comet correspond to --enable-mathml.
Comment 4 Peter Trudelle 2002-02-09 12:10:19 PST
cc'ing everyone who checked in, r=, sr= or wrote the patches checked in during
that time.  If we are holding the tree closed for this bug, we need some urgency
here.
Comment 5 Brendan Eich [:brendan] 2002-02-09 13:51:58 PST
My jsstr.c change removed locking code, so it could only speed up string match,
replace, or split.  My jsregexp.[ch] changes affect code that doesn't run at
startup, based on breakpointing in gdb.

/be
Comment 6 Brian Ryner (not reading) 2002-02-09 13:55:45 PST
My changes only affect mouse clicks on form elements, so they wouldn't be hit
during a startup test.
Comment 8 Samir Gehani 2002-02-09 15:12:22 PST
Just took out one unused ``_elementID'' from pref-debug.xul.  Can't have
affected startup time.
Comment 9 David :Bienvenu 2002-02-09 15:36:32 PST
mail/news code doesn't run at startup, so Navin and Darin's changes would not
affect startup time.
Comment 10 Dawn Endico 2002-02-09 15:55:37 PST
There were 3 jumps in Ts Friday.

One of them was around 16:10 and the only checkin around that time that
seems to have anything to do with startup is danm's. I'm going to try backing
it out to test the effect on ts. (while the tree is closed). I mailed him
earlier but he seems to be away.

02/08/2002 16:10 danm%netscape.com mozilla/xpfe/appshell/src/nsAppShellService.cpp
1.175 6/8  generally give windows a 'minimize' widget. bug 77020 r=ben,jag


http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=danm%25netscape.com&whotype=match&sortby=Date&hours=2&date=explicit&mindate=02%2F08%2F2002+16%3A10&maxdate=02%2F08%2F2002+16%3A10&cvsroot=%2Fcvsroot
Comment 11 Dawn Endico 2002-02-09 17:40:14 PST
blake backed this out and two cycles passed on comet and Ts stayed
the same.

I just now backed out 2/8/2002 danm's 14:34 checkin of 
mozilla/ xpfe/ appshell/ src/ nsWebShellWindow.cpp
mozilla/ xpfe/ appshell/ src/ nsWebShellWindow.h
Comment 12 Dawn Endico 2002-02-09 20:13:57 PST
These are the changes i made. backing this stuff out didn't 
make any differece to startup times.

cvs update -j1.175 -j1.174 mozilla/xpfe/appshell/src/nsAppShellService.cpp
cvs update -j1.123 -j1.122 mozilla/xpfe/appshell/src/nsWebShellWindow.h
cvs update -j1.368 -j1.367 mozilla/xpfe/appshell/src/nsWebShellWindow.cpp

I tried backing out stuff that seemed likely to have caused
the problem but that didn't work and i'm not very inclided 
to back out the entire day's worth of stuff. If someone wants
to continue that then fine, otherwise i think i should just 
put everything back where i found it.

I'll check this back in in two hours unless someone vetos the idea
Comment 13 Dawn Endico 2002-02-09 23:25:19 PST
checked danm's stuff back in. everything should be back to normal now.

bryner is pulling and testing by date.
Comment 14 Brian Ryner (not reading) 2002-02-10 01:30:09 PST
I measured a startup time increase of about 1% (17.5ms) on my machine when
MathML was enabled.
Comment 15 Daniel Bratell 2002-02-10 03:49:58 PST
Could that just that the binaries including MathML are somewhat bigger? In that
case it could be a price we have to pay for that standard. 
Comment 16 Bradley Baetz (:bbaetz) 2002-02-10 05:21:33 PST
mathml was only enabled on unix at 2002/02/08 23:46:37 according to cvs log -
the original configure.in changes were wrong, and rbs backed them out. The real
changes to define mathml only went in then.
Comment 17 Andreas Franke (gone) 2002-02-10 05:34:13 PST
bbaetz: I think 2002/02/08 23:46:37 is GMT, thus 02/08/2002 15:46 local time as
displayed in the bonsai queries.
Comment 18 Andreas Franke (gone) 2002-02-10 05:47:51 PST
Also note that the "real" checkin turning mathml on was the "Automated update"
of configure that went in at 02/08/2002 15:57, showing up as leaf's checkin.

This checkin show up in all the suspicious time intervals, so it certainly
explains the jump.
Comment 19 Andreas Franke (gone) 2002-02-10 07:33:56 PST
Created attachment 68781 [details]
Size diffs of >100 Bytes per file between 2002-02-08-{08,21}

The most significant increase in size is libgklayout with ~230KB, as expected.
With this, enabling mathml cannot be done with zero cost. (But I'm not saying
that all of the delta is unavoidable.) component.reg is another obvious one.

Compare these numbers with attachment 64118 [details] in bug 109826 which were generated
from 0.9.7 builds with and without mathml, but keep in mind that they were done
with --disable-elf-dynstr-gc, so the size diffs for .so files should be smaller
now, and also mathml has had quite a few checkins between 0.9.7 and now.
Comment 20 rbs 2002-02-10 09:18:08 PST
There is something that I can think of re:mathml. At compile time, it adds the
mathml.css stylesheet to the ua.css:

http://lxr.mozilla.org/seamonkey/source/layout/html/document/src/ua.css

 37 @import url(resource:/res/html.css);
 38 @import url(chrome://global/content/xul.css);
 39 @import url(resource:/res/quirk.css);
+   @import url(resource:/res/mathml.css);

(This happens with a little perl script
http://lxr.mozilla.org/seamonkey/find?string=mathml.css)

But the mathml.css style rules are scoped under the mathml namespace, and rules
are hashed by namespace, so these rules should be isolated in a bucked somewhere
preventing other elements from being tested against them (dbaron, that was
bug 35847, right?)

For background, must read: http://bugzilla.mozilla.org/show_bug.cgi?id=35847#c62

The other thing that I can think of is that mathml adds new atoms (which all go
in the single global table that everybody shares). This is needed to test which
tags are encountered in a page -- e.g., how to know if <math> is encountered?
Could the impact of extra atoms be a significant factor in 1% ?

Apart from these two, the mathml code by itself is lazy and does its setup on
demand (when the user visits a page with <math>...</math> for the first time).
Comment 21 rbs 2002-02-10 09:59:18 PST
A fact that is clear, there are side-effects from the libs (old libs that have 
slightly augmented sizes, and new libs -- ucvmath).

Are the outputs of the timeline-enabled builds logged somewhere so that we can 
check the delta from the moment where mathml was enabled? That would be hightly 
informative indeed.

(in particular, I am curious to see if ucvmath is coming into play, I am 
currently setting up a timeline-enabled build to see what is going on at 
startup.)
Comment 22 rbs 2002-02-10 10:33:53 PST
Created attachment 68793 [details]
timeline output - ucvmath doesn't show up at startup

find ucvmath returns no occurrences, doesn't seem like ucvmath is the culprit.
Comment 23 rbs 2002-02-10 10:41:29 PST
mathml.css does show up here:

00009.174:  nsIOService::NewChannelFromURI(resource:///res/quirk.css)
00009.174: nsIOService::NewChannelFromURI([...]/WIN32_D.OBJ/bin/res/quirk.css)
00009.224:  nsIOService::NewChannelFromURI(resource:///res/mathml.css)
00009.224:  nsIOService::NewChannelFromURI([...]/WIN32_D.OBJ/bin/res/mathml.css)
00010.035:  PR_LoadLibrary total: 6.460 
([...]\WIN32_D.OBJ\bin\components\gklayout.dll)
00010.155:  PR_LoadLibrary total: 6.570 
([...]\WIN32_D.OBJ\bin\components\gkview.dll)
Comment 24 rbs 2002-02-10 10:57:17 PST
Didn't find other occurrences of strings with /math/. So culprits might be the 
increased dlls?
Comment 25 rbs 2002-02-10 11:07:38 PST
I am currenlty lurking in the Style System to see if the rules in mathml.css are 
being used (unexpectedly) during general style resolution.
Comment 26 Chris McAfee 2002-02-10 11:57:57 PST
-> rbs.  Added mathml to summary.
Comment 27 Chris McAfee 2002-02-10 12:21:13 PST
disabled mathml with --disable-mathml on comet, sleestack tinderboxes.
We got about half of the Ts delta back on comet, maybe 75% of the
delta back on sleestack.  I am testing this on facedown, results
in a few hours as that is a slower box.

It is possible that either 1) --disable-mathml isn't completely
disabling this, or 2) there is another regression hiding under the
mathml smokescreen.
Comment 28 rbs 2002-02-10 13:01:07 PST
Created attachment 68803 [details]
output during style processing while mathml.css is there

The namespace hashing seems to be doing well. The rules in mathml.css are left
out. But mathml.css is coming into play when initializing the UA stylesheets
(see the lines annotated with [MATHML] -- they are grouped together -- they all
happen when the style system is loading and initializing its data -- but I am
not sure why the namespace ID is zero everywhere).

Could you try to add an early exit in mathml-css.pl so that it doesn't execute
anything (i.e., it doesn't add mathml.css to the ua.css)? If there is a win by
doing that, it might mean that the addition of mathml.css is a hot spot worth
investigating, and so strategies like that used to load viewsource.css on
demand might have to be explored here.
Comment 29 rbs 2002-02-10 14:30:31 PST
> 1) --disable-mathml isn't completely disabling this

That option looks improbable given that TB would have turned red as it did the 
first time. Below is the old vs new configure.in before & after --enable-mathml. 
(Build gurus, is something missing?)

OLD configure.in:

dnl ========================================================
dnl MATHML
dnl ========================================================
MOZ_ARG_ENABLE_BOOL(mathml,
[  --enable-mathml         Enable MathML ],
    MOZ_MATHML=1
    AC_DEFINE(MOZ_MATHML))

NEW configure.in:

dnl ========================================================
dnl MathML on by default
dnl ========================================================
MOZ_MATHML=1
MOZ_ARG_DISABLE_BOOL(mathml,
[  --disable-mathml      Disable MathML],
    MOZ_MATHML= )
if test "$MOZ_MATHML"; then
  AC_DEFINE(MOZ_MATHML)
fi
Comment 30 rbs 2002-02-10 15:35:18 PST
Trying all sorts of voodoo, I disabled mathml.css (which timeline says is 
currently the sole thing that is directly loaded at startup as a result of
--enable-mathml). Let's see if that makes a difference.
Comment 31 rbs 2002-02-10 17:31:00 PST
Not much consistent gain on the TB outputs. Seems to be just noise.
Comment 32 Robert O'Callahan (:roc) (email my personal email if necessary) 2002-02-10 17:57:51 PST
Is Ts measuring a cold start?
Has anyone measured warm starts?
Comment 33 Chris McAfee 2002-02-10 18:55:38 PST
--disable-mathml recovers about 80% of the Ts delta,
this matches sleestack's behavior.  Comet didn't show
as much time recovery, the I/O is much much faster
than both sleestack and facedown so maybe that is
part of the reason.
Comment 34 rbs 2002-02-10 19:08:09 PST
Since removing mathml.css didn't seem to do much good, and there is no other 
loaded thing at startup that is mathml-only, the increase comes from the 
increased libs, right? 
Comment 35 Robert O'Callahan (:roc) (email my personal email if necessary) 2002-02-10 19:17:51 PST
That's why you want to try warm starts. The impact of increased lib size is
smaller with warm starts.
Comment 36 Brian Ryner (not reading) 2002-02-10 19:38:20 PST
rbs -- comet and sleestack had already had mathml turned off with
--diable-mathml when you checked in your patch, so it wouldn't have had any effect.
Comment 37 Chris McAfee 2002-02-10 19:41:18 PST
All tinderbox perf tests first fire the app up for the AliveTest,
e.g. does this crash on startup.  Then the tests run; for the Ts
case this means starting the app up 10 times, and we report the
best time of all those tries.
Comment 38 rbs 2002-02-10 19:55:51 PST
Could you just try to leave mathml.css out (this way, it would be clear that it 
is worth investigating the route of loading it on demand).
Comment 39 Chris McAfee 2002-02-10 20:56:07 PST
tinderbox deletes dist for each build pass, is that good enough?
Are we still pushing mathml.css out even though it's disabled?
Comment 40 rbs 2002-02-10 21:25:42 PST
No it is left out (since the directory where it is housed isn't compiled and the 
perl script isn't launched).

But what is missing is the delta from the increased dlls (the price to pay), and 
other side-effects that can be removed with a bit a work (although not simple). 

In the case of viewsource.css, it applied to most thing (HTML namespace), so a 
win could be anticipated. But in the case of mathml.css, it is already confined 
to the mathml namespace, so it is unclear whether it might help to delay it or 
not. That's why the question is: is there any win with the increased 
mathml-enabled dlls but without the unnecessary mathml.css? If the anwser is yes 
then, mathml.css could somehow be loaded on demand, if doable.
Comment 41 rbs 2002-02-11 12:07:34 PST
All the 'idef MOZ_MATHML' hooks: 
http://lxr.mozilla.org/seamonkey/search?string=MOZ_MATHML
Comment 42 rbs 2002-02-11 12:13:07 PST
(... shows that ucvmath was yet again forgotten in intl/uconv/ etc )
Comment 43 Brian Ryner (not reading) 2002-02-11 15:33:03 PST
Is anyone looking at what caused the _rest_ of the startup time increase? I'm
pretty sure it wasn't all mathml.
Comment 44 David Hyatt 2002-02-11 16:49:54 PST
Loading on demand isn't that hard.  You just wait until an element is 
encountered during XML content node buildup that matches the MathML 
namespace, and at that point synchronously load the agent sheet using 
LoadAgentSheet.
Comment 45 rbs 2002-02-11 17:26:52 PST
MathML has no hook in the content code. I wasn't very keen to add yet another
hook there at this stage, and/or was instead looking at it from the DOCTYPE or
from the frame construction code. I might indeed try lazy loading of it for
sanity sake. Did you people noticed an improvement when it was left out?
Comment 46 rbs 2002-02-11 18:27:40 PST
Will comment back later about the lazy loading of mathml.css - will have a go at 
intercepting the doctype, and see how it goes, or try other things as necessary.
Comment 47 rbs 2002-02-11 21:26:31 PST
Created attachment 68993 [details] [diff] [review]
patch to load mathml.css on demand (first solution)

The patch checks the doctype and loads the file if its FPI (Formal Public
Identifier) is the MathML one. Since there is only one doctype per document,
its cost is negligible since it just adds a single |if| test per page load when
MathML is enabled.

I have a second solution that slightly extends the patch so that it doesn't
need an #idef at all, and can be applied to others (SVG) in an extensible way.
But that iteration requires changing existing APIs.
Comment 48 rbs 2002-02-11 22:05:15 PST
The second solution is to extend the catalog table used in nsExpatDriver as 
follows:

struct nsCatalogEntry {
  const char* mPublicID;
  const char* mLocalDTD;
+ const char* mAgentSheet; // where in the future this can ultimately be an
                           // nsISupports* mCataloData for something bigger
                           // (see bug 98413: Implement XML Catalogs)
}; 

Then reconfigure the catalog table to read:

static const nsCatalogEntry kCatalogTable[] = {
 [...]
 {"-//W3C//DTD XHTML Basic 1.0//EN",           "xhtml11.dtd", nsnull },

 {"-//W3C//DTD XHTML 1.1 plus MathML 2.0//EN", "mathml.dtd",
                                               "resource:/res/mathml.css" },

 {"-//W3C//DTD SVG 20001102//EN",              "svg.dtd", nsnull },

 {nsnull, nsnull, nsnull}
};

Then change nsIExpatSink.idl so that

nsXMLContentSink::HandleDoctypeDecl(const PRUnichar *aDoctype,
                                    PRUint32 aLength)
and its other friends become

nsXMLContentSink::HandleDoctypeDecl(const PRUnichar *aDoctype,
                  PRUint32 aLength,
                  PRUnichar* aCatalogData)

where, for the moment, the catalog data would just be the pointer to the 
additional agent sheet needed for the corresponding XML vocublary. And do
  if (aCatalogData) { // additional agent sheet set
    load it...
  }

That's basically the idea, it just touches lots of files. I am fine with with 
either solution.
Comment 49 rbs 2002-02-12 22:54:48 PST
Created attachment 69207 [details] [diff] [review]
second solution (allow to load catalog data on demand)

harishd, heikki, this patch affects mostly the XML area. What is happening is
explained in my earlier post. In order to implement this, I needed to keep
track of the catalog entry associated toe current FPI (otherwise another lookup
would have been needed). So I added a mCatalogData member variable, and
reshuffled the code around to simple initialize it when available:

+	 if (aFPIStr) {
+	   // see if the Formal Public Identifier (FPI) maps to a catalog entry

+	  mCatalogData = LookupCatalogData(aFPIStr);
+	}

In summary, I turned Driver_HandleExternalEntityRef() into a wrapper in a
similar to the other drivers, and cached the catalog entry when it is
processed. Then, passed the data along at the stage when the doctype handler of
the sink is called. It is much clear to see the full picture upon applying the
patch.

I am more inclined towards this second solution since it removes the #ifdef and
makes another little step towards fixing bug 98413. However, since it crosses 
other modules, I am okay with either solution that other module owners prefer.
The most impacted remains MathML since mathml.css is being parsed all the time.
Comment 50 rbs 2002-02-15 19:06:20 PST
Comment on attachment 69207 [details] [diff] [review]
second solution (allow to load catalog data on demand)

Since mainstream users don't need to load mathml.css upfront, I am asking r=
and sr= so that this little gem can bake in the tree a little while before the
freeze for m0.9.9.
Comment 51 harishd 2002-02-19 13:41:54 PST
Comment on attachment 69207 [details] [diff] [review]
second solution (allow to load catalog data on demand)

>Index: content/xml/document/src/nsXMLContentSink.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/xml/document/src/nsXMLContentSink.cpp,v
>retrieving revision 1.205
>diff -u -r1.205 nsXMLContentSink.cpp
>--- content/xml/document/src/nsXMLContentSink.cpp	8 Feb 2002 22:46:09 -0000	1.205
>+++ content/xml/document/src/nsXMLContentSink.cpp	13 Feb 2002 06:13:44 -0000
>@@ -68,6 +68,7 @@
> #include "nsHTMLParts.h"
> #include "nsVoidArray.h"
> #include "nsCRT.h"
>+#include "nsIStyleSet.h"
> #include "nsICSSLoader.h"
> #include "nsICSSStyleSheet.h"
> #include "nsIHTMLContentContainer.h"
>@@ -1459,7 +1460,8 @@
> 
> NS_IMETHODIMP
> nsXMLContentSink::HandleDoctypeDecl(const PRUnichar *aDoctype,
>-                                    PRUint32 aLength)
>+                                    PRUint32 aLength,
>+                                    nsISupports* aCatalogData)
> {

Why should aCatalogData be nsISupports*? It can be nsIURI* ( and therefore can
avoid an extra QI ). no? 

>+          if (!styleSet) continue;

Please break this ( and others ) into two lines. 

>+Driver_HandleExternalEntityRef(void* aExternalEntityRefHandler,
>                                const XML_Char *openEntityNames,
>                                const XML_Char *base,
>                                const XML_Char *systemId,
>                                const XML_Char *publicId)
> {

Could you replace aExternalEntityRefHandler with aUserData ( just like other
wrapper methods )?

Still reviewing......
Comment 52 rbs 2002-02-19 15:16:05 PST
>Why should aCatalogData be nsISupports*? It can be nsIURI* ( and therefore can
>avoid an extra QI ). no? 

You hit the nail... Actually, I have hesitated between four options at the time 
of writing the patch:
- use a specific typename, e.g., |nsIURI*|, but opted out with the thinking that 
it wouldn't be immediate to a reader that the FPI of a document helps to lookup 
arbitrary built-in catalog data, and that this data can really be any object (or 
set of objects) in the future. (currently: the FPI is already used to determine 
two things -- the additional DTD file and the agent CSS file for formatting the
XML vocabulary being represented)
- use a |PRUnichar* aURL|, and do the NewURI(), & etc, from within the handlers. 
Prone to code duplications in the handlers. 
- use |void*|, then I thought, people would have to carry all the associated 
declarations in order to do casting (i.e., not really XPCOM-friendly)
- use |nsISupports*|, it had my vote because it didn't tie people to anything, 
it didn't impact those that were not actually using the data, and hinted at 
future extensions. The side-effect was that it had the overhead of a QI, but 
this remained limited to the case of an actual consumer and was one-off.

Anyway, these are the four possible options, maybe I am missing something in 
balancing the pros & cons.

> >+          if (!styleSet) continue;
>
>Please break this ( and others ) into two lines.

OK.

>Could you replace aExternalEntityRefHandler with aUserData ( just like other
>wrapper methods )?

Notice that Expat makes a difference. There is a specific setter for this (it 
can set a callback different from the default userdata that the other handlers 
always get). I am not clear why Expat does so. I got confused at first. That's 
why I used a different name so that other people reading the code can stop and 
take notice about this peculiarity too. Do you still prefer to call this 
|aUserData|?
Comment 53 rbs 2002-02-21 04:22:33 PST
Created attachment 70700 [details] [diff] [review]
updated patch in sync with the tip
Comment 54 rbs 2002-02-24 11:04:00 PST
-> moving to 1.0, if/when r/sr/a arrive.
Comment 55 rbs 2002-03-04 23:15:43 PST
Noted that the extra agent sheet is lost when going back and forth in print 
preview. Since PP just creates a document viewer and another style set [c.f.
DocumentViewerImpl::CreateStyleSet()] and walks the existing DOM without 
reparsing/reloading the document again, we don't get the chance to add the extra 
agent sheet in PP. The same story happens when PP goes away and the main window 
is recreated. The document is relaid out without the extra agent sheet.

Perhaps, it might be best to add the extra agent sheet straight in
nsContentDLF::gUAStyleSheet if it isn't there already. However, each doc viewer 
clones the UA sheet, and it could be that the extra sheet wasn't there yet when 
the UA was cloned (this might have a ramification in 
DocumentViewerImpl::CreateDocumentViewerUsing() where the UA sheet is passed on 
to another [c.f. viewer->SetUAStyleSheet(mUAStyleSheet)]. So, some other 
trickeries might be needed to fully cover all the edge cases. Need 
experimentation.
Comment 56 rbs 2002-03-04 23:56:09 PST
rods, care to read comment #55 above.
Comment 57 rbs 2002-03-05 19:31:29 PST
Created attachment 72730 [details] [diff] [review]
third solution (attach catalog sheets to the document)

There are entaglements out there... and it looked shaky to try to figure out
who (e.g., PP) is doing what with the document and stay in sync with them. So
this patch now makes the document the owner of the catalog stylesheet. The
catalog stylesheet stays alongside the other hidden stylesheets (the attr sheet
& inline sheet). I traced in the debugger and noted that things are going on
smoothly and there is even an early return in PresShell::ReconstructStyleData()
due to the fact that no frame has been constructed, for the catalog stylesheet
is added soon after the document has been created and there are no frames yet.

Seeking r/sr/a. As we are still early in the milestone, unanticipated problems
could be resolved or the patch backed out.
Comment 58 rbs 2002-03-10 13:19:13 PST
Created attachment 73472 [details] [diff] [review]
updated path - sync:ing with the tip

Attaching a conflict-free patch in sync with the numerous checkins that have
been going on recently.

I haven't observed a problem so far with this third approach (normal browsing
is OK, mathml.css gets loaded on demand on MathML pages, print-preview &
theme-switching stay okay on MathML pages too).
Comment 59 harishd 2002-03-11 14:46:56 PST
Comment on attachment 73472 [details] [diff] [review]
updated path - sync:ing with the tip

>Index: content/base/public/nsIDocument.h
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/public/nsIDocument.h,v
>retrieving revision 3.124
>diff -u -r3.124 nsIDocument.h
>--- content/base/public/nsIDocument.h	7 Mar 2002 03:33:52 -0000	3.124
>+++ content/base/public/nsIDocument.h	10 Mar 2002 20:49:37 -0000
>@@ -256,13 +256,13 @@
>   NS_IMETHOD GetNumberOfStyleSheets(PRInt32* aCount) = 0;
>   NS_IMETHOD GetStyleSheetAt(PRInt32 aIndex, nsIStyleSheet** aSheet) = 0;
>   NS_IMETHOD GetIndexOfStyleSheet(nsIStyleSheet* aSheet, PRInt32* aIndex) = 0;
>-  virtual void AddStyleSheet(nsIStyleSheet* aSheet) = 0;
>+  virtual void AddStyleSheet(nsIStyleSheet* aSheet, PRUint32 aFlags) = 0;
>   virtual void RemoveStyleSheet(nsIStyleSheet* aSheet) = 0;
>   NS_IMETHOD UpdateStyleSheets(nsISupportsArray* aOldSheets, nsISupportsArray* aNewSheets) = 0;
> 
>   NS_IMETHOD InsertStyleSheetAt(nsIStyleSheet* aSheet, PRInt32 aIndex, PRBool aNotify) = 0;
>   virtual void SetStyleSheetDisabledState(nsIStyleSheet* aSheet,
>-                                          PRBool mDisabled) = 0;
>+                                          PRBool aDisabled) = 0;
> 
Could you please replace |virtual void| with NS_IMETHOD_(void)? ( May be this
is not your problem )

+#ifdef NS_DEBUG
>+      nsCAutoString uriStr;
>+      uri->GetSpec(uriStr);
>+      printf("Loading catalog stylesheet: %s ... %s\n", uriStr.get(), sheet.get() ? "Done" : "Failed");
>+#endif

Optional: You can use nsXPIDLCString instead of nsCAutoString.
Comment 60 rbs 2002-03-11 15:25:01 PST
>Could you please replace |virtual void| with NS_IMETHOD_(void)? ( May be this
>is not your problem )

Let's not bother with this for now. There are several declarations like that in 
those files and with the associated NS_IMETHODIMP that would be needed for an 
overall consistency, the whole patch will just look more scary for r/sr/a.

>Optional: You can use nsXPIDLCString instead of nsCAutoString.

Debug only, and nsXPIDLCString was in the earlier patch. I removed it to sync 
with the iDNS landing that changed the interface from |char*| to |nsAString&|, 
causing the former |getter_Copies()| to stop the compilation.
Comment 61 harishd 2002-03-11 17:56:06 PST
Comment on attachment 73472 [details] [diff] [review]
updated path - sync:ing with the tip

r=harishd
Comment 62 Johnny Stenback (:jst, jst@mozilla.com) 2002-03-12 09:25:07 PST
Comment on attachment 73472 [details] [diff] [review]
updated path - sync:ing with the tip

sr=jst
Comment 63 Asa Dotzler [:asa] 2002-03-14 19:29:59 PST
Comment on attachment 73472 [details] [diff] [review]
updated path - sync:ing with the tip

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Comment 64 rbs 2002-03-15 03:06:01 PST
Patch checked-in, mathml.css is now loaded on demand rather than at startup.
Note: people wanting to use MathML somewhere outside the XHML+MathML doctype 
(e.g., in the chrome) now need to include mathml.css explicitly.
Comment 65 Andreas Franke (gone) 2002-03-24 06:51:16 PST
Is there any evidence that this checkin fixed (a significant part of) the
performance regression? (I looked, but I didn't find one on tinderbox. Maybe I
just didn't look hard enough, or I looked at the wrong place.)

See also comment 43.
Comment 66 rbs 2002-03-24 12:03:25 PST
I didn't notice much difference at the time of my checkin. From comment #14, the
increase in Ts was about 1% (17.5ms) and it was mostly I/O bound. Since other
things have been happening since then, it is hard to tell. What I did notice for
sure was a reported drop in the bloat numbers on TB. Anyway, it appeals to
reason to load this stuff on demand since it isn't the first cookie that makes
one fat.
Comment 67 rbs 2002-04-04 09:18:38 PST
See follow-up bug 132844: "Transformed docs lack the DOCTYPE, detect the MathML 
namespace".

I just checked in a patch to detect the MathML namespace at element creation, 
and load mathml.css from there too. So there is no need to include mathml.css 
explicitly even in documents that don't have a DOCTYPE.

Now if a document has the XHTML+MathML DOCTYPE, mathml.css is speedily loaded
as per this bug, otherwise the first MathML element triggers the load as per
bug 132844 (but since this later case can arise mid-way, a rebuild of the entire
frame tree may arise in the usual layout way).

Note You need to log in before you can comment on or make changes to this bug.