Bug 76458 - SpaceSplitString: Share equivalent string piece vectors.
Summary: SpaceSplitString: Share equivalent string piece vectors.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-17 08:22 PST by Andreas Kling
Modified: 2012-01-17 12:57 PST (History)
1 user (show)

See Also:


Attachments
Patch (8.63 KB, patch)
2012-01-17 10:09 PST, Andreas Kling
koivisto: review-
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Patch v2 (8.68 KB, patch)
2012-01-17 11:41 PST, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch v2 (8.68 KB, patch)
2012-01-17 11:42 PST, Andreas Kling
koivisto: review+
webkit-ews: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2012-01-17 08:22:04 PST
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.)
Comment 1 Andreas Kling 2012-01-17 10:09:23 PST
Created attachment 122781 [details]
Patch
Comment 2 WebKit Review Bot 2012-01-17 10:11:25 PST
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.
Comment 3 Early Warning System Bot 2012-01-17 10:23:22 PST
Comment on attachment 122781 [details]
Patch

Attachment 122781 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11268245
Comment 4 Antti Koivisto 2012-01-17 10:34:18 PST
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.
Comment 5 Andreas Kling 2012-01-17 11:41:17 PST
Created attachment 122790 [details]
Patch v2

Anttified patch.
Comment 6 Andreas Kling 2012-01-17 11:42:29 PST
Created attachment 122791 [details]
Patch v2
Comment 7 WebKit Review Bot 2012-01-17 11:54:07 PST
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.
Comment 8 Antti Koivisto 2012-01-17 12:01:36 PST
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.
Comment 9 Early Warning System Bot 2012-01-17 12:16:06 PST
Comment on attachment 122791 [details]
Patch v2

Attachment 122791 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11107688
Comment 10 Andreas Kling 2012-01-17 12:57:37 PST
Committed r105186: <http://trac.webkit.org/changeset/105186>