[webkit-reviews] review denied: [Bug 76458] SpaceSplitString: Share equivalent string piece vectors. : [Attachment 122781] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has denied Andreas Kling <kling at webkit.org>'s
request for review:
Bug 76458: SpaceSplitString: Share equivalent string piece vectors.
https://bugs.webkit.org/show_bug.cgi?id=76458

Attachment 122781: Patch
https://bugs.webkit.org/attachment.cgi?id=122781&action=review

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
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.