Last Comment Bug 760331 - Consider coalescing inline style rules across nodes when possible
: Consider coalescing inline style rules across nodes when possible
Status: RESOLVED FIXED
[MemShrink:P2]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla18
Assigned To: Kyle Huey [:khuey] (khuey@mozilla.com)
:
Mentors:
Depends on:
Blocks: 686795 826541
  Show dependency treegraph
 
Reported: 2012-05-31 17:12 PDT by Boris Zbarsky [:bz]
Modified: 2013-01-03 16:41 PST (History)
17 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (101.94 KB, patch)
2012-09-27 10:58 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
bzbarsky: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2012-05-31 17:12:36 PDT
See discussion in bug 686795 especially bug 686795 comment 23 through bug 686795 comment 29.

As far as that last comment, comparing rules means possibly-better sharing but more memory used to store the actual strings.  Comparing strings means possibly-worse sharing but ability to share the strings too.

I suspect, with no data to base this on, that it's rare to have different strings but still equal rules...
Comment 1 Boris Zbarsky [:bz] 2012-05-31 17:13:26 PDT
Oh, one more thing: we'll need to be a bit careful about mutations, but the fact that rules are usually immutable anyway will be a win.  We'll just have to flag the rule as "used" when we share it.
Comment 2 David Baron :dbaron: ⌚️UTC+1 (mostly busy through August 4; review requests must explain patch) 2012-05-31 17:17:03 PDT
How do we stop caching the rules that are no longer used?
Comment 3 Boris Zbarsky [:bz] 2012-05-31 17:25:58 PDT
One solution is to hold a weak ref to a refcounted struct of some sort, and that struct removes itself from the cache when its refcount goes to 0...
Comment 4 Jonas Sicking (:sicking) PTO Until July 5th 2012-06-01 00:09:45 PDT
Given that the case we are concerned about is server-side tools that generate the same rule for tons of elements (rather than using a class for example), I think we will be able to catch most cases by hashing the string value.
Comment 5 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-09-24 20:19:58 PDT
I have mostly complete patches for this.
Comment 6 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-09-27 10:58:21 PDT
Created attachment 665541 [details] [diff] [review]
Patch

Parts of this are a bit messy.  It might be cleaner if we started treating MiscContainer less like a plain old struct and more like a real class.  Interested to hear feedback though.  (This is green on try).
Comment 7 Nathan Froyd [:froydnj] 2012-09-27 11:14:25 PDT
Comment on attachment 665541 [details] [diff] [review]
Patch

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

::: layout/style/nsCSSRules.cpp
@@ +71,5 @@
> +nsHTMLCSSStyleSheet*
> +Rule::GetHTMLStyleSheet() const
> +{
> +  if (mSheet & 0x1) {
> +    return reinterpret_cast<nsHTMLCSSStyleSheet*>(mSheet & ~0x1);

I am fairly certain that you want ~uintptr_t(0x1) here.
Comment 8 Boris Zbarsky [:bz] 2012-09-27 21:22:52 PDT
Comment on attachment 665541 [details] [diff] [review]
Patch

>+++ b/content/base/src/nsAttrValue.cpp
>+MiscContainer::Cache()
>+  nsString str;

You could probably just use an nsDependentString here and Rebind() it as needed to avoid the atomic addref/release if you care.  But this is fine too.

>+  if (static_cast<nsAttrValue::ValueBaseType>(mStringBits &
>+                                              NS_ATTRVALUE_BASETYPE_MASK) ==
>+      nsAttrValue::eStringBase) {

This, on the other hand, is insane.  How about we add methods like StringBitsAreAtom() and StringBitsAreStringbuffer() to MiscContainer and use here and elsewhere we do this ludicrious cast-and-bitwise-stuff bit?  ;)

>+MiscContainer::Evict()
>+  void* ptr = MISC_STR_PTR(this);
>+  if (!ptr) {

How can we be cached if !ptr?  Seems like we should MOZ_ASSERT(ptr) instead?

>+  nsString str;

how about we just factor out this "get mStringBits into an nsString" code into a method on MiscContainer too?  Then you can use it in both your new methods as well as nsAttrValue::ToString?

>@@ -114,18 +192,29 @@ nsAttrValue::Reset()
>+        // If the refcount is now zero, we need to evict from the cache and
>+        // delete the MiscContainer.
>+        if (!cont->Release()) {

I'd prefer |cont->Release() == 0| to make it clear that it's a number, not a boolean.

>+nsAttrValue::ParseStyleAttribute(const nsAString& aString,
>+  bool cachingAllowed = sheet && baseURI == docURI;

Worth documenting why you need this check.  And in particular why this is not comparing to the document's base URI.  Presumably because that can in fact change?

We may also want to assert aElement->NodePrincipal() == ownerDoc->NodePrincipal() here.

>+    MiscContainer* cont =
>+      reinterpret_cast<MiscContainer*>(sheet->LookupStyleAttr(aString));

LookupStyleAttr returns MiscContainer*, so no need to cast.

>+nsAttrValue::ClearMiscContainer()
>+      SetPtrValueAndType(cont, eOtherBase);
>+      cont->mType = eColor;
>+      cont->mStringBits = 0;
>+      cont->mValue.mColor = 0;
>+      cont->mValue.mRefCount = 0;
>+      cont->mValue.mCached = 0;

We have another copy of this in EnsureEmptyMiscContainer already.  Can we possibly factor it out?  Like into the MiscContainer constructor?

Also, should this method be called ClearMiscContainerIfAny?

>+nsAttrValue::EnsureEmptyMiscContainer()
>+    cont = GetMiscContainer();

Why do you need to reget it?

>   cont->mType = eColor;

You only need to do these bits in the case when you just allocated it, right?

>+++ b/content/base/src/nsAttrValue.h

>-  nsAttrValue();

Why did these get moved?

>+  /**
>+   *
>+   */
>+  bool ParseStyleAttribute(const nsAString& aString,

That comment block begs to be filled in!

>@@ -423,17 +398,18 @@ private:
>+  MiscContainer* ClearMiscContainer();
>+  MiscContainer* EnsureEmptyMiscContainer();

Please document (esp. that ClearMiscContainer() will return null if there isn't one already!).

>- * Implementation of inline methods

Why not move all of these to the inlines header instead of only moving the ones that currently use MiscContainer?

>+++ b/dom/media/MediaManager.cpp

The changes here seem pretty unrelated to this patch.  Please nix.

>+++ b/layout/style/nsCSSRules.cpp
>+Rule::GetHTMLStyleSheet() const
>+    return reinterpret_cast<nsHTMLCSSStyleSheet*>(mSheet & ~0x1);

I think you need to cast the 0x1 to the right (unsigned, and correct width) typebefore the bitwise negation op.

I think the getter/setter here should be called Get/SetHTMLCSSStyleSheet, because we have a totally different class called nsHTMLStyleSheet.

>+++ b/layout/style/nsHTMLCSSStyleSheet.cpp
>+ClearAttrCache(const nsAString& aKey, MiscContainer*& aValue, void*)

Ah, excellent.  I'd been wondering about the lifetime management and adoption and stuff.  I think this works out.  ;)

>+nsHTMLCSSStyleSheet::EvictStyleAttr(const nsAString& aSerialized,
>+#ifdef DEBUG
>+#endif

Nix, please.

r=me with those nits.  Looks great!
Comment 9 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-09-30 09:49:17 PDT
(In reply to Boris Zbarsky (:bz) from comment #8)
> Comment on attachment 665541 [details] [diff] [review]
> Patch
> 
> >+++ b/content/base/src/nsAttrValue.cpp
> >+MiscContainer::Cache()
> >+  nsString str;
> 
> You could probably just use an nsDependentString here and Rebind() it as
> needed to avoid the atomic addref/release if you care.  But this is fine too.

Done.

> >+  if (static_cast<nsAttrValue::ValueBaseType>(mStringBits &
> >+                                              NS_ATTRVALUE_BASETYPE_MASK) ==
> >+      nsAttrValue::eStringBase) {
> 
> This, on the other hand, is insane.  How about we add methods like
> StringBitsAreAtom() and StringBitsAreStringbuffer() to MiscContainer and use
> here and elsewhere we do this ludicrious cast-and-bitwise-stuff bit?  ;)

Done (more or less).

> >+MiscContainer::Evict()
> >+  void* ptr = MISC_STR_PTR(this);
> >+  if (!ptr) {
> 
> How can we be cached if !ptr?  Seems like we should MOZ_ASSERT(ptr) instead?

Done.

> >+  nsString str;
> 
> how about we just factor out this "get mStringBits into an nsString" code
> into a method on MiscContainer too?  Then you can use it in both your new
> methods as well as nsAttrValue::ToString?

Done.

> >@@ -114,18 +192,29 @@ nsAttrValue::Reset()
> >+        // If the refcount is now zero, we need to evict from the cache and
> >+        // delete the MiscContainer.
> >+        if (!cont->Release()) {
> 
> I'd prefer |cont->Release() == 0| to make it clear that it's a number, not a
> boolean.

Done.

> >+nsAttrValue::ParseStyleAttribute(const nsAString& aString,
> >+  bool cachingAllowed = sheet && baseURI == docURI;
> 
> Worth documenting why you need this check.  And in particular why this is
> not comparing to the document's base URI.  Presumably because that can in
> fact change?

Done.

> We may also want to assert aElement->NodePrincipal() ==
> ownerDoc->NodePrincipal() here.

Done.

> >+    MiscContainer* cont =
> >+      reinterpret_cast<MiscContainer*>(sheet->LookupStyleAttr(aString));
> 
> LookupStyleAttr returns MiscContainer*, so no need to cast.

Fixed.

> >+nsAttrValue::ClearMiscContainer()
> >+      SetPtrValueAndType(cont, eOtherBase);
> >+      cont->mType = eColor;
> >+      cont->mStringBits = 0;
> >+      cont->mValue.mColor = 0;
> >+      cont->mValue.mRefCount = 0;
> >+      cont->mValue.mCached = 0;
> 
> We have another copy of this in EnsureEmptyMiscContainer already.  Can we
> possibly factor it out?  Like into the MiscContainer constructor?

Done.

> Also, should this method be called ClearMiscContainerIfAny?
> 
> >+nsAttrValue::EnsureEmptyMiscContainer()
> >+    cont = GetMiscContainer();
> 
> Why do you need to reget it?

We don't.

> >   cont->mType = eColor;
> 
> You only need to do these bits in the case when you just allocated it, right?

Yes.

> >+++ b/content/base/src/nsAttrValue.h
> 
> >-  nsAttrValue();
> 
> Why did these get moved?
> 
> >+  /**
> >+   *
> >+   */
> >+  bool ParseStyleAttribute(const nsAString& aString,
> 
> That comment block begs to be filled in!

Done.

> >@@ -423,17 +398,18 @@ private:
> >+  MiscContainer* ClearMiscContainer();
> >+  MiscContainer* EnsureEmptyMiscContainer();
> 
> Please document (esp. that ClearMiscContainer() will return null if there
> isn't one already!).

Done.

> >- * Implementation of inline methods
> 
> Why not move all of these to the inlines header instead of only moving the
> ones that currently use MiscContainer?

Because then we have to include the inlines header everywhere :-/

> >+++ b/dom/media/MediaManager.cpp
> 
> The changes here seem pretty unrelated to this patch.  Please nix.
> 
> >+++ b/layout/style/nsCSSRules.cpp
> >+Rule::GetHTMLStyleSheet() const
> >+    return reinterpret_cast<nsHTMLCSSStyleSheet*>(mSheet & ~0x1);
> 
> I think you need to cast the 0x1 to the right (unsigned, and correct width)
> typebefore the bitwise negation op.
> 
> I think the getter/setter here should be called Get/SetHTMLCSSStyleSheet,
> because we have a totally different class called nsHTMLStyleSheet.

Done.

> >+++ b/layout/style/nsHTMLCSSStyleSheet.cpp
> >+ClearAttrCache(const nsAString& aKey, MiscContainer*& aValue, void*)
> 
> Ah, excellent.  I'd been wondering about the lifetime management and
> adoption and stuff.  I think this works out.  ;)
> 
> >+nsHTMLCSSStyleSheet::EvictStyleAttr(const nsAString& aSerialized,
> >+#ifdef DEBUG
> >+#endif
> 
> Nix, please.

I added a real check here.

> r=me with those nits.  Looks great!

https://hg.mozilla.org/integration/mozilla-inbound/rev/72e482dbd384
Comment 10 Andrew McCreight [:mccr8] 2012-09-30 09:58:38 PDT
Can you quantify the memory improvement on at least some page?
Comment 11 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-09-30 10:10:46 PDT
On the page from bug 686795

before: 235,838,240 B (18.67%)  style-contexts
after: 11,880 B (00.00%)  style-contexts
Comment 12 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-09-30 10:14:19 PDT
And after this patch, the total memory numbers for that page (in a debug build) are:

│  │  ├──692,295,650 B (90.53%) -- active/window(http://people.mozilla.org/~khuey/tracemonkey-diff.html)
│  │  │  ├──486,009,990 B (63.55%) -- dom
│  │  │  │  ├──334,214,321 B (43.70%) ── element-nodes
│  │  │  │  ├───90,326,981 B (11.81%) ── text-nodes
│  │  │  │  ├───61,468,584 B (08.04%) ── other [2]
│  │  │  │  └──────────104 B (00.00%) ── comment-nodes
│  │  │  ├──204,479,372 B (26.74%) -- layout
│  │  │  │  ├──171,890,608 B (22.48%) -- frames
│  │  │  │  │  ├───85,665,120 B (11.20%) ── nsTextFrame
│  │  │  │  │  ├───85,558,400 B (11.19%) ── nsInlineFrame
│  │  │  │  │  ├──────417,120 B (00.05%) ── nsBlockFrame
│  │  │  │  │  ├──────176,832 B (00.02%) ── nsTableCellFrame
│  │  │  │  │  ├───────68,992 B (00.01%) ── nsTableRowFrame
│  │  │  │  │  └────────4,144 B (00.00%) ── sundries
│  │  │  │  ├───25,797,792 B (03.37%) ── line-boxes
│  │  │  │  ├────5,744,180 B (00.75%) ── pres-contexts
│  │  │  │  ├──────935,164 B (00.12%) ── pres-shell
│  │  │  │  ├───────87,904 B (00.01%) ── style-sets
│  │  │  │  ├───────12,092 B (00.00%) ── text-runs
│  │  │  │  ├────────8,272 B (00.00%) ── style-contexts
│  │  │  │  └────────3,360 B (00.00%) ── rule-nodes
│  │  │  ├────1,572,920 B (00.21%) ── property-tables
│  │  │  ├──────232,232 B (00.03%) -- js/compartment(http://people.mozilla.org/~khuey/tracemonkey-diff.html)
│  │  │  │      ├──130,496 B (00.02%) ── analysis-temporary
│  │  │  │      ├───81,920 B (00.01%) -- gc-heap
│  │  │  │      │   ├──44,696 B (00.01%) ── unused-gc-things
│  │  │  │      │   ├──14,688 B (00.00%) ── shapes/dict
│  │  │  │      │   ├──12,104 B (00.00%) ── sundries
│  │  │  │      │   └──10,432 B (00.00%) ── objects/function
│  │  │  │      └───19,816 B (00.00%) ── other-sundries
│  │  │  └────────1,136 B (00.00%) ── style-sheets
Comment 13 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-09-30 10:34:10 PDT
Compared to without:

│    │  ├──965,563,386 B (92.92%) -- active/window(http://people.mozilla.org/~khuey/tracemonkey-diff.html)
│    │  │  ├──486,009,990 B (46.77%) -- dom
│    │  │  │  ├──334,214,321 B (32.16%) ── element-nodes
│    │  │  │  ├───90,326,981 B (08.69%) ── text-nodes
│    │  │  │  ├───61,468,584 B (05.92%) ── other [2]
│    │  │  │  └──────────104 B (00.00%) ── comment-nodes
│    │  │  ├──477,747,796 B (45.97%) -- layout
│    │  │  │  ├──235,836,040 B (22.69%) ── style-contexts
│    │  │  │  ├──171,890,608 B (16.54%) -- frames
│    │  │  │  │  ├───85,665,120 B (08.24%) ── nsTextFrame
│    │  │  │  │  ├───85,558,400 B (08.23%) ── nsInlineFrame
│    │  │  │  │  ├──────417,120 B (00.04%) ── nsBlockFrame
│    │  │  │  │  ├──────176,832 B (00.02%) ── nsTableCellFrame
│    │  │  │  │  ├───────68,992 B (00.01%) ── nsTableRowFrame
│    │  │  │  │  └────────4,144 B (00.00%) ── sundries
│    │  │  │  ├───25,797,792 B (02.48%) ── line-boxes
│    │  │  │  ├───18,131,620 B (01.74%) ── pres-shell
│    │  │  │  ├───17,870,080 B (01.72%) ── rule-nodes
│    │  │  │  ├────8,121,692 B (00.78%) ── pres-contexts
│    │  │  │  ├───────87,872 B (00.01%) ── style-sets
│    │  │  │  └───────12,092 B (00.00%) ── text-runs
│    │  │  ├────1,572,920 B (00.15%) ── property-tables
│    │  │  ├──────231,544 B (00.02%) -- js/compartment(http://people.mozilla.org/~khuey/tracemonkey-diff.html)
│    │  │  │      ├──130,496 B (00.01%) ── analysis-temporary
│    │  │  │      ├───81,920 B (00.01%) -- gc-heap
│    │  │  │      │   ├──44,832 B (00.00%) ── unused-gc-things
│    │  │  │      │   ├──14,688 B (00.00%) ── shapes/dict
│    │  │  │      │   ├──11,968 B (00.00%) ── sundries
│    │  │  │      │   └──10,432 B (00.00%) ── objects/function
│    │  │  │      └───19,128 B (00.00%) ── other-sundries
│    │  │  └────────1,136 B (00.00%) ── style-sheets

So the noticeable differences are

before:

235,836,040 B (22.69%) ── style-contexts
18,131,620 B (01.74%) ── pres-shell
17,870,080 B (01.72%) ── rule-nodes

after:

935,164 B (00.12%) ── pres-shell
8,272 B (00.00%) ── style-contexts
3,360 B (00.00%) ── rule-nodes
Comment 14 Nicholas Nethercote [:njn] 2012-09-30 16:18:08 PDT
Awesome work, khuey.  In retrospect this probably should have been a MemShrink:P1.

Can you clarify exactly the case you've improved?  AIUI, if you have bazillions of <foo class="blah"> elements, you've avoided store the |class="blah"| part repeatedly -- is that right?
Comment 15 Nicholas Nethercote [:njn] 2012-09-30 16:45:48 PDT
I measured that page before and after on Linux64.  Full info below, but the main changes are as follows:

 style-contexts:    -408 MiB
 rule-nodes:        -36  MiB
 pres-shell:        -19  MiB
 heap-unclassified: -116 MiB
 pres-contexts      +145 MiB
 total:             -463 MiB

I don't know why I saw a pres-contexts increase and khuey didn't. 

The heap-unclassified improvement indicates that we're not measuring a non-trivial fraction of stuff related to style contexts.  (Perhaps this patch eliminated that unmeasured data?  Hard to know.)


BEFORE

1,759.30 MB (100.0%) -- explicit
├──1,495.27 MB (84.99%) -- window-objects
│  ├──1,487.06 MB (84.53%) -- top(file:///home/njn/huge.html, id=8)
│  │  ├──1,486.33 MB (84.48%) -- active/window(file:///home/njn/huge.html)
│  │  │  ├────774.67 MB (44.03%) -- layout
│  │  │  │    ├──408.93 MB (23.24%) ── style-contexts
│  │  │  │    ├──237.75 MB (13.51%) -- frames
│  │  │  │    │  ├──122.39 MB (06.96%) ── nsInlineFrame
│  │  │  │    │  ├──114.38 MB (06.50%) ── nsTextFrame
│  │  │  │    │  └────0.98 MB (00.06%) ++ (4 tiny)
│  │  │  │    ├───32.80 MB (01.86%) ── line-boxes
│  │  │  │    ├───30.68 MB (01.74%) ── rule-nodes
│  │  │  │    ├───24.74 MB (01.41%) ── text-runs
│  │  │  │    ├───22.19 MB (01.26%) ── pres-shell
│  │  │  │    └───17.58 MB (01.00%) ++ (2 tiny)
│  │  │  ├────705.28 MB (40.09%) -- dom
│  │  │  │    ├──456.78 MB (25.96%) ── element-nodes
│  │  │  │    ├──152.21 MB (08.65%) ── text-nodes
│  │  │  │    ├───96.29 MB (05.47%) ── other [2]
│  │  │  │    └────0.00 MB (00.00%) ── comment-nodes
│  │  │  └──────6.38 MB (00.36%) ++ (3 tiny)
│  │  └──────0.73 MB (00.04%) ++ cached/window(about:home)
│  └──────8.21 MB (00.47%) ++ (4 tiny)
├────151.16 MB (08.59%) ── heap-unclassified
├─────48.18 MB (02.74%) ── history-links-hashtable
├─────26.54 MB (01.51%) -- js-non-window
│     ├──18.10 MB (01.03%) ++ compartments
│     └───8.44 MB (00.48%) ++ (2 tiny)
├─────24.66 MB (01.40%) ── cycle-collector
└─────13.50 MB (00.77%) ++ (9 tiny)


AFTER

1,306.82 MB (100.0%) -- explicit
├──1,182.38 MB (90.48%) -- window-objects
│  ├──1,172.67 MB (89.73%) -- top(file:///home/njn/huge.html, id=8)/active/window(file:///home/njn/huge.html)
│  │  ├────705.34 MB (53.97%) -- dom
│  │  │    ├──456.84 MB (34.96%) ── element-nodes
│  │  │    ├──152.21 MB (11.65%) ── text-nodes
│  │  │    ├───96.29 MB (07.37%) ── other [2]
│  │  │    └────0.00 MB (00.00%) ── comment-nodes
│  │  ├────418.89 MB (32.05%) -- layout
│  │  │    ├──237.75 MB (18.19%) -- frames
│  │  │    │  ├──122.39 MB (09.37%) ── nsInlineFrame
│  │  │    │  ├──114.38 MB (08.75%) ── nsTextFrame
│  │  │    │  └────0.98 MB (00.07%) ++ (4 tiny)
│  │  │    ├──145.64 MB (11.14%) ── pres-contexts
│  │  │    ├───32.80 MB (02.51%) ── line-boxes
│  │  │    └────2.70 MB (00.21%) ++ (4 tiny)
│  │  ├─────48.00 MB (03.67%) ── property-tables
│  │  └──────0.44 MB (00.03%) ++ (2 tiny)
│  └──────9.71 MB (00.74%) ++ (4 tiny)
├─────48.18 MB (03.69%) ── history-links-hashtable
├─────35.17 MB (02.69%) ── heap-unclassified
├─────28.80 MB (02.20%) -- js-non-window
│     ├──16.57 MB (01.27%) -- compartments
│     │  ├──14.68 MB (01.12%) ++ non-window-global
│     │  └───1.89 MB (00.14%) ++ no-global
│     └──12.22 MB (00.94%) ++ (2 tiny)
└─────12.29 MB (00.94%) ++ (10 tiny)
Comment 16 Nathan Froyd [:froydnj] 2012-09-30 16:49:55 PDT
(In reply to Nicholas Nethercote [:njn] from comment #15)
> The heap-unclassified improvement indicates that we're not measuring a
> non-trivial fraction of stuff related to style contexts.  (Perhaps this
> patch eliminated that unmeasured data?  Hard to know.)

ISTR that there are comments in the SizeOf bits for style contexts that indicate measuring some of the most prominent members is difficult due to style sharing.
Comment 17 David Baron :dbaron: ⌚️UTC+1 (mostly busy through August 4; review requests must explain patch) 2012-09-30 17:12:20 PDT
(In reply to Nathan Froyd (:froydnj) from comment #16)
> (In reply to Nicholas Nethercote [:njn] from comment #15)
> > The heap-unclassified improvement indicates that we're not measuring a
> > non-trivial fraction of stuff related to style contexts.  (Perhaps this
> > patch eliminated that unmeasured data?  Hard to know.)
> 
> ISTR that there are comments in the SizeOf bits for style contexts that
> indicate measuring some of the most prominent members is difficult due to
> style sharing.

Style contexts allocate most things in the pres shell's arena, so they don't have their own SizeOf methods.  However, there are things like string and array members (of style structs) that they aren't in the pres shell arena.  We could probably add SizeOfExcludingThis methods to style structs, and iterate the frame tree to find all the style structs, but that would slow down sizeof calculations a good bit.  Alternatively, we could try to put everything that's part of the style structs into the pres shell arena.  Perhaps worth filing a separate bug on if there isn't one already.
Comment 18 Boris Zbarsky [:bz] 2012-09-30 17:33:50 PDT
Are we measuring the actual inline style rules for about:memory?  There's a good chance that we're not.
Comment 19 Ryan VanderMeulen [:RyanVM] 2012-09-30 18:02:18 PDT
https://hg.mozilla.org/mozilla-central/rev/72e482dbd384
Comment 20 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-09-30 19:18:14 PDT
(In reply to Nicholas Nethercote [:njn] from comment #14)
> Awesome work, khuey.  In retrospect this probably should have been a
> MemShrink:P1.

Well, the testcase is very pathological.  I suspect P2 was the correct prioritization.

> Can you clarify exactly the case you've improved?  AIUI, if you have
> bazillions of <foo class="blah"> elements, you've avoided store the
> |class="blah"| part repeatedly -- is that right?

I've improved bazillions of <foo style="blah">.  I did not implement caching for other types of attributes (though most of the infrastructure is there).  I suspect that the win to be had on something like 'class' is much smaller.

As for the measurements, I don't have any explanation for the pres-context discrepancy.  I suspect that the inline style rules (almost all of which this patch eliminated) are not being counted for about:memory.  There's no code to get at them from the content side, so unless we are counting style rules as we traverse the rule node tree (and I don't think that would make sense to do) we are not counting these rules.
Comment 21 David Baron :dbaron: ⌚️UTC+1 (mostly busy through August 4; review requests must explain patch) 2012-09-30 20:38:04 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #20)
> I suspect that the inline style rules (almost all of which
> this patch eliminated) are not being counted for about:memory.  There's no
> code to get at them from the content side, so unless we are counting style
> rules as we traverse the rule node tree (and I don't think that would make
> sense to do) we are not counting these rules.

There's commented-out code to get to them from nsAttrValue::SizeOfExcludingThis , which seems like the right place (if uncommented):
      if (Type() == eCSSStyleRule && container->mCSSStyleRule) {
        // TODO: mCSSStyleRule might be owned by another object which would
        //       make us count them twice, bug 677493.
        //n += container->mCSSStyleRule->SizeOfIncludingThis(aMallocSizeOf);
      } else ...

Depending on whether the very large number of style contexts each had a unique copy of certain style structs (e.g., the font struct), a decent part of the heap-unclassified could also have been comment 17.
Comment 22 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-09-30 20:39:29 PDT
(In reply to David Baron [:dbaron] from comment #21)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #20)
> > I suspect that the inline style rules (almost all of which
> > this patch eliminated) are not being counted for about:memory.  There's no
> > code to get at them from the content side, so unless we are counting style
> > rules as we traverse the rule node tree (and I don't think that would make
> > sense to do) we are not counting these rules.
> 
> There's commented-out code to get to them from
> nsAttrValue::SizeOfExcludingThis , which seems like the right place (if
> uncommented):
>       if (Type() == eCSSStyleRule && container->mCSSStyleRule) {
>         // TODO: mCSSStyleRule might be owned by another object which would
>         //       make us count them twice, bug 677493.
>         //n += container->mCSSStyleRule->SizeOfIncludingThis(aMallocSizeOf);
>       } else ...

Yes, but now with the rule sharing implemented in this bug avoiding double-counting is much harder.

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