Bug 136830 – Use an AtomicString as key for caching ClassNodeList objects

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

Use an AtomicString as key for caching ClassNodeList objects instead of a String. ClassNodeList is the only type using a String instead of an AtomicString as key in the cache HashTable. This brings some complexity.
I believe this was done to avoid unnecessarily atomizing the String, for performance reasons. However, at the moment, the String gets atomized anyway when constructing the ClassNodeList object. This is because the ClassNodeList::m_classNames member is of SpaceSplitString type and the SpaceSplitString constructor takes an AtomicString in argument.

[Chris Dumez]

Created attachment 238137 [details]
Patch

[WebKit Commit Bot]

Attachment 238137 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/NodeRareData.h:311:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 238137 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238137&action=review

> Source/WebCore/dom/NodeRareData.h:312
> +        + m_tagNodeListCacheNS.size() + m_cachedCollections.size() != 1) {

I think I did what the style checker asked (add the curly brackets), it is still complaining though.

[Benjamin Poulain]

Comment on attachment 238137 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238137&action=review

> Source/WebCore/ChangeLog:23
> +        No new tests, no behavior change.

I disagree. :)

If not already covered, you should have a test ensuring that there are no name conflicts in the cache.

For example:
   getElemensByTagName("foo")
   getElementsByClassNames("foo")
   ... etc.
should have independent node lists.

If such a test already exist, you should mention it in the changelog.

> Source/WebCore/dom/ClassNodeList.h:45
> +    static PassRef<ClassNodeList> create(ContainerNode& rootNode, const AtomicString& classNames)
>      {
>          return adoptRef(*new ClassNodeList(rootNode, classNames));
>      }

Looks like poor inlining. The create() function is inline and the constructor is out of line.

> Source/WebCore/dom/ContainerNode.cpp:1057
>  PassRefPtr<NodeList> ContainerNode::getElementsByClassName(const String& classNames)

You should convert this argument to AtomicString to avoid any conversion on the call sites.

> Source/WebCore/dom/NodeRareData.h:-139
> -        NodeListNameCacheMap::AddResult result = m_nameCaches.fastAdd(namedNodeListKey<T>(name), nullptr);

Shouldn't you also update namedNodeListKey() to remove the generalization?

[Chris Dumez]

Comment on attachment 238137 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238137&action=review

>> Source/WebCore/ChangeLog:23
>> +        No new tests, no behavior change.
> 
> I disagree. :)
> 
> If not already covered, you should have a test ensuring that there are no name conflicts in the cache.
> 
> For example:
>    getElemensByTagName("foo")
>    getElementsByClassNames("foo")
>    ... etc.
> should have independent node lists.
> 
> If such a test already exist, you should mention it in the changelog.

I will check if we have a test and I can easily add one if missing. In any case, this case is not an issue because both the <NodeList Type, AtomicString> pair is used for hashing.

>> Source/WebCore/dom/NodeRareData.h:-139
>> -        NodeListNameCacheMap::AddResult result = m_nameCaches.fastAdd(namedNodeListKey<T>(name), nullptr);
> 
> Shouldn't you also update namedNodeListKey() to remove the generalization?

I updated namedNodeListKey() below already, to take an AtomicString in argument instead of a String. Note that the generalization is for the NodeList type, not the String type though.

[Ryosuke Niwa]

Comment on attachment 238137 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238137&action=review

>> Source/WebCore/dom/NodeRareData.h:312
>> +        + m_tagNodeListCacheNS.size() + m_cachedCollections.size() != 1) {
> 
> I think I did what the style checker asked (add the curly brackets), it is still complaining though.

We shouldn't have curly brackets here.  "return false" is a single line statement.

[Chris Dumez]

Comment on attachment 238137 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238137&action=review

>>> Source/WebCore/dom/NodeRareData.h:312
>>> +        + m_tagNodeListCacheNS.size() + m_cachedCollections.size() != 1) {
>> 
>> I think I did what the style checker asked (add the curly brackets), it is still complaining though.
> 
> We shouldn't have curly brackets here.  "return false" is a single line statement.

I added them because I thought this was what the style script meant:
"ERROR: Source/WebCore/dom/NodeRareData.h:311:  Multi line control clauses should use braces."

Anyway, I'll remove them since the script still isn't happy.

[Chris Dumez]

Created attachment 238152 [details]
Patch

[WebKit Commit Bot]

Attachment 238152 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/NodeRareData.h:311:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 238137 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238137&action=review

>>> Source/WebCore/ChangeLog:23
>>> +        No new tests, no behavior change.
>> 
>> I disagree. :)
>> 
>> If not already covered, you should have a test ensuring that there are no name conflicts in the cache.
>> 
>> For example:
>>    getElemensByTagName("foo")
>>    getElementsByClassNames("foo")
>>    ... etc.
>> should have independent node lists.
>> 
>> If such a test already exist, you should mention it in the changelog.
> 
> I will check if we have a test and I can easily add one if missing. In any case, this case is not an issue because both the <NodeList Type, AtomicString> pair is used for hashing.

I added a test even (my patch does not change web-exposed behavior though) and the test would pass with or without my patch.

>> Source/WebCore/dom/ClassNodeList.h:45
>>      }
> 
> Looks like poor inlining. The create() function is inline and the constructor is out of line.

Done.

>> Source/WebCore/dom/ContainerNode.cpp:1057
>>  PassRefPtr<NodeList> ContainerNode::getElementsByClassName(const String& classNames)
> 
> You should convert this argument to AtomicString to avoid any conversion on the call sites.

Done (it is only called by the JS bindings).

>>>> Source/WebCore/dom/NodeRareData.h:312
>>>> +        + m_tagNodeListCacheNS.size() + m_cachedCollections.size() != 1) {
>>> 
>>> I think I did what the style checker asked (add the curly brackets), it is still complaining though.
>> 
>> We shouldn't have curly brackets here.  "return false" is a single line statement.
> 
> I added them because I thought this was what the style script meant:
> "ERROR: Source/WebCore/dom/NodeRareData.h:311:  Multi line control clauses should use braces."
> 
> Anyway, I'll remove them since the script still isn't happy.

Done.

[Benjamin Poulain]

Comment on attachment 238152 [details]
Patch

Looks great. Thanks!

[Benjamin Poulain]

(In reply to comment #10)
> > I will check if we have a test and I can easily add one if missing. In any case, this case is not an issue because both the <NodeList Type, AtomicString> pair is used for hashing.
> 
> I added a test even (my patch does not change web-exposed behavior though) and the test would pass with or without my patch.

I was not doubting the correctness of your patch. It did look absolutely correct the first round.

The reason I asked for the test is to improve the test coverage in case any future change breaks this. Since we are not rushing to finish a release, we can take our time to make perfect patches. We should take any opportunity to improve our (terrifyingly bad) test coverage.

[WebKit Commit Bot]

Comment on attachment 238152 [details]
Patch

Clearing flags on attachment: 238152

Committed r173648: <http://trac.webkit.org/changeset/173648>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.