The default bug view has changed. See this FAQ.

Be smarter about putting preloaded sheets in the CSSLoader hashtables

RESOLVED FIXED in mozilla23

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

unspecified
mozilla23
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P3])

Attachments

(2 attachments)

Right now, if we start a sheet preload, then coalesce with the real load, the preloaded sheet ends up in the CSSLoader hashtable.  Then if the page uses CSSOM to touch the DOM-visible clone, we end up with twice the memory usage we would have had if we'd had that clone in the hashtable and the preloaded sheet not held on to by anything.

Should see what we can do better here to avoid that.
Whiteboard: [MemShrink]
Created attachment 670557 [details]
Testcase, of sorts.  You'll need a big CSS file

I see the stylesheet memory for that testcase be 150KB using my test sheet.  It's 80KB if I comment out that one line of JS in the load handler.
Created attachment 670563 [details] [diff] [review]
Try to not have preload sheets (or other unreferenced sheets) hanging out in our mCompleteSheets hashtable.
Attachment #670563 - Flags: review?(dbaron)
Whiteboard: [MemShrink] → [MemShrink:P3]
Comment on attachment 670563 [details] [diff] [review]
Try to not have preload sheets (or other unreferenced sheets) hanging out in our mCompleteSheets hashtable.

I've been hoping I could get John to take over doing code reviews for the CSS loader (which is a relatively small piece of code, with the bulk of the changes made by bz), since it's something that I never remember much about and have to read up on every time I review code in it.  Since I figure that the CSS loader bears a decent amount of similarity to the Font Face loader, which John knows a good bit about, I thought he'd be a decent candidate.

If you don't like this idea, feel free to transfer the review request back to me.
Attachment #670563 - Flags: review?(dbaron) → review?(jdaggett)
Whiteboard: [MemShrink:P3] → [need review][MemShrink:P3]

Comment 4

4 years ago
Comment on attachment 670563 [details] [diff] [review]
Try to not have preload sheets (or other unreferenced sheets) hanging out in our mCompleteSheets hashtable.

Looking over this code, I don't really feel qualified at this point to act as a reviewer.  Sorry for not looking at this sooner.
Attachment #670563 - Flags: review?(jdaggett)
/me sighs loudly.
Comment on attachment 670563 [details] [diff] [review]
Try to not have preload sheets (or other unreferenced sheets) hanging out in our mCompleteSheets hashtable.

Alright, let's try Kyle.  He's a sucker for being peer/owner on random code... ;)
Attachment #670563 - Flags: review?(khuey)
Comment on attachment 670563 [details] [diff] [review]
Try to not have preload sheets (or other unreferenced sheets) hanging out in our mCompleteSheets hashtable.

I can do it; I'd just been trying to reduce the areas of code where I'm the primary reviewer by redirecting those where I don't feel like I have particular expertise in the area.

r=dbaron
Attachment #670563 - Flags: review?(khuey) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6a0fd976a43
Flags: in-testsuite-
Whiteboard: [need review][MemShrink:P3] → [MemShrink:P3]
Target Milestone: --- → mozilla23
https://hg.mozilla.org/mozilla-central/rev/c6a0fd976a43
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Depends on: 867487
You need to log in before you can comment on or make changes to this bug.