Measure style sheets only visible from Loader

RESOLVED FIXED in Firefox 18

Status

()

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

People

(Reporter: njn, Assigned: bz)

Tracking

unspecified
mozilla19
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox18 fixed, firefox19 fixed)

Details

(Whiteboard: [MemShrink])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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)
(Reporter)

Updated

5 years ago
Whiteboard: [MemShrink]
(Reporter)

Updated

5 years ago
Depends on: 768901
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.
Created attachment 670262 [details] [diff] [review]
Do memory reporting for stylesheets that only the CSS loader might know about.
Attachment #670262 - Flags: review?(n.nethercote)
Attachment #670262 - Flags: review?(dbaron)
Whiteboard: [MemShrink] → [need review][MemShrink]
(Reporter)

Comment 3

5 years ago
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.
Attachment #670262 - Flags: review?(n.nethercote) → review-
> 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.
Created attachment 670551 [details] [diff] [review]
Do memory reporting for stylesheets that only the CSS loader might know about.
Attachment #670551 - Flags: review?(n.nethercote)
Attachment #670551 - Flags: review?(dbaron)
Attachment #670262 - Attachment is obsolete: true
Attachment #670262 - Flags: review?(dbaron)
(Reporter)

Comment 6

5 years ago
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.
Attachment #670551 - Flags: review?(n.nethercote) → review+
(Reporter)

Comment 7

5 years ago
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.
> 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.
> 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.
Looks like it's not wasted, because presumably they're not poking the CSSOM.  OK, that's better.
Created attachment 670669 [details] [diff] [review]
With Nicholas' comments addressed.
Attachment #670669 - Flags: review?(dbaron)
Attachment #670551 - Attachment is obsolete: true
Attachment #670551 - Flags: review?(dbaron)
Comment on attachment 670669 [details] [diff] [review]
With Nicholas' comments addressed.

r=dbaron
Attachment #670669 - Flags: review?(dbaron) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/557cfb0bdc39
Flags: in-testsuite?
Whiteboard: [need review][MemShrink] → [MemShrink]
Target Milestone: --- → mozilla19
https://hg.mozilla.org/mozilla-central/rev/557cfb0bdc39
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Comment 15

5 years ago
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
Attachment #670669 - Flags: approval-mozilla-aurora?

Updated

5 years ago
Attachment #670669 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/30941fc8d850
status-firefox18: --- → fixed
status-firefox19: --- → fixed
Depends on: 811021
You need to log in before you can comment on or make changes to this bug.