Use a StringImpl* instead of an AtomicString for NodeListAtomicNameCacheMap's key

Use a StringImpl* instead of an AtomicString for NodeListAtomicNameCacheMap's key

Use a StringImpl* instead of an AtomicString for NodeListAtomicNameCacheMap's key.
This results in slightly better performance, mainly by avoiding refcounting
churn on the StringImpl.

This improves Dromaeo's dom-attr test score by ~5.5% on my Linux desktop.
When profiling Dromaeo's dom-attr, I noticed that
NodeListsNodeData::invalidateCaches() was hot (6.7% of time spent), this patch
makes it go down to 5.0%.

This patch minimizes the performance regression caused by r166263, which made
getElementsByTagName() return an HTMLCollection instead of a NodeList. According
to the profiler, the performance impact is mainly due to node lists invalidation,
which is now occuring more frequently because HTMLCollection has an id/name cache
and therefore needs to be invalidated on id/name attribute change.

R=abarth, adamk
BUG=340325
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=166796

[abarth-chromium, 2014-02-09 03:46:27]

https://codereview.chromium.org/141743010/diff/1/Source/core/dom/NodeRareData.h
File Source/core/dom/NodeRareData.h (right):

https://codereview.chromium.org/141743010/diff/1/Source/core/dom/NodeRareData...
Source/core/dom/NodeRareData.h:242: return std::pair<unsigned char,
StringImpl*>(type, name.impl());
Why is this memory safe?  What retains |name| ?

[Inactive, 2014-02-09 14:06:53]

https://codereview.chromium.org/141743010/diff/1/Source/core/dom/NodeRareData.h
File Source/core/dom/NodeRareData.h (right):

https://codereview.chromium.org/141743010/diff/1/Source/core/dom/NodeRareData...
Source/core/dom/NodeRareData.h:242: return std::pair<unsigned char,
StringImpl*>(type, name.impl());
On 2014/02/09 03:46:27, abarth wrote:
> Why is this memory safe?  What retains |name| ?

Nothing retains name. This is safe because we are never doing anything with this
StringImpl* besides pointer comparison. It merely serves as a unique identifier
in the HashTable. It seems to be quite common in Blink to use atomic StringImpl*
for keys in Hash Tables without ref'ing them (see for e.g.
DocumentOrderedMap.h).

[Inactive, 2014-02-09 14:28:01]

https://codereview.chromium.org/141743010/diff/1/Source/core/dom/NodeRareData.h
File Source/core/dom/NodeRareData.h (right):

https://codereview.chromium.org/141743010/diff/1/Source/core/dom/NodeRareData...
Source/core/dom/NodeRareData.h:242: return std::pair<unsigned char,
StringImpl*>(type, name.impl());
On 2014/02/09 14:06:54, Chris Dumez wrote:
> On 2014/02/09 03:46:27, abarth wrote:
> > Why is this memory safe?  What retains |name| ?
> 
> Nothing retains name. This is safe because we are never doing anything with
this
> StringImpl* besides pointer comparison. It merely serves as a unique
identifier
> in the HashTable. It seems to be quite common in Blink to use atomic
StringImpl*
> for keys in Hash Tables without ref'ing them (see for e.g.
> DocumentOrderedMap.h).

Never mind my previous comment, it is inaccurate. The AtomicStrings are retained
by the NodeLists (e.g. HTMLNameCollection.m_name, TagCollection.m_localName,
...). And the node lists are in charge of removing themselves from the cache on
deletion.

[Inactive, 2014-02-09 15:23:28]

Here is the diff on the generated asm code from invalidateCaches:
http://pastebin.com/3Zw9UQMw

As you can see, code related to ref counting is gone.

[abarth-chromium, 2014-02-09 16:55:17]

LGTM.  Please add a comment that explains the lifetime.

BTW, it's not safe to use deallocated points, even to compare by pointer.  The
problem is that another string might be allocated at the same address.

[Inactive, 2014-02-09 17:14:03]

On 2014/02/09 16:55:17, abarth wrote:
> LGTM.  Please add a comment that explains the lifetime.

Will do, thanks.

> BTW, it's not safe to use deallocated points, even to compare by pointer.  The
> problem is that another string might be allocated at the same address.

Yes, I realized that afterwards, which is why I looked further. This will teach
me to comment before my coffee :)

[Inactive, 2014-02-09 17:48:04]

On 2014/02/09 16:55:17, abarth wrote:
> LGTM.  Please add a comment that explains the lifetime.

Done.

[abarth-chromium, 2014-02-09 18:38:23]

lgtm

[Inactive, 2014-02-09 18:47:18]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-02-09 18:47:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/141743010/130001

[commit-bot: I haz the power, 2014-02-09 20:28:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-09 20:28:03]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[Inactive, 2014-02-09 23:58:26]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-02-09 23:58:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/141743010/130001

[commit-bot: I haz the power, 2014-02-10 01:50:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-10 01:50:52]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[Inactive, 2014-02-10 02:01:54]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-02-10 02:02:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/141743010/130001

[commit-bot: I haz the power, 2014-02-10 04:04:19]

Change committed as 166796