760331 - Consider coalescing inline style rules across nodes when possible

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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

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-4, no longer working on Mozilla)]

How do we stop caching the rules that are no longer used?

Comment 3

[Boris Zbarsky [:bzbarsky]]

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) No longer reading bugmail consistently]

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 (Exited; not receiving bugmail, old account, do not use)]

I have mostly complete patches for this.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

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

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 (Exited; not receiving bugmail, old account, do not use)]

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

Can you quantify the memory improvement on at least some page?

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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 (Exited; not receiving bugmail, old account, do not use)]

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 (Exited; not receiving bugmail, old account, do not use)]

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

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

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

(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-4, no longer working on Mozilla)]

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

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

https://hg.mozilla.org/mozilla-central/rev/72e482dbd384

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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-4, no longer working on Mozilla)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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 (Exited; not receiving bugmail, old account, do not use)]

(In reply to David Baron [:dbaron] from comment #21)
> (In reply to Kyle Huey [:khuey] ([e-mail]) 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.