Closed Bug 124570 Opened 22 years ago Closed 22 years ago

1.5% increase in Ts time, --enable-mathml ?

Categories

(SeaMonkey :: General, defect)

Product:
SeaMonkey
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla1.0

People

(Reporter: mcafee, Assigned: rbs)

Details

Attachments

(5 files, 4 obsolete files)

startup increase graph
5.21 KB, image/png
Details
Size diffs of >100 Bytes per file between 2002-02-08-{08,21}
2.62 KB, text/plain
Details
timeline output - ucvmath doesn't show up at startup
59.33 KB, text/plain
Details
output during style processing while mathml.css is there
426.58 KB, text/plain
Details
updated path - sync:ing with the tip
36.96 KB, patch
harishd
: review+
jst
: superreview+
asa
: approval+
Details | Diff | Splinter Review 
Sometime around 6pm tonight, startup time Ts
increased across the 6 tinderboxes running Ts test
about 1.5% or so.
Attached image startup increase graph — 

    
    

    
  
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?

    
    

    
  
None of the jumps on comet correspond to --enable-mathml.

    
    

    
  
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.

    
    

    
  
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

    
    

    
  
My changes only affect mouse clicks on form elements, so they wouldn't be hit
during a startup test.

    
    

    
  
Just took out one unused ``_elementID'' from pref-debug.xul.  Can't have
affected startup time.

    
    

    
  
mail/news code doesn't run at startup, so Navin and Darin's changes would not
affect startup time.

    
    

    
  
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

    
    

    
  
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

    
    

    
  
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

    
    

    
  
checked danm's stuff back in. everything should be back to normal now.

bryner is pulling and testing by date.

    
    

    
  
I measured a startup time increase of about 1% (17.5ms) on my machine when
MathML was enabled.


    
    

    
  
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. 

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  


  
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.

    
    

    
  
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).

    
    

    
  
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.)

    
    

    
  


  
find ucvmath returns no occurrences, doesn't seem like ucvmath is the culprit.

    
    

    
  
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)


    
    

    
  
Didn't find other occurrences of strings with /math/. So culprits might be the 
increased dlls?

    
    

    
  
I am currenlty lurking in the Style System to see if the rules in mathml.css are 
being used (unexpectedly) during general style resolution.

    
    

    
  
-> rbs.  Added mathml to summary.

Assignee: cathleen → rbs
Summary: 1.5% increase in Ts time → 1.5% increase in Ts time, --enable-mathml ?

    
    

    
  
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.


    
    

    
  


  
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.

    
    

    
  
> 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

    
    

    
  
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.
Status: NEW → ASSIGNED

    
    

    
  
Not much consistent gain on the TB outputs. Seems to be just noise.

    
    

    
  
Is Ts measuring a cold start?
Has anyone measured warm starts?

    
    

    
  
--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.


    
    

    
  
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? 

    
    

    
  
That's why you want to try warm starts. The impact of increased lib size is
smaller with warm starts.

    
    

    
  
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.


    
    

    
  
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.

    
    

    
  
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).

    
    

    
  
tinderbox deletes dist for each build pass, is that good enough?
Are we still pushing mathml.css out even though it's disabled?


    
    

    
  
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.

    
    

    
  
All the 'idef MOZ_MATHML' hooks: 
http://lxr.mozilla.org/seamonkey/search?string=MOZ_MATHML

    
    

    
  
(... shows that ucvmath was yet again forgotten in intl/uconv/ etc )


    
    

    
  
Is anyone looking at what caused the _rest_ of the startup time increase? I'm
pretty sure it wasn't all mathml.


    
    

    
  
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.


    
    

    
  
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?

    
    

    
  
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.

    
    

    
  


  
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.

    
    

    
  
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.

    
    

    
  


  
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 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 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......

    
    

    
  
>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|?

    
    

    
  


  

    
  

        Attachment #68993 -
        Attachment is obsolete: true

        Attachment #69207 -
        Attachment is obsolete: true

    
  
Target Milestone: --- → mozilla0.9.9

    
    

    
  
-> moving to 1.0, if/when r/sr/a arrive.
Target Milestone: mozilla0.9.9 → mozilla1.0

    
    

    
  
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.

    
    

    
  
rods, care to read comment #55 above.

    
    

    
  


  
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.

        Attachment #70700 -
        Attachment is obsolete: true

    
  
Summary: 1.5% increase in Ts time, --enable-mathml ? → [fix, awaiting r/sr/a(trunk)] 1.5% increase in Ts time, --enable-mathml ?

    
    

    
  


  
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).

        Attachment #72730 -
        Attachment is obsolete: true

    
    

    
  
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.

    
    

    
  
>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 on attachment 73472 [details] [diff] [review]
updated path - sync:ing with the tip

r=harishd

        Attachment #73472 -
        Flags: review+

    
  
Summary: [fix, awaiting r/sr/a(trunk)] 1.5% increase in Ts time, --enable-mathml ? → [fix, awaiting sr/a(trunk)] 1.5% increase in Ts time, --enable-mathml ?
Whiteboard: have r=harishd

    
    

    
  
Comment on attachment 73472 [details] [diff] [review]
updated path - sync:ing with the tip

sr=jst

        Attachment #73472 -
        Flags: superreview+

    
  
Summary: [fix, awaiting sr/a(trunk)] 1.5% increase in Ts time, --enable-mathml ? → [fix, awaiting a=] 1.5% increase in Ts time, --enable-mathml ?
Whiteboard: have r=harishd → have r=harishd, sr=jst

    
    

    
  
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

        Attachment #73472 -
        Flags: approval+

    
    

    
  
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.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Summary: [fix, awaiting a=] 1.5% increase in Ts time, --enable-mathml ? → 1.5% increase in Ts time, --enable-mathml ?
Whiteboard: have r=harishd, sr=jst

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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).
Product: Browser → Seamonkey

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