Remove attrNodeListMap.

Remove AttrNodeListMap.

Change the ownership of AttrNodeList from a static HashMap to ElementRareData.

ElementRareData size increases by this change. A complex web page will
have slight increase of memory consumption even if the page doesn't
use Attr nodes.

BUG=357163, 381629
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176366

[tkent, 2014-06-16 08:23:07]

Please review this.

The static HashMap was introduced by http://trac.webkit.org/changeset/114870,
and no one have tried to move the list to ElementRareData.



Dromaeo DOM Attributes test:

Without this CL:
*RESULT dom_attr: dom_attr= 506.015387438 runs/s

With this CL:
*RESULT dom_attr: dom_attr= 525.412285397 runs/s

[tkent, 2014-06-16 08:28:25]

> Dromaeo DOM Attributes test:
> 
> Without this CL:
> *RESULT dom_attr: dom_attr= 506.015387438 runs/s
> 
> With this CL:
> *RESULT dom_attr: dom_attr= 525.412285397 runs/s

I ran just once for each configurations.  These numbers are not reliable.

[haraken, 2014-06-16 08:29:23]

On 2014/06/16 08:23:07, tkent wrote:
> Please review this.
> 
> The static HashMap was introduced by http://trac.webkit.org/changeset/114870,
> and no one have tried to move the list to ElementRareData.
> 
> 
> 
> Dromaeo DOM Attributes test:
> 
> Without this CL:
> *RESULT dom_attr: dom_attr= 506.015387438 runs/s
> 
> With this CL:
> *RESULT dom_attr: dom_attr= 525.412285397 runs/s

What's a memory impact of this change? Given that a lot of elements would have
attribute nodes, I'm a bit concerned that we will end up creating
ElementRareData on a lot of elements just for the AttrNodeList.

In https://code.google.com/p/chromium/issues/detail?id=381629#c2, Mads is saying
that "Measuring on gmail I see a memory overhead of more than 12kB which does
not seem reasonable".

[Mads Ager (chromium), 2014-06-16 08:39:43]

This was my initial reaction as well. However, I measured the memory overhead
before doing the change and found that this adds around 12kB of overhead on
initial load of GMail and similar sites. On GMail the size of this map is 0 and
therefore we are paying a full pointer size per object that has rare data. Those
measurements led me to the conclusion that we shouldn't move this to element
rare data:

https://code.google.com/p/chromium/issues/detail?id=381629#c2

Because attr nodes are so rarely used I think we should keep the map for this as
a memory optimization.

[tkent, 2014-06-16 08:40:29]

I haven't measure memory consumption change yet.
 - IMO, we don't need to worry about increase of the number of elements with
ElementRareData.  Attr usage is really low.
 - We should worry about increase of ElementRareData size, which affects all of
elements with ElementRareData.

Another idea is to move the map to Document.

[Mads Ager (chromium), 2014-06-16 08:42:47]

On 2014/06/16 08:40:29, tkent wrote:
> I haven't measure memory consumption change yet.
>  - IMO, we don't need to worry about increase of the number of elements with
> ElementRareData.  Attr usage is really low.
>  - We should worry about increase of ElementRareData size, which affects all
of
> elements with ElementRareData.

Exactly, that's what I found as well. I found that Attr node usage is so low
that it seems unreasonable to pay an extra word per ElementRareData for it.
 
> Another idea is to move the map to Document.

That sounds much better to me. :)

[tkent, 2014-06-16 08:43:07]

On 2014/06/16 08:39:43, Mads Ager (chromium) wrote:
> This was my initial reaction as well. However, I measured the memory overhead
> before doing the change and found that this adds around 12kB of overhead on
> initial load of GMail and similar sites. On GMail the size of this map is 0
and
> therefore we are paying a full pointer size per object that has rare data.
Those
> measurements led me to the conclusion that we shouldn't move this to element
> rare data:

Do you mean 12KB per page?

[haraken, 2014-06-16 08:44:03]

> I haven't measure memory consumption change yet.
>  - IMO, we don't need to worry about increase of the number of elements with
> ElementRareData.  Attr usage is really low.
>  - We should worry about increase of ElementRareData size, which affects all
of
> elements with ElementRareData.

Understood; I was misunderstanding when Attr nodes are created.

Then I don't think it makes sense to measure performance in
Dromaeo/dom-attr.html. It won't create any Attr node.

> Another idea is to move the map to Document.

This sounds nice to me. Another idea would be just to wait for Erik to implement
the new semantics of weak collections :) We'll need the new semantics anyway.

[Mads Ager (chromium), 2014-06-16 08:50:47]

On 2014/06/16 08:43:07, tkent wrote:
> On 2014/06/16 08:39:43, Mads Ager (chromium) wrote:
> > This was my initial reaction as well. However, I measured the memory
overhead
> > before doing the change and found that this adds around 12kB of overhead on
> > initial load of GMail and similar sites. On GMail the size of this map is 0
> and
> > therefore we are paying a full pointer size per object that has rare data.
> Those
> > measurements led me to the conclusion that we shouldn't move this to element
> > rare data:
> 
> Do you mean 12KB per page?

12KB for the renderer running gmail. I measured this by doing this calculation:

overhead = 
(#ElementRareData * pointerSize) +
(#ElementsWithAttrNodesButNoRareData * (sizeof(ElementRareData) + pointerSize))
-
(mapCapacity * 2 * pointerSize)

Each element rare data needs an extra pointer.
Each element with attr nodes but no rare data now needs rare data.
We do not need the space for the map (size approximated to 2 pointers per
capacity which is sligthly lower than it really is).

These are global numbers for the renderer.

On real sites it looks like attr nodes are very rarely used, so the overhead
basically reduced to an extra pointer per element rare data and the two other
elements in the calculation were zero.

[tkent, 2014-06-16 08:53:20]

On 2014/06/16 08:50:47, Mads Ager (chromium) wrote:
> On 2014/06/16 08:43:07, tkent wrote:
> > On 2014/06/16 08:39:43, Mads Ager (chromium) wrote:
> > > This was my initial reaction as well. However, I measured the memory
> overhead
> > > before doing the change and found that this adds around 12kB of overhead
on
> > > initial load of GMail and similar sites. On GMail the size of this map is
0
> > and
> > > therefore we are paying a full pointer size per object that has rare data.
> > Those
> > > measurements led me to the conclusion that we shouldn't move this to
element
> > > rare data:
> > 
> > Do you mean 12KB per page?
> 
> 12KB for the renderer running gmail. I measured this by doing this
calculation:
> 
> overhead = 
> (#ElementRareData * pointerSize) +
> (#ElementsWithAttrNodesButNoRareData * (sizeof(ElementRareData) +
pointerSize))
> -
> (mapCapacity * 2 * pointerSize)
> 
> Each element rare data needs an extra pointer.
> Each element with attr nodes but no rare data now needs rare data.
> We do not need the space for the map (size approximated to 2 pointers per
> capacity which is sligthly lower than it really is).
> 
> These are global numbers for the renderer.
> 
> On real sites it looks like attr nodes are very rarely used, so the overhead
> basically reduced to an extra pointer per element rare data and the two other
> elements in the calculation were zero.

Thank you.


I have found a way to add AttrNodeList to ElementRareData without increasing
ElementRareData size.  I'll work on it tomorrow.

[tkent, 2014-06-16 23:53:03]

Confirmed Dromaeo didn't use Attr objects.

On 2014/06/16 08:40:29, tkent wrote:
> Another idea is to move the map to Document.

This won't resolve the issue. An Element with Attrs won't be collected until
Document is collected.

[tkent, 2014-06-16 23:53:32]

On 2014/06/16 08:53:20, tkent wrote:
> I have found a way to add AttrNodeList to ElementRareData without increasing
> ElementRareData size.  I'll work on it tomorrow.

Uploaded new patch.
Please take another look.

[haraken, 2014-06-17 02:07:02]

https://codereview.chromium.org/328243005/diff/20001/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/328243005/diff/20001/Source/core/dom/Element....
Source/core/dom/Element.cpp:2966: if (AttrNodeList* attrNodeList =
this->attrNodeList())

Rename the |attrNodeList| variable to something else and remove this-> ? Ditto
for other parts in this file.

https://codereview.chromium.org/328243005/diff/20001/Source/core/dom/ElementR...
File Source/core/dom/ElementRareData.h (right):

https://codereview.chromium.org/328243005/diff/20001/Source/core/dom/ElementR...
Source/core/dom/ElementRareData.h:230: class AttrData : public
NoBaseWillBeGarbageCollected<AttrData> {

This is something like we're introducing another RareData to the
ElementRareData. Although I agree that it makes sense to pack Attr objects and a
NamedNodeMap together (since they are likely to be used at the same time), it's
also true that this adds another complexity to the ElementRareData.

I'm not quite sure if it's worth making this change. Once Erik implements the
fixed-point algorithm of weak collections, we can just keep using the current
AttrNodeList as is. That seems better to me.

[haraken, 2014-06-17 02:08:17]

+Elliott

[tkent, 2014-06-17 06:47:50]

Regardless of the Oilpan leak issue, I think this CL is better than the static
HashMap for code maintenability.

Adding another raredata-like object is a hack.  I personally think 14KB increase
in Gmail is acceptable and it's ok to add AttrNodeList to ElementRareData.

Note that ElementRareData::m_inputMethodContext is unused in production.  No one
didn't aware increased memory consumption when it was added.

[Mads Ager (chromium), 2014-06-17 07:43:57]

On 2014/06/17 06:47:50, tkent wrote:
> Regardless of the Oilpan leak issue, I think this CL is better than the static
> HashMap for code maintenability.
> 
> Adding another raredata-like object is a hack.  I personally think 14KB
increase
> in Gmail is acceptable and it's ok to add AttrNodeList to ElementRareData.
> 
> Note that ElementRareData::m_inputMethodContext is unused in production.  No
one
> didn't aware increased memory consumption when it was added.

I agree that it is cleaner to have the map in ElementRareData; that was my
initial thought as well. I am still concerned about increasing memory for a
largely unused feature. However, if we believe that adding an extra pointer to
ElementRareData is not a big deal I would be happy to get this map into
ElementRareData instead of putting it in a global map.

[haraken, 2014-06-17 08:03:27]

On 2014/06/17 07:43:57, Mads Ager (chromium) wrote:
> On 2014/06/17 06:47:50, tkent wrote:
> > Regardless of the Oilpan leak issue, I think this CL is better than the
static
> > HashMap for code maintenability.
> > 
> > Adding another raredata-like object is a hack.  I personally think 14KB
> increase
> > in Gmail is acceptable and it's ok to add AttrNodeList to ElementRareData.
> > 
> > Note that ElementRareData::m_inputMethodContext is unused in production.  No
> one
> > didn't aware increased memory consumption when it was added.
> 
> I agree that it is cleaner to have the map in ElementRareData; that was my
> initial thought as well. I am still concerned about increasing memory for a
> largely unused feature. However, if we believe that adding an extra pointer to
> ElementRareData is not a big deal I would be happy to get this map into
> ElementRareData instead of putting it in a global map.

I don't have enough experience to judge which is better, so I'd like to delegate
the decision to Elliott.

[esprehn, 2014-06-17 08:27:05]

I'm surprised there's a memory usage increase here. Touching element.attributes
already allocates a rare data, and almost no one ever does setAttributeNode()
without touching .attributes, so what makes GMail use more memory? Is it that
touching .attributes now allocates this AttrData object which adds 2 ptrs?

https://codereview.chromium.org/328243005/diff/20001/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/328243005/diff/20001/Source/core/dom/Element....
Source/core/dom/Element.cpp:1835: return hasRareData() &&
elementRareData()->attrData() ? elementRareData()->attrData()->attrNodeList() :
0;
This would be better as:

if (!hasRareData())
  return 0;
if (AttrData* data = elementRareData()->attrData())
  return data->attrNodeList();
return 0;

https://codereview.chromium.org/328243005/diff/20001/Source/core/dom/Element....
Source/core/dom/Element.cpp:1840: setHasSyntheticAttrChildNodes(true);
You can remove this bitfield now. hasSyntheticAttrChildNodes should check
hasRareData() && elementRareData()->attrData()->attrNodeList()

https://codereview.chromium.org/328243005/diff/20001/Source/core/dom/Element....
Source/core/dom/Element.cpp:2751: return hasRareData() &&
elementRareData()->attrData() && elementRareData()->attrData()->attributeMap();
ditto

https://codereview.chromium.org/328243005/diff/20001/Source/core/dom/Element....
Source/core/dom/Element.cpp:2966: if (AttrNodeList* attrNodeList =
this->attrNodeList())
list = ...

https://codereview.chromium.org/328243005/diff/20001/Source/core/dom/ElementR...
File Source/core/dom/ElementRareData.cpp (right):

https://codereview.chromium.org/328243005/diff/20001/Source/core/dom/ElementR...
Source/core/dom/ElementRareData.cpp:78: inline AttrData::AttrData()
Put it in the header or remove the inline.

https://codereview.chromium.org/328243005/diff/20001/Source/core/dom/ElementR...
Source/core/dom/ElementRareData.cpp:93: #endif
This code doesn't make sense. We should never call trace() methods when not in
oilpan mode. Remove this ifdef.

https://codereview.chromium.org/328243005/diff/20001/Source/core/dom/ElementR...
File Source/core/dom/ElementRareData.h (right):

https://codereview.chromium.org/328243005/diff/20001/Source/core/dom/ElementR...
Source/core/dom/ElementRareData.h:125: AttrData* attrData() { return
m_attrData.get(); }
const

https://codereview.chromium.org/328243005/diff/20001/Source/core/dom/ElementR...
Source/core/dom/ElementRareData.h:230: class AttrData : public
NoBaseWillBeGarbageCollected<AttrData> {
We really should put these in separate files, but it's fine to put it here for
now since they're all here already.

https://codereview.chromium.org/328243005/diff/20001/Source/core/dom/ElementR...
Source/core/dom/ElementRareData.h:237: WillBeHeapVector<RefPtrWillBeMember<Attr>
>* attrNodeList() { return m_attrNodeList.get(); }
const

[Mads Ager (chromium), 2014-06-17 08:32:31]

The latest patch set does not increase size because Kent has combined the two
fields in the rare data. The size concern is for patch set one where all element
rare data objects get an extra pointer field regardless of whether attributes
are used or not.

[Mads Ager (chromium), 2014-06-17 08:35:09]

https://codereview.chromium.org/328243005/diff/20001/Source/core/dom/ElementR...
File Source/core/dom/ElementRareData.cpp (right):

https://codereview.chromium.org/328243005/diff/20001/Source/core/dom/ElementR...
Source/core/dom/ElementRareData.cpp:93: #endif
On 2014/06/17 08:27:05, esprehn wrote:
> This code doesn't make sense. We should never call trace() methods when not in
> oilpan mode. Remove this ifdef.

They are never called, but they are compiled.

[Mads Ager (chromium), 2014-06-17 08:45:10]

Looking at this again I like patch set 2. For patch set 2 we are not adding
memory overhead for all ElementRareData and only add a little more overhead if
you are using the NamedNodeMap or AttributeNodeMap.

Patch set 2 therefore LGTM with Elliot's comments addressed, but Elliot should
sign off on it as well. :)

[Mads Ager (chromium), 2014-06-17 09:49:04]

Kent, as a sanity check, could you measure the amount of AttrData objects that
you get allocated that do not have a AttrNodeList while browsing around on
common sites? In other words, how often is the NamedNodeMap used where the
AttrNodeList is not used. If that happens a lot patch set two can also add quite
a bit of overhead (two extra pointers plus internal allocator overhead because
the AttrData object is separately allocated). At a quick glance it looks like
pages like nyt.com actually use the NamedNodeMap quite a bit but not the
AttrNodeList. Since the AttrNodeList is so rarely used it might be best to leave
this in the global map instead...

[esprehn, 2014-06-17 11:06:04]

The only way I see to use AttrNodeList without using the NamedNodeMap is to use
the deprecated get/setAttributeNode methods. I don't think we should care much
about that. I'm also not sure it really matters if you make ElementRareData one
pointer bigger. There's lots of cases to make it smaller, for example we can
combine the 3 ptrs for PseudoElements into PseudoElementData, we can get rid of
the InputMethodContext ptr since that feature doesn't even ship...

I'd suggest doing whatever is simplest. I don't think you need to do all this
measurement either, we've never done that much for other things we've added to
ElementRareData, and we've added many ptrs (at least 3 by my count) worth of
stuff in the past year.

https://codereview.chromium.org/328243005/diff/20001/Source/core/dom/ElementR...
File Source/core/dom/ElementRareData.cpp (right):

https://codereview.chromium.org/328243005/diff/20001/Source/core/dom/ElementR...
Source/core/dom/ElementRareData.cpp:93: #endif
Why won't think compile in non-oilpan mode? It's not at all clear here why the
trace() call below is fine outside the ifdef, but the one above requires it.

[Mads Ager (chromium), 2014-06-17 11:40:22]

On 2014/06/17 11:06:04, esprehn wrote:
> The only way I see to use AttrNodeList without using the NamedNodeMap is to
use
> the deprecated get/setAttributeNode methods.

It is the other way around I'm worries about. Using NamedNodeMap without using
AttrNodeList. I think that could happen a lot and patch set 2 would add overhead
to that.

> I don't think we should care much
> about that. I'm also not sure it really matters if you make ElementRareData
one
> pointer bigger. There's lots of cases to make it smaller, for example we can
> combine the 3 ptrs for PseudoElements into PseudoElementData, we can get rid
of
> the InputMethodContext ptr since that feature doesn't even ship...
> 
> I'd suggest doing whatever is simplest. I don't think you need to do all this
> measurement either, we've never done that much for other things we've added to
> ElementRareData, and we've added many ptrs (at least 3 by my count) worth of
> stuff in the past year.

Alright! In that case I would support just going with patch set 1 which adds an
extra pointer to element rare data. That is clearly the simplest approach.
Thanks Elliot. :)

>
https://codereview.chromium.org/328243005/diff/20001/Source/core/dom/ElementR...
> File Source/core/dom/ElementRareData.cpp (right):
> 
>
https://codereview.chromium.org/328243005/diff/20001/Source/core/dom/ElementR...
> Source/core/dom/ElementRareData.cpp:93: #endif
> Why won't think compile in non-oilpan mode? It's not at all clear here why the
> trace() call below is fine outside the ifdef, but the one above requires it.

Yeah, I know. :-/ I think we should ifdef the entire body here. When parts of
the trace method do not compile we should just ifdef out the entire thing since
it will not be called non-oilpan anyway.

The history behind this is that we attempted in the beginning to make sure that
the trace methods could just compile with and without oilpan. However, that was
probably a mistake and there are cases where we cannot easily make it compile
and shouldn't spent effort attempting to. Therefore, we now end up ifdef'ing out
many of them as we go.

I think we should ifdef out the entire body here.

[haraken, 2014-06-17 11:46:30]

> > I don't think we should care much
> > about that. I'm also not sure it really matters if you make ElementRareData
> one
> > pointer bigger. There's lots of cases to make it smaller, for example we can
> > combine the 3 ptrs for PseudoElements into PseudoElementData, we can get rid
> of
> > the InputMethodContext ptr since that feature doesn't even ship...
> > 
> > I'd suggest doing whatever is simplest. I don't think you need to do all
this
> > measurement either, we've never done that much for other things we've added
to
> > ElementRareData, and we've added many ptrs (at least 3 by my count) worth of
> > stuff in the past year.
> 
> Alright! In that case I would support just going with patch set 1 which adds
an
> extra pointer to element rare data. That is clearly the simplest approach.
> Thanks Elliot. :)

(FWIW, we're now evaluating memory impact of adding extra 8 byte to Node and 16
byte to CSSValue using real-world web sites (for other purposes). The conclusion
is going to be "it doesn't matter".)

[tkent, 2014-06-17 23:42:32]

Everyone, thank you for comments.

I agree that we should try the simplest solution first, then resolve issues if
it causes issues.
I'll land Patch Set 3, which is Patch Set 1 + minor changes.

[tkent, 2014-06-18 00:15:54]

The CQ bit was checked by tkent@chromium.org

[tkent, 2014-06-18 00:16:02]

The CQ bit was unchecked by tkent@chromium.org

[tkent, 2014-06-18 00:18:41]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-06-18 00:19:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tkent@chromium.org/328243005/40001

[commit-bot: I haz the power, 2014-06-18 00:23:45]

Change committed as 176366