The default bug view has changed. See this FAQ.

[FIX]Stylesheet loading blocks parser

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
CSS Parsing and Computation
P1
major
RESOLVED FIXED
16 years ago
3 years ago

People

(Reporter: Hixie (not reading bugmail), Assigned: bz)

Tracking

(Depends on: 1 bug, Blocks: 3 bugs, 4 keywords)

Trunk
mozilla1.9alpha1
arch, css1, perf, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Hixie-P2][Hixie-2.0], URL)

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

16 years ago
When the style system hits a stylesheet it blocks the parser and content sink.
This is Bad (tm).

What we should do is asynchronously download and parse the stylesheet, but with
frame creation supressed for a certain amount of time (hidden-pref configurable,
an initial value of a few seconds, 6s maybe, would be good).

See:
http://www.bath.ac.uk/~py8ieh/internet/importtest/extra/nestedstylesheets.html
http://www.bath.ac.uk/~py8ieh/internet/importtest/extra/infinite-persistent.html
http://www.bath.ac.uk/~py8ieh/internet/importtest/extra/infinite-alternate.html
http://www.bath.ac.uk/~py8ieh/internet/importtest/extra/infinite-import.html
(Reporter)

Updated

16 years ago
Keywords: css1, mozilla1.0, perf, regression, testcase
Whiteboard: [Hixie-P2]

Comment 1

16 years ago
Deja Vu

I had previously tried to do something like this, without the delay in frame
creation, but if I remember correctly Pierre had some issues with the idea (I
think he was fine with alternates but not persistent sheets). Personally, I
think that stylesheets should ALWAYS be loaded in the background, and the
supression of frame creation is a nice twist to prevent the inevitable reflow
when the stylesheet is loaded.

Reassigning to Pierre.
Assignee: attinasi → pierre

Comment 2

16 years ago
Related discussions can be found in bug 17309. Bug 26297 and bug 6782 are 
interesting too.

In bug 17309, read [Pierre Saslawsky 2000-03-31 02:54] and 
[Marc Attinasi 2000-04-04 15:28]. Marking future.
Status: NEW → ASSIGNED
Target Milestone: --- → Future

Updated

16 years ago
Blocks: 79021

Updated

15 years ago
Keywords: regression
(Reporter)

Comment 3

15 years ago
Note the specific comments in the initial description of this bug. Frame
creation should not be put off forever. There should be a timeout.
Keywords: arch
(Reporter)

Updated

15 years ago
Blocks: 137159
(Reporter)

Updated

15 years ago
No longer blocks: 137159
Whiteboard: [Hixie-P2] → [Hixie-P2][Hixie-2.0]

Updated

15 years ago
Blocks: 137159

Comment 4

15 years ago
Is bug 123319 a dup of this one?
No, but it could be related.
Priority: -- → P1
Assigning pierre's remaining Style System-related bugs to myself.
Assignee: pierre → dbaron
Status: ASSIGNED → NEW

Comment 7

14 years ago
Sounds like a reasonable performance gain - isn't it?
Keywords: mozilla1.0 → mozilla1.4
taking; I have plans to work on this anyway...
Assignee: dbaron → bzbarsky
Target Milestone: Future → mozilla1.4alpha
Correctness issues blocking this, to some extent (in that they could become 
more visible when this gets fixed):

1)  {ib} situations triggered by a style reresolve don't get handled correctly
    (bug 168883, basically).
2)  ::before/::after not handled correctly by ReResolveStyleContext yet (bug
    126072)
Depends on: 126072, 168883
Blocks: 186943
Blocks: 197015
Target Milestone: mozilla1.4alpha → mozilla1.5alpha
Depends on: 200931

Updated

14 years ago
Blocks: 203448
Blocks: 204086
(Reporter)

Comment 10

14 years ago
Note that the sheets that should be applied may change as we are loading them;
we should apply NO sheets if all the applicable sheets aren't available yet.
Well, there is one problem with this.  How do we tell whether all the sheets are
available if we haven't even parsed the whole document yet?

Right now, I plan to have a small timeout (10ms?  100ms?  250ms?  Whatever the
paint suppression timeout is?) before I even bother to check whether sheets are
loaded.  At that point, if there are pending sheet loads for sheets that will be
applied immediately, we will wait for a further small timeout (100ms? 250ms? 
2000ms?) before just giving up on the still-loading sheets and applying what we
have.

Of course I still need to make this work somehow with persistent alternate style
sets....
(Reporter)

Comment 12

14 years ago
> Right now, I plan to have a small timeout (10ms?  100ms?  250ms?  Whatever the
> paint suppression timeout is?) before I even bother to check whether sheets 
> are loaded.

I think that's reasonable. I think we can also short-circuit the stylesheet
application timeout in the case of hitting the end of the <head> element (or the
end of the document, of course), or maybe even the start of the root element in
the case of non-XHTML XML documents.


> At that point, if there are pending sheet loads for sheets that will be
> applied immediately, we will wait for a further small timeout (100ms? 250ms? 
> 2000ms?) before just giving up on the still-loading sheets and applying what 
> we have.

We really really don't want to apply partial stylesheet sets. It's quite common
for authors to depend on the cascade. If we know there are outstanding sheets
that still want to be applied, then don't apply any author sheets.

At least, that was my gut instinct. But what if we have a document with three
sheets, a, b, and c, and initially just a applies, but then through media
queries a and c apply, and c is not yet ready, should we revert to no sheets? or
show only a?

This is a problem. What we really want to do from a user's point of view is
stick to the currently applied stylesheets until we can switch to others. Hmm.

How about this: Every time the applied set of sheets changes, if we don't have
the available sheets ready yet, we stick to the current set, and each time a
stylesheet finishes loading, we check if now we can apply the optimal set of
sheets, and if we can, we switch.

Really this would require the alternate stylesheet UI to be able to have
unavailable stylesheet sets disabled until all the available stylesheets are in.
This probably wouldn't be a bad idea anyway.

The key thing, though, is that if you have two rules a and b, then the three
following cases really should act identically to the user:

   <style type="text/css">
      a {}
      b {}
   </style>

   <link rel="stylesheet" href="data:text/css, @import 'data:text/css, a { }';
                                               b {}">

   <link rel="stylesheet" href="data:text/css, a {}">
   <link rel="stylesheet" href="data:text/css, b {}">

...(assuming data: URIs took time to load).


> Of course I still need to make this work somehow with persistent alternate 
> style sets....

You could just check the available sets when your first timer expires (or is
short-circuited).
> We really really don't want to apply partial stylesheet sets.

So we should just wait for http timeouts and then apply partial stylesheet sets?  :)
(Reporter)

Comment 14

14 years ago
Yes.
Blocks: 220142
Blocks: 218764
Blocks: 185285
Blocks: 220542
Mental note.  Bug 200931 (which was necessary as a prerequisite to this bug)
regressed Tp by about 0.5% (5ms on btek), Txul by 2-3% (looks like 10ms on
comet, 30ms on luna), Ts by ~1% (12ms on comet, 20ms on luna).  I suspect this
is mostly due to the fact that we now properly call BeginUpdate() before
toggling sheet states and this makes the content sinks a little more likely to
flush out content then instead of just updating their idea of what's been flushed...
Blocks: 215465

Comment 16

14 years ago
An important note about fixing this bug.  Safari now has a blocker bug on it from a Citibank site where 
their DHTML doesn't work because they try to ask for a layout property before the stylesheets have all 
loaded.

We don't really even know how to fix the problem, except to move to the Mozilla model.  We may end 
up having to make <script> tags block until all stylesheets are loaded.
Heh.  I was afraid of something like that....
(Reporter)

Comment 18

14 years ago
Hyatt: Can you give us the URI so we can see how Opera deals with this?

Comment 19

14 years ago
I can't get the real URL (it's a private Citibank page).  I can give you the reduction.  Is that good enough?  
Our solution to this problem was to actually go ahead and allow FOUC if you access a property from JS 
that requires a layout to yield an answer.
That's simpler than blocking when you hit a script, sure.  But the answer is
likely to be wrong if the sheets are at all nontrivial, no?
I suppose that blocking the JS (and the rest of the page load) if someone
requests a property which an unloaded stylesheet could affect is too complicated
an error-prone?
You mean blocking the JS in mid-execution?  Could be done by throwing on a
nested event queue.... but background loads have to proceed because we need to
load those stylesheets, and those loads will trigger onload events for those
elements, and then the page JS has to be able to deal with reentering a function
before it finished executing the first time, which it probably can't.
(Reporter)

Comment 23

14 years ago
You can't block JS anyway because you don't know how many sheets you'll need.
One of them might take minutes to download, and would never get applied normally
anyway, for example. (Think DNS failure or host timeout.)

hyatt: sure, the testcase would be fine, thanks.
Hixie: You'd block until timeout, though. If I have a webpage pointing to a
stylesheet on a host which is dead, we presuably don't block the parser forever
currently.

bz: Hmm, yeah, onload events possibly looking at vars would be an issue.
I thought about it, and starting layout on script access would basically need a
new API similar to FlushPendingNotifications but not quite.

Adding this is likely to be just as much work as just blocking the script
execution till the sheets are loaded... though that does mean that pages that
use <script> will see less of a win from this bug.
(Reporter)

Comment 26

13 years ago
> If I have a webpage pointing to a stylesheet on a host which is dead, we 
> presuably don't block the parser forever currently.

Actually, we do. That's (mostly) what this bug is about. Try looking at a google
cache page when the site is down, for example. Takes minutes to load, because we
twiddle our thumbs waiting for the stylesheets to come.


bz: Block when trying to evaluate a <script> until the short timer has blown or
the pages have been loaded. I wouldn't bother doing any special about inline
event handlers, or scripts attached from other documents, or anything like that.
They can just live in the not-yet-styled world if they are fired early. People
who want to reliably do offsetTop nonsense in esoteric cases like that can use
the onload event.

Comment 27

13 years ago
See also bug 200994 : when the host isn't just down, but doesn't exists at all,
we'll have another delay in loading the page. I know that this bug speaks about
scripts, not stylesheets, but I suspect that it will have the same effect if
multiple stylesheets are in use (less common than multiple scripts). Bug 208312
(negative DNS-cache) should fix that. Should this bug depend on that ?
This bug is about a specific problem -- stylesheet loading blocks the parser. 
Comment 27 is not really related to that... 
Blocks: 224029
Blocks: 241128
Blocks: 243338
Blocks: 244150
Blocks: 241982
Depends on: 293825
Blocks: 289325
Blocks: 289502
Blocks: 156430
Blocks: 309587
(Reporter)

Comment 29

11 years ago
I'm no longer really sure we should do any of the fancy "wait for all the sheets to be available", by the way. I'm thinking now that applying just the available sheets is better than waiting around for all of them to not come.
We still need to do _something_ to avoid FOUC and the resulting performance hit...  I agree that we should do the simplest thing that's reasonable effective.

For what it's worth, I've had this change in my tree for about a year and a half now (but without a solution to the script access and FOUC issues, and with outstanding XXX comments).  If people think that simply doing layout without the sheets on script access is a reasonable approach, that could probably be done in a reasonable amount of time.
Any chance you could attach a patch for the change you've had in your tree?

It seems like we probably do want to do a bit to prevent both problems, although I think a timer (if the stylesheets don't load within N seconds of requesting the first/last one, then go on without them) would probably be sufficient for the FOUC problem.

I'm not sure about script access, though.  We can block the HTML parser.  I'm not sure how long we should, though.
Created attachment 230047 [details] [diff] [review]
What I have in my tree right now

Comment 33

11 years ago
http://webkit.org/blog/?p=66 talks about this issue.
Blocks: 360467
Depends on: 18333
Created attachment 253836 [details] [diff] [review]
I think this is set to go

Still need to write tests, but...
Attachment #253836 - Flags: superreview?(jonas)
Attachment #253836 - Flags: review?(jonas)
Attachment #230047 - Attachment is obsolete: true
So I should clarify what this patch implements.

With this patch the content sink can keep track of exactly how many sheets are still loading (and if it really cares, which ones).  The script loader gets blocked on pending sheets -- note that this blocks just script execution, not loading.  So we'll load sheets and a following script in parallel.  The sink can use the information it has to decide what it wants to do in terms of calling StartLayout.

The patch implements a really simple policy thus far:

1)  If an unforced (not due to script access from another page) StartLayout call
    happens, and we have pending non-alternate sheets, don't start layout.
2)  If we run out of pending sheets and we skipped starting layout for pending
    sheets, go ahead and start layout.

If we have performance issues we can tweak step 2 to post an event or timer instead.  If we have some sort of other issues (e.g. decide we want to address the "page never shows if sheets never load"), we can tweak step 1 to post a timer.

I figure those are best done as followups; this patch is already big enough.
Well, I found my first bug!  Google spreadsheet does some sort of weirdness where it doesn't wait for the subframe's onload to calculate widths of things in it.  In this particular case, we actually finish parsing before the sheets are done loading, and call EndLoad() on the document, which nulls out the mParser. So when they ask for the width, the document can't tell the sink to flush and hence to StartLayout().
Created attachment 253999 [details] [diff] [review]
With that addressed
Attachment #253836 - Attachment is obsolete: true
Attachment #253836 - Flags: superreview?(jonas)
Attachment #253836 - Flags: review?(jonas)
Attachment #253999 - Flags: superreview?(jonas)
Attachment #253999 - Flags: review?(jonas)
Summary: Stylesheet loading blocks parser → [FIX]Stylesheet loading blocks parser
Target Milestone: mozilla1.5alpha → mozilla1.9alpha
Comment on attachment 253999 [details] [diff] [review]
With that addressed

Let's try the "who'll get to this first?" approach.  Peter, let Jonas know in this bug if you start reviewing this?  Jonas promises to do the same.
Attachment #253999 - Flags: review?(jonas) → review?(peterv)
Note to self: Need to diff nsXULContentSink.h as well.
Blocks: 333192
Created attachment 261623 [details] [diff] [review]
Updated to tip
Attachment #253999 - Attachment is obsolete: true
Attachment #253999 - Flags: superreview?(jonas)
Attachment #253999 - Flags: review?(peterv)
Attachment #261623 - Flags: superreview?(jonas)
Attachment #261623 - Flags: review?(peterv)
Comment on attachment 261623 [details] [diff] [review]
Updated to tip

>Index: layout/style/nsCSSLoader.cpp

>@@ -1556,33 +1561,27 @@ CSSLoaderImpl::SheetComplete(SheetLoadDa
...
>-  if (mLoadingDatas.Count() == 0 && mPendingDatas.Count() > 0) {
>-    LOG(("  No more loading sheets; starting alternates"));
>-    mPendingDatas.Enumerate(StartAlternateLoads, this);
>-  }

Why is this not needed any more?

>@@ -1686,25 +1684,27 @@ CSSLoaderImpl::LoadStyleLink(nsIContent*
...
>-    return PostLoadEvent(aURL, sheet, aObserver, aParserToUnblock,
>-                         *aIsAlternate);
>+    if (aObserver) {
>+      rv = PostLoadEvent(aURL, sheet, aObserver, *aIsAlternate);
>+    }
>+    return rv;

Initialize rv to NS_OK before the |if|. Or even better, move the return to inside the |if| and add a |return NS_OK| instead. Or change it to |return aObserver ? PostLoadEvent(...) : NS_OK;|

>Index: content/base/src/nsStyleLinkElement.h
>+  NS_IMETHOD UpdateStyleSheet(nsICSSLoaderObserver* aObserver,
>+                              PRBool* aWillNotify,
>+                              PRBool* aIsAlternate);
...
>+  nsresult UpdateStyleSheet(nsIDocument *aOldDocument,
>+                            PRBool aForceUpdate = PR_FALSE);
...
>+  nsresult UpdateStyleSheet(nsIDocument *aOldDocument,
>+                            nsICSSLoaderObserver* aObserver,
>+                            PRBool* aWillNotify,
>+                            PRBool* aIsAlternate,
>+                            PRBool aForceUpdate);

Please don't have three different functions called UpdateStyleSheet on the same class. Makes it hard to see at the callsite which one is called. You could rename the third DoUpdateStyleSheet. Not sure what to call the second, UpdateStyleSheetInternal?

>Index: content/base/src/nsStyleLinkElement.cpp

>@@ -273,46 +280,41 @@ nsStyleLinkElement::UpdateStyleSheet(nsI
>   if (isInline) {
...
>     rv = doc->CSSLoader()->
>       LoadInlineStyle(thisContent, uin, mLineNumber, title, media,
>-                      parser, aObserver, &doneLoading, &isAlternate);
>+                      aObserver, &doneLoading, &isAlternate);
>   }
>   else {
>-    doneLoading = PR_FALSE;  // If rv is success, we won't be done loading; if
>-                             // it's not, this value doesn't matter.
>     rv = doc->CSSLoader()->
>-      LoadStyleLink(thisContent, uri, title, media, isAlternate,
>-                    parser, aObserver, &isAlternate);
>+      LoadStyleLink(thisContent, uri, title, media, isAlternate, aObserver,
>+                    &isAlternate);
>   }
> 
>-  if (NS_SUCCEEDED(rv) && !doneLoading && !isAlternate) {
>-    rv = NS_ERROR_HTMLPARSER_BLOCK;
>+  if (NS_SUCCEEDED(rv)) {
>+    *aWillNotify = !doneLoading;
>+    *aIsAlternate = isAlternate;
>   }
> 
>   return rv;
> }

Please put an NS_ENSURE_SUCCESS(rv, rv) after the first |if| instead of using an ending |return rv;|. I'm allergic to ending |return rv|s as you can tell :) They aren't safe in the face of future changes.

>Index: content/base/src/nsContentSink.cpp
>@@ -705,26 +712,22 @@ nsContentSink::ProcessStyleLink(nsIConte
...
>   rv = mCSSLoader->LoadStyleLink(aElement, url, aTitle, aMedia, aAlternate,
>-                                 parser, this, &isAlternate);
>-  if (NS_SUCCEEDED(rv) && parser && !isAlternate) {
>-    rv = NS_ERROR_HTMLPARSER_BLOCK;
>+                                 this, &isAlternate);
>+  if (NS_SUCCEEDED(rv) && !isAlternate) {
>+    ++mPendingSheetCount;
>+    mScriptLoader->AddExecuteBlocker();
>   }
> 
>   return rv;

Same thing here.

>Index: content/html/document/src/nsHTMLContentSink.cpp
> HTMLContentSink::DidBuildModel(void)
...
>+  // Go ahead and try to start layout now even if there are pending
>+  // sheets, since the document is about to forget about us.
>+  StartLayout(PR_TRUE);

Why is this needed? Same thing in the XML contentsink.

These are just sr comments. Gonna look through the functional changes a bit more for the review.
Attachment #261623 - Flags: review?(peterv) → review?(jonas)
Attachment #261623 - Flags: superreview?(jonas)
Attachment #261623 - Flags: superreview+
Attachment #261623 - Flags: review?(jonas)
Attachment #261623 - Flags: review+
r/sr=sicking
> Why is this not needed any more?

It is.  But it's not needed for all the sheets glommed on that load; it lives in SheetComplete, not DoSheetComplete.

> Why is this needed? Same thing in the XML contentsink.

I thought we needed it to handle DOMContentLoaded stuff, but think you're right -- we don't really need that.

I'll remove that here and from the XML sink.

I'll also address the other comments.
Created attachment 262315 [details] [diff] [review]
Patch to undo minor behavior change -- fixes one of the reftests
Comment on attachment 262315 [details] [diff] [review]
Patch to undo minor behavior change -- fixes one of the reftests

I'd be fine with not propagating OOM errors either. The insert of the PI has still succeeded.

r/sr=me either way.
Attachment #262315 - Flags: superreview+
Attachment #262315 - Flags: review+
Fix checked in.  Doesn't seem to have helped Tp, sadly.  :(
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
This probably wants a Litmus test; I don't know how you'd reliably test it in an automated manner.
Flags: in-testsuite?
You could have a slow-loading stylesheet in a subframe and poke the DOM off an interval in the parent to see what it looks like....

Comment 49

10 years ago
Did this regress tp2?
It's possible, I suppose... Last I checked, the number Tp2 reported had no bearing on reality, so I've been ignoring it.  Did we fix that little problem?
Apparently we did, in November.

The basic difference in Tp2 around this checkin is in the times reported for www.w3.org_DOML2Core: before the checkin they look like:

www.w3.org_DOML2Core/;855.5;847.5;838;887;887;861;838;841;850

and after they look like:

www.w3.org_DOML2Core/;1692.5;1667.25;1642;1724;1712;1673;1724;1642;1642

The latter set is a lot more like what Tp reports, fow what it's worth.  That 800ms differnce basically accounts for the entire jump (once you divided it by 40, which is the number of tests).

So one possibility, I guess, could be that we're calling InitialReflow when we already have a bunch of content, which we reflow right then.  Then we go ahead and restyle, add more content, and reflow again.  So depending on how timing works out we could be doing more work than we used to.

I wonder whether it would make sense to have InitialReflow() just set the size of the root frame to the size of the prescontext, stick it in mDirtyRoots, and post a reflow event...  I tried doing that, but locally I get so much noise in my measurements it's hard to see anything.

Eli, are you planning on touching the InitialReflow() code in bug 370952?  Or should I go ahead and try a patch that does the above and see how tinderbox reacts?

Comment 53

10 years ago
(In reply to comment #52)
> Eli, are you planning on touching the InitialReflow() code in bug 370952?  Or
> should I go ahead and try a patch that does the above and see how tinderbox
> reacts?

I'll be touching InitialReflow eventually... but go ahead and try it.  My patch won't be ready for a while.

Hmm, is that page equivalent to http://www.w3.org/TR/DOM-Level-2-Core/core.html?  I'm looking at that page, and it looks pretty simple: 2 external stylesheets, no scripts.  If we are calling InitialReflow while one of the stylesheets is still loading, that would obviously be more expensive...

There's a possibility that delaying the call to InitialReflow actually makes it more expensive; it might be worth testing what the perf effect is on that page if you make the body block the parser when there are pending stylesheets.
> but go ahead and try it.

I did last night; it seemed to make things even worse.  :(

> Hmm, is that page equivalent to
> http://www.w3.org/TR/DOM-Level-2-Core/core.html?

Yep.  And yes, it's simple.

> If we are calling InitialReflow while one of the stylesheets is still loading

We're not; we're calling it once the second one finishes loading.

> There's a possibility that delaying the call to InitialReflow actually makes
> it more expensive

Sure.  The InitialReflow call should be more expensive than when the document is blank, but cheaper than incremental frame construction...

As for blocking the parser on <body>, that _seems_ like it should be slower in general...   not to mention a bit of a pain.

I've filed bug 378480 on the problem; I'll try profiling both loads and see what they look like...
Depends on: 378480
Created attachment 262522 [details] [diff] [review]
Final patch that was checked in, including followups
Attachment #261623 - Attachment is obsolete: true
Attachment #262315 - Attachment is obsolete: true
Depends on: 378559
Depends on: 378606
Note: Fixing bug 378975 apparently helped the Tp(2) fallout from this bug.
Depends on: 378975
Depends on: 379485

Updated

10 years ago
Depends on: 380028
Depends on: 383331

Updated

10 years ago
Depends on: 387669

Updated

10 years ago
Depends on: 392318
(In reply to comment #43)
> I thought we needed it to handle DOMContentLoaded stuff

And I was sorta right: see bug 392318.
Depends on: 396226
Depends on: 416289
Depends on: 443616
Depends on: 431694
Depends on: 496631
Depends on: 500673
Depends on: 662685
No longer depends on: 662685
You need to log in before you can comment on or make changes to this bug.