Bug 10818 – String::append does 2 full copies instead of 1 (or zero!)

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Eric Seidel (no email)]

I'm surprised this hasn't been noticed before.

It seems String::append is about as inefficient as possible.

String::append results in 2 full copies of the string being made.  That's one more than necessary.  If String was even smarter, it would pre-allocate a certain amount of extra space, resulting in 0 necessary copies.

Safari hung for several minutes loading a 30MB SVG file.  Pretty much all of that time was spent copying strings for String::append:


	0.0%	100.0%	WebCore	WebCore::Frame::addData(char const*, int)	
	0.0%	100.0%	WebCore	WebCore::Frame::write(char const*, int)	
	0.0%	100.0%	WebCore	WebCore::Decoder::decode(char const*, unsigned long)	
	0.0%	100.0%	WebCore	WebCore::TextDecoder::decode(char const*, unsigned long, bool)	
	0.0%	100.0%	WebCore	WebCore::TextDecoder::checkForBOM(char const*, unsigned long, bool)	
	0.6%	100.0%	WebCore	WebCore::TextCodecICU::decode(char const*, unsigned long, bool)	
	0.5%	99.4%	WebCore	WebCore::TextCodec::appendOmittingBOM(WebCore::String&, unsigned short const*, unsigned long)	
	0.0%	98.8%	WebCore	WebCore::String::append(WebCore::String const&)	
	0.0%	49.8%	WebCore	WebCore::StringImpl::copy() const	
	48.9%	48.9%	WebCore	WebCore::StringImpl::append(WebCore::StringImpl const*)

[Eric Seidel (no email)]

Wow.  This is still the case:

void String::append(const String &str)
{
    if (str.m_impl) {
        if (!m_impl) {
            // ### FIXME!!!
            m_impl = str.m_impl;
            return;
        }
        m_impl = m_impl->copy();
        m_impl->append(str.m_impl.get());
    }
}

[Alexey Proskuryakov]

Created attachment 16947 [details]
proposed fix

Granted, String::append() is not about performance, but I guess someone has to fix this, after all :-)

[Darin Adler]

Comment on attachment 16947 [details]
proposed fix

r=me

But these are so inefficient we might some day decide to get rid of them just to encourage people to use some other idiom.

[Eric Seidel (no email)]

I can outfit you with the SVG in question if you like.  maciej and olliej probably have a copy as well, it's called "plan".

[Alexey Proskuryakov]

Committed revision 27300.