Bug 76458 – SpaceSplitString: Share equivalent string piece vectors.

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

[Andreas Kling]

SpaceSplitString is used primarily to store element class names. We can save a bunch of memory by sharing the SpaceSplitStringData for SpaceSplitStrings that are based on the same strings. (This also means we only need to split each string once.)

[Andreas Kling]

Created attachment 122781 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/dom/SpaceSplitString.h:30:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/dom/SpaceSplitString.h:72:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 3 files


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

[Early Warning System Bot]

Comment on attachment 122781 [details]
Patch

Attachment 122781 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11268245

[Antti Koivisto]

Comment on attachment 122781 [details]
Patch

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

> Source/WebCore/dom/SpaceSplitString.cpp:141
> +static void deleteFromSharedDataMap(SpaceSplitStringData* data)
> +{
> +    SpaceSplitStringDataMap& map = sharedDataMap();
> +    SpaceSplitStringDataMap::iterator end = map.end();
> +    for (SpaceSplitStringDataMap::iterator it = map.begin(); it != end; ++it) {
> +        if (it->second.get() == data) {
> +            map.remove(it);
> +            return;
> +        }
> +    }
> +}

This looks like O(n^2) for SpaceSplitStringData's.

> Source/WebCore/dom/SpaceSplitString.cpp:147
> +    // If this is the last reference to 'data', remove it from cache.
> +    if (data && data->refCount() == 2)
> +        deleteFromSharedDataMap(data);

Testing for ref count 2 is strange. I think it would be cleaner to use plain pointers in the global cache and simply remove the cache entry from SpaceSplitStringData destructor.

[Andreas Kling]

Created attachment 122790 [details]
Patch v2

Anttified patch.

[Andreas Kling]

Created attachment 122791 [details]
Patch v2

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/dom/SpaceSplitString.h:30:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 3 files


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

[Antti Koivisto]

Comment on attachment 122791 [details]
Patch v2

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

looks good, r=me

> Source/WebCore/dom/SpaceSplitString.cpp:161
> +PassRefPtr<SpaceSplitStringData> SpaceSplitStringData::create(const SpaceSplitStringData& other)

Maybe createUnique() or similar?

> Source/WebCore/dom/SpaceSplitString.cpp:178
> +    // Note that we don't copy m_string to indicate to the destructor that there's nothing
> +    // to be removed from the sharedDataMap().

You might want to rename m_string to m_stringKey or similar to indicate its purpose.

[Early Warning System Bot]

Comment on attachment 122791 [details]
Patch v2

Attachment 122791 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11107688

[Andreas Kling]

Committed r105186: <http://trac.webkit.org/changeset/105186>