Closed Bug 124570 Opened 23 years ago Closed 23 years ago

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

Categories

(SeaMonkey :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.0

People

(Reporter: mcafee, Assigned: rbs)

Details

Attachments

(5 files, 4 obsolete files)

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.
(... 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: 23 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.

Attachment

General

Creator:
Created:
Updated:
Size: