46129 - implement finer-grained sharing in XUL prototype cache

Status: NEW

(Core :: XUL, defect, P3)

Description

[Chris Waterson]

Currently, the XUL prototype shares XUL, JS and CSS at the level of a XUL 
document. In other words, we have one entry in the cache for each XUL document 
that is loaded, and the JS and CSS files are "attached" to that entry.

Because of this, warren & dougt discovered (in bug 46045) that many CSS and JS 
files are loaded repeatedly because they are "included" from several different 
overlay files.

It seems that the right thing to do here is to create additional caches (or, 
augment the existing "prototype cache") to allow JS and CSS to be cached 
independently.

This will improve startup and window-open time, as well as reduce overall 
footprint. According to dougt's trace in bug 46045, we load 96 JS and CSS 
files. Of those, 65 are unique. That means that, on startup, we are re-parsing 
31 files! In other words, a third of the files are read >1 time!

Comment 1

[Chris Waterson]

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

Comment 2

[David Hyatt]

I thought scripts and stylesheets were cached independently.

Comment 3

[Chris Waterson]

Boioioioing. I just re-read this code, because it's been a while...

Out-of-line scripts are not independently cached (e.g., by the script's src= 
URL). I'm pretty sure that we could & should do this.

Style sheets look like they *should* be independently cached (contrary to my 
ramblings in the original report). I checked the content sink and XUL document 
code, and both look like they try to look up the sheet in the cache. The code 
looks like it would only load it from the filesystem if it can't find the sheet 
in the cache. For some reason, it's not working (see dougt's data in bug 46045): 
we are definitely re-parsing style sheets.

Comment 4

[David Hyatt]

One thing I remembered... we never hooked into the style system to cache style 
sheets that are imported BY OTHER STYLE SHEETS.  Make sure you aren't just 
running into that.

e.g., we might keep loading global.css because other stylesheets import it.

Comment 5

[David Hyatt]

I've got this one.

Observations on the navigator window: 
(1) It's the sidebar that loads duplicate files.  If you have the sidebar 
closed, no redundant stylesheets or scripts are loaded.  This is pretty sweet.
(2) The sidebar sucks in scripts and stylesheets that were already loaded by 
navigator.xul's load.  They are the exact two problems mentioned here, namely 
not caching scripts independently and the @import problem with stylesheets.

Once I take care of this, we should see a boost when loading windows of 
different types, e.g., messenger.css won't waste time reloading the global 
and communicator stylesheets once you've loaded navigator.

Comment 6

[Chris Waterson]

cc'ing pierre, because we talked with him about this today.

Comment 7

[David Hyatt]

Attachment: Patch to ensure all @import stylesheets are cached.

Comment 8

[Chris Waterson]

hyatt: looks good to me. but pierre or attinasi will probably want to review.

Comment 9

[Marc Attinasi]

Looking at the patch I notice a couple of things I'd like to bring up: 

1) Coupling the style system to XUL without '#ifdef INCLUDE_XUL' seems like it 
might be a mistake. I think CSSFrameConstructor is currently the only CSS class 
coupled to XUL and it is in #ifdef.  We should keep the CSS code free of 
specific knowledge of higher-level concepts like XUL, but at least #ifdef the 
use of XUL in the proposed patch for now. (If you think I am off-base in my 
desire to limit the coupling to XUL please state your case...)

2) (minor) The strings that represent the component URI's 
("component://netscape/rdf/xul-content-utils" and 
"component://netscape/rdf/xul-prototype-cache") should be constants defined once 
and used twice.

These points aside, the change looks good.

Comment 10

[David Hyatt]

I seem to have caused a refcounting problem, so I'll have to look at this 
further.

Comment 11

[David Hyatt]

Attachment: Better patch. Added #ifdef and cloned cache-hit child import sheets.

Comment 12

[Pierre Saslawsky]

Thanks for copying me on this. I'm looking into it.

Comment 13

[Matthias Kerkhoff]

This one may depend on / be related to bug #29370.

Comment 14

[David Hyatt]

I'm ready to check in the CSS loader patch once I'm given the word from the CSS 
guys (pierre, attinasi, is the new patch good to go?)  It's working great on my 
machine and ensuring that our @imported chrome CSS gets into the XUL cache like 
the other stylesheets.

Comment 15

[Marc Attinasi]

The patch looks fine to me - thanks for #ifdef'ing the XUL logic. 

I guess the cloning of the sheets is necessary to prevent a refcount probem that 
you mentioned? I don't quite get it though. It looks like you are getting a 
sheet from the cache, then cloning it, and then putting the cloned sheet into 
the cache (replacing the first sheet?). I guess I don't understand how the cache 
works. Anyway, the changes look fine if the cloning is really what you need to 
do (if you could explain it to me off-line I'd appreciate it).

Comment 16

[Pierre Saslawsky]

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

Comment 17

[Pierre Saslawsky]

I partially agree with Marc. I think it is correct to clone the stylesheet out of 
the cache but:
- Why do we cache the cloned stylesheet too?
- Shouldn't we remove the stylesheet from the cache when it is deleted? Or how 
does the cache work in that regard?

Comment 18

[David Hyatt]

Yeah, there's a dup issue in the caching that occurs because I haven't removed 
the style sheet caching code from the XUL side, i.e., the CSS loader is the only 
one that really needs to cache now.

I believe cloning gets involved because the parent sheets are different for two 
instances of the same stylesheet, so you have to clone the child.

or something like that... :)

Since it sounds good, I'm gonna check this in now becuase of the obvious 
performance win (and the nsbeta3+ marking), and then I'll go ahead and fine-tune 
it on the mozilla/rdf/content side of things to prevent the dup problem.

Comment 19

[David Hyatt]

To answer your question, pierre, the chrome cache right now holds onto the 
stylesheets for the lifetime of the app (unless flushed out of the cache, which 
happens on a skin switch).

We will be implementing memory pressure APIs that will enable you to flush 
stylesheets selectively and to make the cache a little less of a memory-gobbler.

Comment 20

[David Hyatt]

I'm also  going to handle putting XBL into the chrome cache with this bug... so 
it now has three parts, CSS, JS, and XBL.  The first part is fixed.  The JS and 
XBL fixes are still to come.

Comment 21

[David Hyatt]

Ok, XBL now uses the chrome cache for chrome/resource URLs.

Comment 22

[David Hyatt]

In an effort to spread love and sweet cheer, I'm gonna leave the JS caching to 
you, waterson.  Enjoy.

Comment 23

[Chris Waterson]

Fixing the XUL cache to cache JS will save us from loading one extra JS file out
of 22 when bringing up the first navigator window.

(FWIW, The file is chrome://global/content/builtinURLs.js, which probably is
only being loaded twice because it's a bug in the inclusion scheme. It's
included from both navigator.xul and utilityOverlay.xul. ben, is this necessary?
I'll file a separate bug if not.)

I'm going to future and [nsbeta3-] the "remnants" of this bug. We may want to
cache JS individually some day, but for now it's not going to buy us much.

Comment 24

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.