Last Comment Bug 799796 - Measure style sheets only visible from Loader
: Measure style sheets only visible from Loader
Status: RESOLVED FIXED
[MemShrink]
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla19
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
: Jet Villegas (:jet)
Mentors:
Depends on: NewDMD 768901 811021
Blocks: B2GDarkMatter
  Show dependency treegraph
 
Reported: 2012-10-09 18:42 PDT by Nicholas Nethercote [:njn]
Modified: 2012-11-12 22:19 PST (History)
7 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
Testcase (142 bytes, text/html)
2012-10-10 22:47 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
Do memory reporting for stylesheets that only the CSS loader might know about. (5.03 KB, patch)
2012-10-10 22:52 PDT, Boris Zbarsky [:bz] (still a bit busy)
n.nethercote: review-
Details | Diff | Splinter Review
Do memory reporting for stylesheets that only the CSS loader might know about. (5.36 KB, patch)
2012-10-11 14:49 PDT, Boris Zbarsky [:bz] (still a bit busy)
n.nethercote: review+
Details | Diff | Splinter Review
With Nicholas' comments addressed. (5.48 KB, patch)
2012-10-11 19:24 PDT, Boris Zbarsky [:bz] (still a bit busy)
dbaron: review+
bajaj.bhavana: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Nicholas Nethercote [:njn] 2012-10-09 18:42:27 PDT
DMD shows this for B2G, and similar (but smaller) amounts for desktop:

00.67% (206,480B) operator new (layout/style/../../dist/include/mozilla/mozalloc.h:200)
00.65% (200,080B) ParseSelector (/home/njn/moz/mi8/layout/style/nsCSSParser.cpp:3790)
00.47% (144,400B) ParseSelectorList (/home/njn/moz/mi8/layout/style/nsCSSParser.cpp:2787)
00.47% (144,400B) ParseRuleSet (/home/njn/moz/mi8/layout/style/nsCSSParser.cpp:2750)
00.45% (140,080B) ParseSheet (/home/njn/moz/mi8/layout/style/nsCSSParser.cpp:936)
00.45% (140,080B) mozilla::css::Loader::ParseSheet() (/home/njn/moz/mi8/layout/style/Loader.cpp:1627)
00.45% (140,080B) mozilla::css::SheetLoadData::OnStreamComplete() (/home/njn/moz/mi8/layout/style/Loader.cpp:957)
00.45% (140,080B) nsUnicharStreamLoader::OnStopRequest() (/home/njn/moz/mi8/netwerk/base/src/nsUnicharStreamLoader.cpp:95)
00.36% (112,160B) nsJARChannel::OnStopRequest() (/home/njn/moz/mi8/modules/libjar/nsJARChannel.cpp:888)
00.36% (112,160B) nsInputStreamPump::OnStateStop() (/home/njn/moz/mi8/netwerk/base/src/nsInputStreamPump.cpp:553)
00.36% (112,160B) nsInputStreamPump::OnInputStreamReady() (/home/njn/moz/mi8/netwerk/base/src/nsInputStreamPump.cpp:375)
00.36% (112,160B) nsInputStreamReadyEvent::Run() (/home/njn/moz/mi8/xpcom/io/nsStreamUtils.cpp:83)
00.36% (112,160B) nsThread::ProcessNextEvent() (/home/njn/moz/mi8/xpcom/threads/nsThread.cpp:595)
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2012-10-10 22:47:41 PDT
Created attachment 670255 [details]
Testcase

about:memory for this page shows 0 memory used for stylesheets, without the patch I'm about to attach.  With that patch, it shows 0.08MB used.
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2012-10-10 22:52:36 PDT
Created attachment 670262 [details] [diff] [review]
Do memory reporting for stylesheets that only the CSS loader might know about.
Comment 3 Nicholas Nethercote [:njn] 2012-10-11 02:41:20 PDT
Comment on attachment 670262 [details] [diff] [review]
Do memory reporting for stylesheets that only the CSS loader might know about.

Review of attachment 670262 [details] [diff] [review]:
-----------------------------------------------------------------

Looks mostly good, but I'd like to see the Loader change suggested below.

::: layout/style/Loader.cpp
@@ +2441,5 @@
> +  return PL_DHASH_NEXT;
> +}
> +
> +size_t
> +Loader::SizeOfCachedStylesheets(nsMallocSizeOfFun aMallocSizeOf) const

Is there any rule as to when "Stylesheet" is used and when "StyleSheet" is used?  I see both being used all over the place.

@@ +2448,5 @@
> +  if (mCompleteSheets.IsInitialized()) {
> +    // Should we be using mCompleteSheets.SizeOfIncludingThis instead?  That is,
> +    // should we include the hashtable's storage in this measurement?  Seems
> +    // like we shouldn't, if we're claiming to be measuring the memory used by
> +    // stylesheets per se.

Since mCompleteSheets is not a pointer, we would use mCompleteSheets.SizeOfExcludingThis().

However, to be thorough we should instead measure the entire Loader as well as the storage allocated for mComplete.  I can live with those bytes being lumped into "style-sheets", esp. since they're likely to be fairly small.

(Having said that, if you ever feel like splitting the "style-sheets" number into multiple would be useful, please feel free to do so or make suggestions on how to do so.)

Can you therefore change this to Loader::SizeOfIncludingThis(), and document which fields are measured and which aren't, a la http://mxr.mozilla.org/mozilla-central/source/layout/style/StyleRule.cpp#820 ?

::: layout/style/Loader.h
@@ +360,5 @@
>    // should only be called from the document that owns this loader.
>    void UnlinkCachedSheets();
>  
> +  // Measure the size of the stylesheets hanging out in our cached
> +  // sheets hashtable

Nit:  add a period.
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2012-10-11 09:01:30 PDT
> Is there any rule as to when "Stylesheet" is used and when "StyleSheet" is used?

I think the DOM tends to use "Stylesheet" (as do the DOM specs) and the style system tends to use "StyleSheet".  :(

> Can you therefore change this to Loader::SizeOfIncludingThis()

Will do.
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2012-10-11 14:49:12 PDT
Created attachment 670551 [details] [diff] [review]
Do memory reporting for stylesheets that only the CSS loader might know about.
Comment 6 Nicholas Nethercote [:njn] 2012-10-11 18:50:56 PDT
Comment on attachment 670551 [details] [diff] [review]
Do memory reporting for stylesheets that only the CSS loader might know about.

Review of attachment 670551 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsDocument.cpp
@@ +10040,5 @@
> +    mAdditionalSheets[eUserSheet].
> +      SizeOfExcludingThis(SizeOfStyleSheetsElementIncludingThis,
> +                          aWindowSizes->mMallocSizeOf);
> +  aWindowSizes->mStyleSheets +=
> +    CSSLoader()->SizeOfIncludingThis(aWindowSizes->mMallocSizeOf);

You could add a comment saying that lumping in Loader in with the style-sheets size isn't ideal, but that it doesn't seem worth separating it for now.

::: layout/style/Loader.h
@@ +360,5 @@
>    // should only be called from the document that owns this loader.
>    void UnlinkCachedSheets();
>  
> +  // Measure the size of the stylesheets hanging out in our cached
> +  // sheets hashtable

Nit:  Add a period at the end of the sentence.
Comment 7 Nicholas Nethercote [:njn] 2012-10-11 18:55:30 PDT
I ran DMD over this patch, after instrumenting each of the four new counting places individually.  Here is the summary of the results for the Loader counting:

- desktop, about:home:      r00> 00.01% (3,648B) loader
- desktop, techcrunch.com:  r00> 01.38% (1,031,920B) loader
- B2G startup:              r00> 02.42% (657,272B) loader

Nice!  And no double-counting.

The new code for measuring mAdditionalSheets and mCatalogSheets gave us zero additional bytes.  Hopefully this is expected.
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2012-10-11 19:12:35 PDT
> Nice!  And no double-counting.

Yes, but those numbers are HUGE.  That's 1MB of memory that's basically wasted, on techcrunch.com.

How do those numbers look with bug 799816 also applied, if I might ask?

> Hopefully this is expected.

Yep.  mAdditionalSheets is there for extensions to add stuff, so you would only hit it if you had such an extension.  And mCatalogSheets would only get things if you looked at MathML, I believe.
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2012-10-11 19:13:29 PDT
> How do those numbers look with bug 799816 also applied, if I might ask?

Actually, I can test techcrunch on desktop myself with that.  Let me do it.
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2012-10-11 19:20:45 PDT
Looks like it's not wasted, because presumably they're not poking the CSSOM.  OK, that's better.
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2012-10-11 19:24:45 PDT
Created attachment 670669 [details] [diff] [review]
With Nicholas' comments addressed.
Comment 12 David Baron :dbaron: ⌚️UTC-10 2012-10-16 16:18:05 PDT
Comment on attachment 670669 [details] [diff] [review]
With Nicholas' comments addressed.

r=dbaron
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2012-10-17 14:04:46 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/557cfb0bdc39
Comment 14 Ryan VanderMeulen [:RyanVM] 2012-10-17 19:20:36 PDT
https://hg.mozilla.org/mozilla-central/rev/557cfb0bdc39
Comment 15 Nicholas Nethercote [:njn] 2012-10-26 12:49:30 PDT
Comment on attachment 670669 [details] [diff] [review]
With Nicholas' comments addressed.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A

User impact if declined: lesser understanding of B2G memory consumption for developers.

Testing completed (on m-c, etc.): has been on m-c for 9 days without problem.

Risk to taking this patch (and alternatives if risky): minimal;  the code only runs when viewing about:memory or triggering a memory reporter dump.

String or UUID changes made by this patch: none
Comment 16 Ryan VanderMeulen [:RyanVM] 2012-10-27 17:02:53 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/30941fc8d850

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