799796 - Measure style sheets only visible from Loader

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Nicholas Nethercote [:njn] (On Long Service Leave)]

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 [:bzbarsky]]

Attachment: 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 [:bzbarsky]]

Attachment: Do memory reporting for stylesheets that only the CSS loader might know about.

Comment 3

[Nicholas Nethercote [:njn] (On Long Service Leave)]

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 [:bzbarsky]]

> 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 [:bzbarsky]]

Attachment: Do memory reporting for stylesheets that only the CSS loader might know about.

Comment 6

[Nicholas Nethercote [:njn] (On Long Service Leave)]

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] (On Long Service Leave)]

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 [:bzbarsky]]

> 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 [:bzbarsky]]

> 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 [:bzbarsky]]

Looks like it's not wasted, because presumably they're not poking the CSSOM.  OK, that's better.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: With Nicholas' comments addressed.

Comment 12

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

Comment on attachment 670669 [details] [diff] [review]
With Nicholas' comments addressed.

r=dbaron

Comment 13

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/557cfb0bdc39

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/557cfb0bdc39

Comment 15

[Nicholas Nethercote [:njn] (On Long Service Leave)]

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]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/30941fc8d850