Last Comment Bug 799816 - Be smarter about putting preloaded sheets in the CSSLoader hashtables
: Be smarter about putting preloaded sheets in the CSSLoader hashtables
Status: RESOLVED FIXED
[MemShrink:P3]
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: unspecified
: x86 Mac OS X
-- normal (vote)
: mozilla23
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
: Jet Villegas (:jet)
Mentors:
Depends on: 867487
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-09 20:31 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2013-05-13 17:09 PDT (History)
7 users (show)
bzbarsky: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase, of sorts. You'll need a big CSS file (199 bytes, text/html)
2012-10-11 14:59 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
Try to not have preload sheets (or other unreferenced sheets) hanging out in our mCompleteSheets hashtable. (4.09 KB, patch)
2012-10-11 15:16 PDT, Boris Zbarsky [:bz] (still a bit busy)
dbaron: review+
Details | Diff | Splinter Review

Description User image Boris Zbarsky [:bz] (still a bit busy) 2012-10-09 20:31:22 PDT
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.
Comment 1 User image Boris Zbarsky [:bz] (still a bit busy) 2012-10-11 14:59:11 PDT
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.
Comment 2 User image Boris Zbarsky [:bz] (still a bit busy) 2012-10-11 15:16:22 PDT
Created attachment 670563 [details] [diff] [review]
Try to not have preload sheets (or other unreferenced sheets) hanging out in our mCompleteSheets hashtable.
Comment 3 User image David Baron :dbaron: ⌚️UTC-8 2012-12-31 13:34:48 PST
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.
Comment 4 User image John Daggett (:jtd) 2013-04-07 21:42:51 PDT
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.
Comment 5 User image Nicholas Nethercote [:njn] 2013-04-07 21:49:26 PDT
/me sighs loudly.
Comment 6 User image Boris Zbarsky [:bz] (still a bit busy) 2013-04-08 08:55:43 PDT
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... ;)
Comment 7 User image David Baron :dbaron: ⌚️UTC-8 2013-04-22 05:02:42 PDT
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
Comment 8 User image Boris Zbarsky [:bz] (still a bit busy) 2013-04-24 12:05:49 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6a0fd976a43
Comment 9 User image Ryan VanderMeulen [:RyanVM] 2013-04-24 18:56:44 PDT
https://hg.mozilla.org/mozilla-central/rev/c6a0fd976a43

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