Closed
Bug 124570
Opened 23 years ago
Closed 23 years ago
1.5% increase in Ts time, --enable-mathml ?
Categories
(SeaMonkey :: General, defect)
SeaMonkey
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.0
People
(Reporter: mcafee, Assigned: rbs)
Details
Attachments
(5 files, 4 obsolete files)
|
5.21 KB,
image/png
|
Details | |
|
2.62 KB,
text/plain
|
Details | |
|
59.33 KB,
text/plain
|
Details | |
|
426.58 KB,
text/plain
|
Details | |
|
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.
| Reporter | ||
Comment 1•23 years ago
|
||
| Reporter | ||
Comment 2•23 years ago
|
||
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.
Comment 4•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
My changes only affect mouse clicks on form elements, so they wouldn't be hit
during a startup test.
more detailed check-in logs from all ts test machines
openwound 15063->15125 (+62ms)
http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&date=explicit&mindate=1013212620&maxdate=1013221379
mecha 6423->6472->6495 (+72ms total)
http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&date=explicit&mindate=1013210820&maxdate=1013215799
http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&date=explicit&mindate=1013215800&maxdate=1013232719
facedown 6341->6412 (+71ms)
http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&date=explicit&mindate=1013210280&maxdate=1013214479
http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&date=explicit&mindate=1013214480&maxdate=1013229959
comet 1616->1626 (+10ms)
http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&date=explicit&mindate=1013212500&maxdate=1013214419
sleekstack 5447->5497 (+50ms)
http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&date=explicit&mindate=1013211600&maxdate=1013214419
Comment 8•23 years ago
|
||
Just took out one unused ``_elementID'' from pref-debug.xul. Can't have
affected startup time.
Comment 9•23 years ago
|
||
mail/news code doesn't run at startup, so Navin and Darin's changes would not
affect startup time.
Comment 10•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
checked danm's stuff back in. everything should be back to normal now.
bryner is pulling and testing by date.
Comment 14•23 years ago
|
||
I measured a startup time increase of about 1% (17.5ms) on my machine when
MathML was enabled.
Comment 15•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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.
| Assignee | ||
Comment 20•23 years ago
|
||
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).
| Assignee | ||
Comment 21•23 years ago
|
||
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.)
| Assignee | ||
Comment 22•23 years ago
|
||
find ucvmath returns no occurrences, doesn't seem like ucvmath is the culprit.
| Assignee | ||
Comment 23•23 years ago
|
||
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)
| Assignee | ||
Comment 24•23 years ago
|
||
Didn't find other occurrences of strings with /math/. So culprits might be the
increased dlls?
| Assignee | ||
Comment 25•23 years ago
|
||
I am currenlty lurking in the Style System to see if the rules in mathml.css are
being used (unexpectedly) during general style resolution.
| Reporter | ||
Comment 26•23 years ago
|
||
-> rbs. Added mathml to summary.
Assignee: cathleen → rbs
Summary: 1.5% increase in Ts time → 1.5% increase in Ts time, --enable-mathml ?
| Reporter | ||
Comment 27•23 years ago
|
||
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.
| Assignee | ||
Comment 28•23 years ago
|
||
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.
| Assignee | ||
Comment 29•23 years ago
|
||
> 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
| Assignee | ||
Comment 30•23 years ago
|
||
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
| Assignee | ||
Comment 31•23 years ago
|
||
Not much consistent gain on the TB outputs. Seems to be just noise.
Is Ts measuring a cold start?
Has anyone measured warm starts?
| Reporter | ||
Comment 33•23 years ago
|
||
--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.
| Assignee | ||
Comment 34•23 years ago
|
||
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.
Comment 36•23 years ago
|
||
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.
| Reporter | ||
Comment 37•23 years ago
|
||
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.
| Assignee | ||
Comment 38•23 years ago
|
||
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).
| Reporter | ||
Comment 39•23 years ago
|
||
tinderbox deletes dist for each build pass, is that good enough?
Are we still pushing mathml.css out even though it's disabled?
| Assignee | ||
Comment 40•23 years ago
|
||
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.
| Assignee | ||
Comment 41•23 years ago
|
||
All the 'idef MOZ_MATHML' hooks:
http://lxr.mozilla.org/seamonkey/search?string=MOZ_MATHML
| Assignee | ||
Comment 42•23 years ago
|
||
(... shows that ucvmath was yet again forgotten in intl/uconv/ etc )
Comment 43•23 years ago
|
||
Is anyone looking at what caused the _rest_ of the startup time increase? I'm
pretty sure it wasn't all mathml.
Comment 44•23 years ago
|
||
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.
| Assignee | ||
Comment 45•23 years ago
|
||
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?
| Assignee | ||
Comment 46•23 years ago
|
||
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.
| Assignee | ||
Comment 47•23 years ago
|
||
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.
| Assignee | ||
Comment 48•23 years ago
|
||
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.
| Assignee | ||
Comment 49•23 years ago
|
||
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.
| Assignee | ||
Comment 50•23 years ago
|
||
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•23 years ago
|
||
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......
| Assignee | ||
Comment 52•23 years ago
|
||
>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|?
| Assignee | ||
Comment 53•23 years ago
|
||
Attachment #68993 -
Attachment is obsolete: true
Attachment #69207 -
Attachment is obsolete: true
| Assignee | ||
Comment 54•23 years ago
|
||
-> moving to 1.0, if/when r/sr/a arrive.
Target Milestone: mozilla0.9.9 → mozilla1.0
| Assignee | ||
Comment 55•23 years ago
|
||
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.
| Assignee | ||
Comment 56•23 years ago
|
||
rods, care to read comment #55 above.
| Assignee | ||
Comment 57•23 years ago
|
||
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 ?
| Assignee | ||
Comment 58•23 years ago
|
||
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 59•23 years ago
|
||
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.
| Assignee | ||
Comment 60•23 years ago
|
||
>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•23 years ago
|
||
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 62•23 years ago
|
||
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 63•23 years ago
|
||
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+
| Assignee | ||
Comment 64•23 years ago
|
||
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
Comment 65•23 years ago
|
||
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.
| Assignee | ||
Comment 66•23 years ago
|
||
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.
| Assignee | ||
Comment 67•23 years ago
|
||
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).
Updated•21 years ago
|
Product: Browser → Seamonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•