Bug 10818 - String::append does 2 full copies instead of 1 (or zero!)
Summary: String::append does 2 full copies instead of 1 (or zero!)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-09-11 19:38 PDT by Eric Seidel (no email)
Modified: 2007-10-31 01:32 PDT (History)
1 user (show)

See Also:


Attachments
proposed fix (2.71 KB, patch)
2007-10-30 07:15 PDT, Alexey Proskuryakov
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2006-09-11 19:38:34 PDT
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*)
Comment 1 Eric Seidel (no email) 2006-12-21 03:52:48 PST
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());
    }
}
Comment 2 Alexey Proskuryakov 2007-10-30 07:15:47 PDT
Created attachment 16947 [details]
proposed fix

Granted, String::append() is not about performance, but I guess someone has to fix this, after all :-)
Comment 3 Darin Adler 2007-10-30 22:56:11 PDT
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.
Comment 4 Eric Seidel (no email) 2007-10-30 22:58:44 PDT
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".
Comment 5 Alexey Proskuryakov 2007-10-31 01:32:01 PDT
Committed revision 27300.