Bug 16159 – String optimization- remove realloc from StringImpl::adopt

• Status: RESOLVED
• Resolution: INVALID
• Component: Platform

[Mike Belshe]

While doing some work which needed to import external strings into WebCore strings, I tried to use adopt to avoid buffer copies, but found it to be slow.  It does both a realloc() and also a malloc().

From discussing with bdash, it seems that on platforms using fastmalloc the realloc() call may be cheap.  However, on the windows build w/o fastmalloc, the realloc call is very significant; in my profiling it accounts for 46% of the time spent in StringImpl::adopt().  You can see the source to realloc() on windows in the CRT runtime sources (usually located in c:\Program Files\Microsoft Visual Studio 8\VC\crt\src\realloc.c).  You can see that it is not cheap :-)

I believe there is a simple optimization which dramatically helps the adopt() performance in StringImpl.cpp:

StringImpl* StringImpl::adopt(Vector<UChar>& buffer)
{
    size_t size = buffer.size();
    UChar* data = buffer.releaseBuffer();

    // Just remove this line!
    //data = static_cast<UChar*>(fastRealloc(data, size * sizeof(UChar)));   

    StringImpl* result = new StringImpl;
    result->m_length = size;
    result->m_data = data;
    return result;
}

There is a potential downside to this; if the vector being adopted had a large unused capacity, then this call would no longer reclaim that extra space.  However, from my testing, I've found no cases where this is significant, while the performance delta is significant.

BTW - I did discover that I can use StringImpl::newUninitialized() as a workaround in my case.  But I still believe adopt() should be cheap and that this is a good optimization.

[Alexey Proskuryakov]

(In reply to comment #0)
> There is a potential downside to this; if the vector being adopted had a large
> unused capacity, then this call would no longer reclaim that extra space. 

I think that this is exactly the reason why the buffer is reallocated - the vector capacity is doubled as it runs out of space, so it's quite common to have a lot of extra capacity.

The performance impact may make us reconsider this. Could you please attach a patch and mark it for review, as described in <http://webkit.org/coding/contributing.html>?

[Mike Belshe]

The code has been changed since this bug was filed, and the bug is no longer valid.