Cache cssomWrappers for StylePropertySets directly on the MutableStylePropertySet.

Cache cssomWrappers for StylePropertySets directly on the MutableStylePropertySet.

In my measurements the mapping is adding more overhead than just having a field
in all MutableStylePropertySet objects. So this saves memory and makes access
faster.

Memory measurements:

gmail:
Wrappers: 276/297 (92%) Overhead: table > 16384 bytes, fields 2376 bytes

facebook:
Wrappers: 9/51 (17%) Overhead: table > 512 bytes, fields 408 bytes

google plus:
Wrappers: 208/229 (90%) Overhead: table > 16384 bytes, fields 1832 bytes

cnn:
Wrappers: 66/106 (62%) Overhead: table > 4096 bytes, fields 848 bytes

youtube:
Wrappers: 9/27 (33%) Overhead: table > 512 bytes, fields 216 bytes

amazon:
Wrappers: 48/90 (53%) Overhead: table > 4096 bytes, fields 720 bytes

R=abarth@chromium.org
BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=153700

[Mads Ager (chromium), 2013-07-03 13:59:37]

For the record, the measurements were performed with the attached patch.

Cheers,    -- Mads


On Wed, Jul 3, 2013 at 3:58 PM, <ager@chromium.org> wrote:

> Reviewers: abarth,
>
> Description:
> Cache cssomWrappers for StylePropertySets directly on the
> MutableStylePropertySet.
>
> In my measurements the mapping is adding more overhead than just having a
> field
> in all MutableStylePropertySet objects. So this saves memory and makes
> access
> faster.
>
> Memory measurements:
>
> gmail:
> Wrappers: 276/297 (92%) Overhead: table > 16384 bytes, fields 2376 bytes
>
> facebook:
> Wrappers: 9/51 (17%) Overhead: table > 512 bytes, fields 408 bytes
>
> google plus:
> Wrappers: 208/229 (90%) Overhead: table > 16384 bytes, fields 1832 bytes
>
> cnn:
> Wrappers: 66/106 (62%) Overhead: table > 4096 bytes, fields 848 bytes
>
> youtube:
> Wrappers: 9/27 (33%) Overhead: table > 512 bytes, fields 216 bytes
>
> amazon:
> Wrappers: 48/90 (53%) Overhead: table > 4096 bytes, fields 720 bytes
>
> R=abarth@chromium.org
> BUG=
>
> Please review this at
https://codereview.chromium.**org/18272014/<https://codereview.chromium.org/1...
>
> SVN Base:
svn://svn.chromium.org/blink/**trunk<http://svn.chromium.org/blink/trunk>
>
> Affected files:
>   M Source/core/css/**StylePropertySet.h
>   M Source/core/css/**StylePropertySet.cpp
>
>
> Index: Source/core/css/**StylePropertySet.cpp
> diff --git a/Source/core/css/**StylePropertySet.cpp b/Source/core/css/**
> StylePropertySet.cpp
> index 81467555886c2c1843a5b55395a299**10d6d99ea4..**
> 8061f217835cd63b7ed455d6e0cef1**a8342b4e49 100644
> --- a/Source/core/css/**StylePropertySet.cpp
> +++ b/Source/core/css/**StylePropertySet.cpp
> @@ -42,13 +42,6 @@ using namespace std;
>
>  namespace WebCore {
>
> -typedef HashMap<**MutableStylePropertySet*, OwnPtr<**
> PropertySetCSSStyleDeclaration**> > PropertySetCSSOMWrapperMap;
> -static PropertySetCSSOMWrapperMap& propertySetCSSOMWrapperMap()
> -{
> -    DEFINE_STATIC_LOCAL(**PropertySetCSSOMWrapperMap,
> propertySetCSSOMWrapperMapInst**ance, ());
> -    return propertySetCSSOMWrapperMapInst**ance;
> -}
> -
>  static size_t sizeForImmutableStylePropertyS**etWithPropertyCount(unsigned
> count)
>  {
>      return sizeof(**ImmutableStylePropertySet) - sizeof(void*) +
> sizeof(CSSValue*) * count + sizeof(StylePropertyMetadata) * count;
> @@ -68,6 +61,10 @@ PassRefPtr<**ImmutableStylePropertySet>
> StylePropertySet::**immutableCopyIfNeeded()
>      return
ImmutableStylePropertySet::**create(mutableThis->m_**propertyVector.data(),
> mutableThis->m_propertyVector.**size(), cssParserMode());
>  }
>
> +MutableStylePropertySet::**MutableStylePropertySet(**CSSParserMode
> cssParserMode)
> +    : StylePropertySet(**cssParserMode)
> +{ }
> +
>  MutableStylePropertySet::**MutableStylePropertySet(const CSSProperty*
> properties, unsigned length)
>      : StylePropertySet(**CSSStrictMode)
>  {
> @@ -107,13 +104,6 @@ MutableStylePropertySet::**MutableStylePropertySet(const
> StylePropertySet& other)
>      }
>  }
>
> -MutableStylePropertySet::~**MutableStylePropertySet()
> -{
> -    ASSERT(!m_ownsCSSOMWrapper || propertySetCSSOMWrapperMap().**
> contains(this));
> -    if (m_ownsCSSOMWrapper)
> -        propertySetCSSOMWrapperMap().**remove(this);
> -}
> -
>  String StylePropertySet::**getPropertyValue(CSSPropertyID propertyID)
> const
>  {
>      RefPtr<CSSValue> value = getPropertyCSSValue(**propertyID);
> @@ -515,34 +505,30 @@ PassRefPtr<**MutableStylePropertySet>
> StylePropertySet::**copyPropertiesInSet(const
>
>  PropertySetCSSStyleDeclaration*** MutableStylePropertySet::**
> cssStyleDeclaration()
>  {
> -    if (!m_ownsCSSOMWrapper)
> -        return 0;
> -    return propertySetCSSOMWrapperMap().**get(this);
> +    return m_cssomWrapper.get();
>  }
>
>  CSSStyleDeclaration* MutableStylePropertySet::**
> ensureCSSStyleDeclaration()
>  {
>      if (m_ownsCSSOMWrapper) {
> -        ASSERT(!static_cast<**CSSStyleDeclaration*>(**
> propertySetCSSOMWrapperMap().**get(this))->parentRule());
> -        ASSERT(!**propertySetCSSOMWrapperMap().**
> get(this)->parentElement());
> -        return propertySetCSSOMWrapperMap().**get(this);
> +        ASSERT(!static_cast<**CSSStyleDeclaration*>(m_**
> cssomWrapper.get())->**parentRule());
> +        ASSERT(!m_cssomWrapper->**parentElement());
> +        return m_cssomWrapper.get();
>      }
>      m_ownsCSSOMWrapper = true;
> -    PropertySetCSSStyleDeclaration*** cssomWrapper = new
> PropertySetCSSStyleDeclaration**(this);
> -    propertySetCSSOMWrapperMap().**add(this, adoptPtr(cssomWrapper));
> -    return cssomWrapper;
> +    m_cssomWrapper = adoptPtr(new PropertySetCSSStyleDeclaration**
> (this));
> +    return m_cssomWrapper.get();
>  }
>
>  CSSStyleDeclaration* MutableStylePropertySet::**
> ensureInlineCSSStyleDeclaratio**n(Element* parentElement)
>  {
>      if (m_ownsCSSOMWrapper) {
> -        ASSERT(**propertySetCSSOMWrapperMap().**get(this)->parentElement()
> == parentElement);
> -        return propertySetCSSOMWrapperMap().**get(this);
> +        ASSERT(m_cssomWrapper->**parentElement() == parentElement);
> +        return m_cssomWrapper.get();
>      }
>      m_ownsCSSOMWrapper = true;
> -    PropertySetCSSStyleDeclaration*** cssomWrapper = new
> InlineCSSStyleDeclaration(**this, parentElement);
> -    propertySetCSSOMWrapperMap().**add(this, adoptPtr(cssomWrapper));
> -    return cssomWrapper;
> +    m_cssomWrapper = adoptPtr(new InlineCSSStyleDeclaration(**this,
> parentElement));
> +    return m_cssomWrapper.get();
>  }
>
>  unsigned StylePropertySet::**averageSizeInBytes()
> Index: Source/core/css/**StylePropertySet.h
> diff --git a/Source/core/css/**StylePropertySet.h b/Source/core/css/**
> StylePropertySet.h
> index 3fed4302f2db09da3ccb9aca97bd3c**3cb4db6ddb..**
> c115a87a6261e4a96a7fb3b3a88276**2c39e0c203 100644
> --- a/Source/core/css/**StylePropertySet.h
> +++ b/Source/core/css/**StylePropertySet.h
> @@ -25,6 +25,7 @@
>  #include "core/css/CSSParserMode.h"
>  #include "core/css/CSSPrimitiveValue.h"
>  #include "core/css/CSSProperty.h"
> +#include "core/css/**PropertySetCSSStyleDeclaration**.h"
>  #include <wtf/ListHashSet.h>
>  #include <wtf/text/WTFString.h>
>  #include <wtf/Vector.h>
> @@ -37,7 +38,6 @@ class Element;
>  class ImmutableStylePropertySet;
>  class KURL;
>  class MutableStylePropertySet;
> -class PropertySetCSSStyleDeclaration**;
>  class StylePropertyShorthand;
>  class StyleSheetContents;
>
> @@ -174,8 +174,6 @@ inline const StylePropertyMetadata*
> ImmutableStylePropertySet::**metadataArray() c
>
>  class MutableStylePropertySet : public StylePropertySet {
>  public:
> -    ~MutableStylePropertySet();
> -
>      static PassRefPtr<**MutableStylePropertySet> create(CSSParserMode =
> CSSQuirksMode);
>      static PassRefPtr<**MutableStylePropertySet> create(const
> CSSProperty* properties, unsigned count);
>
> @@ -216,14 +214,12 @@ public:
>      Vector<CSSProperty, 4> m_propertyVector;
>
>  private:
> -    MutableStylePropertySet(**CSSParserMode cssParserMode)
> -        : StylePropertySet(**cssParserMode)
> -    { }
> -
> +    MutableStylePropertySet(**CSSParserMode);
>      MutableStylePropertySet(const CSSProperty* properties, unsigned
> count);
>
>      bool removeShorthandProperty(**CSSPropertyID);
>      CSSProperty* findCSSPropertyWithID(**CSSPropertyID);
> +    OwnPtr<**PropertySetCSSStyleDeclaration**> m_cssomWrapper;
>  };
>
>  inline StylePropertyMetadata
StylePropertySet::**PropertyReference::**propertyMetadata()
> const
>
>
>

[abarth-chromium, 2013-07-03 18:25:42]

LGTM with some minor comments below.

https://codereview.chromium.org/18272014/diff/1/Source/core/css/StyleProperty...
File Source/core/css/StylePropertySet.cpp (right):

https://codereview.chromium.org/18272014/diff/1/Source/core/css/StyleProperty...
Source/core/css/StylePropertySet.cpp:66: { }
Please put each brace on it's own line.

https://codereview.chromium.org/18272014/diff/1/Source/core/css/StyleProperty...
Source/core/css/StylePropertySet.cpp:513: if (m_ownsCSSOMWrapper) {
It seems like we can just check whether m_cssomWrapper is zero here instead of
having this separate m_ownsCSSOMWrapper flag.

https://codereview.chromium.org/18272014/diff/1/Source/core/css/StyleProperty...
File Source/core/css/StylePropertySet.h (right):

https://codereview.chromium.org/18272014/diff/1/Source/core/css/StyleProperty...
Source/core/css/StylePropertySet.h:138: mutable unsigned m_ownsCSSOMWrapper : 1;
Do we still need this field?

https://codereview.chromium.org/18272014/diff/1/Source/core/css/StyleProperty...
Source/core/css/StylePropertySet.h:217: MutableStylePropertySet(CSSParserMode);
Please mark one-argument constructors "explicit"

[Mads Ager (chromium), 2013-07-04 06:05:47]

Thanks Adam. Could you have another quick look?

https://codereview.chromium.org/18272014/diff/1/Source/core/css/StyleProperty...
File Source/core/css/StylePropertySet.cpp (right):

https://codereview.chromium.org/18272014/diff/1/Source/core/css/StyleProperty...
Source/core/css/StylePropertySet.cpp:66: { }
On 2013/07/03 18:25:42, abarth wrote:
> Please put each brace on it's own line.

Done.

https://codereview.chromium.org/18272014/diff/1/Source/core/css/StyleProperty...
Source/core/css/StylePropertySet.cpp:513: if (m_ownsCSSOMWrapper) {
On 2013/07/03 18:25:42, abarth wrote:
> It seems like we can just check whether m_cssomWrapper is zero here instead of
> having this separate m_ownsCSSOMWrapper flag.

We can do that. The only complication is that StylePropertySet has a
hasCSSOMWrapper method that is used in a couple of places. I have implemented
that in terms of m_isMutable and m_cssomWrapper. That means that
StylePropertySet needs to be a friend of MutableStylePropertySet. WDYT?

https://codereview.chromium.org/18272014/diff/1/Source/core/css/StyleProperty...
File Source/core/css/StylePropertySet.h (right):

https://codereview.chromium.org/18272014/diff/1/Source/core/css/StyleProperty...
Source/core/css/StylePropertySet.h:138: mutable unsigned m_ownsCSSOMWrapper : 1;
On 2013/07/03 18:25:42, abarth wrote:
> Do we still need this field?

Thanks, no we don't need it. See later comment on the hasCSSOMWrapper method.

https://codereview.chromium.org/18272014/diff/1/Source/core/css/StyleProperty...
Source/core/css/StylePropertySet.h:217: MutableStylePropertySet(CSSParserMode);
On 2013/07/03 18:25:42, abarth wrote:
> Please mark one-argument constructors "explicit"

Done for this one and the constructor in the public section. Moved that
constructor to the private section now that I have StylePropertySet as a friend
here (for the hasCSSOMWrapper method).

[abarth-chromium, 2013-07-08 18:16:00]

lgtm

Thanks!

[commit-bot: I haz the power, 2013-07-08 18:16:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ager@chromium.org/18272014/7001

[commit-bot: I haz the power, 2013-07-08 20:23:42]

Change committed as 153700