Closed Bug 84582 Opened 23 years ago Closed 17 years ago

[FIX]Stylesheet loading blocks parser

Categories

(Core :: CSS Parsing and Computation, defect, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9alpha1

People

(Reporter: ian, Assigned: bzbarsky)

References

(Blocks 2 open bugs,
URL
)

Details

(4 keywords, Whiteboard: [Hixie-P2][Hixie-2.0])

Attachments

(1 file, 5 obsolete files)

Final patch that was checked in, including followups
70.72 KB, patch
Details | Diff | Splinter Review 
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
Keywords: css1, mozilla1.0, perf, regression, testcase
Whiteboard: [Hixie-P2]
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
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
Blocks: 79021
Keywords: regression
Note the specific comments in the initial description of this bug. Frame
creation should not be put off forever. There should be a timeout.
Blocks: 137159
No longer blocks: 137159
Whiteboard: [Hixie-P2] → [Hixie-P2][Hixie-2.0]
Blocks: 137159
Is bug 123319 a dup of this one?
Assigning pierre's remaining Style System-related bugs to myself.
Assignee: pierre → dbaron
Status: ASSIGNED → NEW
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
Blocks: 203448
Blocks: 204086
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....
> 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?  :)
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
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....
Hyatt: Can you give us the URI so we can see how Opera deals with this?
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.
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.
> 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.
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
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.
Attached patch What I have in my tree right now (obsolete) — — Splinter Review 

    
    

    
  
http://webkit.org/blog/?p=66 talks about this issue.

    
  
Blocks: 360467

    
  
Depends on: incrementalxml

    
    

    
  

      
      
      
      

        Attached patch
          
          
        I think this is set to go (obsolete)
        — 
          — Splinter Review
      

    


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

    
    

    
  

      
      
      
      

        Attached patch
          
          
        With that addressed (obsolete)
        — 
          — Splinter Review
      

    


  

        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

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Updated to tip (obsolete)
        — 
          — Splinter Review
      

    


  

        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+

    
    

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

    
    

    
  
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
Closed: 17 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....

    
    

    
  
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?

    
    

    
  
(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

    
    

    
  


  

        Attachment #261623 -
        Attachment is obsolete: true

        Attachment #262315 -
        Attachment is obsolete: true

    
  
Depends on: 378559

    
    

    
  
Note: Fixing bug 378975 apparently helped the Tp(2) fallout from this bug.
Depends on: 378975
Depends on: 379485
Depends on: 380028

    
  
Depends on: 383331
Depends on: 387669
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: 662685

    
  
No longer depends on: 662685

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: