219276 - Cache parsed stylesheets

Status: RESOLVED DUPLICATE of bug 1599160

(Core :: CSS Parsing and Computation, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We could put parsed stylesheets in the cache to avoid reparsing them. When
surfing a site you will probably reload the same CSS file(s) for pretty much
every page you visit so being able to just grab a parsed stylesheet seems like a
nice thing.

There are of course some issues. First of all it's a perf vs. memory issue. Is
it reall better to keep other things in the cache then two copies (the normal
filestream and the parsed objects) then to just reparse the file every time? And
is it worth the extra code?

Second the cachecode needs to know the mem-size of the parsed stylesheet. We
could probably estimate this by multiplying the filesize of the stylesheet with
some constant (less then 1).

Third we need code to deal with @import-ed stylesheets when calculating the
expiredate. It shouldn't be too hard though, just take the date closest into the
future. We could also simply skip stylesheets with @import rules. However my
guess is that those are the stylesheets where we would save the most time.

What do you guys think? Could this be worth it? Has anybody seen any profiles
that show if we spend time in the css-loader/parser?

Comment 1

[Jo Hermans]

I think this has been filed before, but I can't find a duplicate.

Related: bug 38486

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

one very important difference to bug 38486 though is that for stylesheets we're
very likly to actually use the cached object since it's probably used on the
next page or pages you go to.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> some constant (less then 1)

Why less than 1?  I would not be surprised if the parsed representation of a
sheet is larger than the original plaintext file...

In addition to @import rules we may need to watch out for background images,
list-style images, etc, depending on what gets done in bug 113173 (we may end up
using imgIRequest objects there instead of nsIURI objects).

Comment 4

[Boris Zbarsky [:bzbarsky]]

*** Bug 269031 has been marked as a duplicate of this bug. ***

Comment 5

[(no longer active)]

Bobby, does stylo make this easier/harder?  Or is it relevant at all to stylo?

Comment 6

[(no longer active)]

bkelly says this has a huge impact on Facebook page load times.

Comment 7

[Bobby Holley (:bholley)]

(In reply to :Ehsan Akhgari from comment #5)
> Bobby, does stylo make this easier/harder?  Or is it relevant at all to
> stylo?

I think stylo is mostly orthogonal to this, though there's enough churn in the style system right now that we might want to hold off on doing more things there if we can avoid it.

Stylo does give us the potential opportunity to parallelize CSS parsing, see bug 1346988. That's probably not going to be in the MVP though, where we're just shooting for parity on parsing performance.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

(In reply to :Ehsan Akhgari from comment #6)
> bkelly says this has a huge impact on Facebook page load times.

I think I mispoke about FB.  I believe I was looking at linkedin.  

This is less important for SPAs, but sites like washingtonpost.com, linkedin, etc that do real navigations between pages end up reloading stylesheets.  Chrome avoids the need to even start a network request which makes these intra-site navigations feel a lot faster.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Note, I think a lot of the wins from this are not on raw cost of http cache lookup or css parse.  Avoiding the need to perform an async operation and schedule another main thread runnable can have important latency benefits.  Our main thread is very congested during loads, so anything we can do to avoid main thread runnables will probably pay off.

Comment 10

[(no longer active)]

Ben, can you please provide an STR that can be profiled, or provide a profile yourself?  Thanks!

Comment 11

[(no longer active)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #7)
> (In reply to :Ehsan Akhgari from comment #5)
> > Bobby, does stylo make this easier/harder?  Or is it relevant at all to
> > stylo?
> 
> I think stylo is mostly orthogonal to this, though there's enough churn in
> the style system right now that we might want to hold off on doing more
> things there if we can avoid it.

Well, I'd like to first see a profile to understand what level of caching we're talking about...  For example, naively thinking here, it seems to me that we could easily extend nsLayoutStylesheetCache with the ability to also cache stylesheets coming from web pages, if we know what the right cache key would be, when to evict entries, etc, and that sounds fairly simple to me and completely doable in parallel to Stylo given that it happens at a layer above Stylo.  I'd be *super* interested to do something along those lines for QF if it wins us some perf and is tractable.

Comment 12

[(no longer active)]

I'd also like to know what David thinks about this, since he may have thought about this before.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

I can't immediately reproduce what I saw previously.  Unfortunately I probably won't have time to investigate further in the near future.  Feel free to ignore my previous statements on this.

Comment 14

[Boris Zbarsky [:bzbarsky]]

The "when to evict entries" bit is somewhat complicated, because of the need to respect HTTP semantics, right?  Imagelib does this by loading a stale thing and then replacing it with the new thing if HTTP says it should actually have changed, but that's a harder sell with CSS, I think.

That said, it's possible other browsers don't respect HTTP semantics here; I don't know.

Comment 15

[(no longer active)]

Let's at least investigate this for QF.

Comment 16

[Jonathan Watt [:jwatt]]

FWIW this impacts https://krakenbenchmark.mozilla.org/ which makes network requests for /kraken.css something like 140 times while running. (Based on bug 1352085 I suspect this bug impacts other Talos tests too.)

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #14)
> Imagelib does this by loading a stale thing and then replacing it
> with the new thing if HTTP says it should actually have changed

(Doing that for CSS is bug 510121.)

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think caching parsed style sheets in an nsExpirationTracker-based cache is a good idea, presuming that we can either respect HTTP semantics across pages, or that we're not the first browser not to do so.

Once we do this, we should also file a followup bug on doing a bug 77999-equivalent in some cases for content style sheets.

Comment 18

[(no longer active)]

Marking as [qf:p1] for an initial investigation on what other browsers do here, and whether we want to decide that we *don't* want to do this for QF.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

We spoke a little bit about higher level caching like this and service workers at the spec meeting.  We have previously done work in image cache to make sure service worker is consulted again instead of using the cache in some places.  The current thinking, though, is its probably ok to allow this higher level caching as long as something like the "cache-control:no-store" header disables the cache.

So, if this is moves forward I think we could avoid having to do anything special in regards to service workers as long as "no-store" is respected.  At least to start, anyway.

Comment 20

[Massimo Fidanza]

Why the activity on this bug stopped a year ago? and never start again?

Comment 21

[Boris Zbarsky [:bzbarsky]]

Two reasons:

1)  It was investigated and decided that it wasn't a hard blocker for the goals we had at the time.  
2)  Time was focused on the new style system instead.

This may still be worth doing, but needs measurements to see how worthwhile it is.

Comment 22

[Bobby Holley (:bholley)]

Another point is that, starting in Firefox 61 (released yesterday), we parse stylesheets off the main thread, in parallel with the other work during pageload. So unless the stylesheets are particularly big, or the user only has a single core, stylesheet parsing is much less likely to be the long pole in page rendering.

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

OMT parsing surely helps, but there is still a price to pay for opening a main thread nsIChannel to load the stylsheet bytes.  In particular, I expect any MT jank that delays the nsIChannel runnables will delay the start of the OMT parsing.  But yea, need measurements.

Comment 24

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Emilio, anything left to do in this bug after bug 1599160?

Comment 25

[Emilio Cobos Álvarez (:emilio)]

Nope, this looks like a dupe of that!