90247 2012-06-28 21:56:15 -0700 Use StringBuilder in SegmentedString::toString() 2012-06-29 00:07:06 -0700 1 1 1 Unclassified WebKit Platform 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 0 skyul webkit-unassigned abarth eric webkit.review.bot yurys oldest_to_newest 660127 0 skyul 2012-06-28 21:56:15 -0700 Use a StringBuilder instead of String concatenation because StringBuilder is generally faster. 660128 1 150082 skyul 2012-06-28 21:58:59 -0700 Created attachment 150082 Patch 660140 2 150082 abarth 2012-06-28 22:33:06 -0700 Comment on attachment 150082 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150082&action=review > Source/WebCore/ChangeLog:9 > + Use a StringBuilder instead of String concatenation because StringBuilder is generally faster. > + No new tests. Covered by existing tests. Does this actually make things faster? If so, by how much? 660153 3 skyul 2012-06-28 23:06:29 -0700 (In reply to comment #2) > (From update of attachment 150082 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=150082&action=review > > > Source/WebCore/ChangeLog:9 > > + Use a StringBuilder instead of String concatenation because StringBuilder is generally faster. > > + No new tests. Covered by existing tests. > > Does this actually make things faster? If so, by how much? The only caller of SegmentedString::toString is XMLDocumentParser::append(const SegmentedString&). This method retrieves parseString from the argument s as in the following: void XMLDocumentParser::append(const SegmentedString& s) { String parseString = s.toString(); ... if (m_parserPaused) { m_pendingSrc.append(s); return; } ... } Because callers of XMLDocumentParser::append usually pass a String, the SegmentedString s usually consists of a single String. In this case, this patch might not improve performance. However, if the parser is paused, s is appended to m_pendingSrc. When the parser is resumed, m_pendingSrc is appended again. Now we have a SegmentedString with multiple substrings. I guess this patch improves the performance in this case. I admit this is a micro optimization, so I am not sure if I can measure the performance improvement in a statistically valid method. If StringBuilder with a single append performs worse than a Single string concatenation, I am skeptical of this being a noticeable improvement 660160 4 150082 abarth 2012-06-28 23:16:39 -0700 Comment on attachment 150082 Patch Ok. I'm not sure whether this is actually a perf win, but I'm willing to say we should do this on general principles. 660193 5 150082 webkit.review.bot 2012-06-29 00:07:01 -0700 Comment on attachment 150082 Patch Clearing flags on attachment: 150082 Committed r121523: <http://trac.webkit.org/changeset/121523> 660194 6 webkit.review.bot 2012-06-29 00:07:06 -0700 All reviewed patches have been landed. Closing bug. 150082 2012-06-28 21:58:59 -0700 2012-06-29 00:07:01 -0700 Patch bug-90247-20120629135849.patch text/plain 2862 skyul U3VidmVyc2lvbiBSZXZpc2lvbjogMTIxNTE3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNmRkMTYzNjdmNjBkOTRk OGQzOWRjZTU3MGQxZjM0M2QwYmJhNTg3Yi4uNDk3ZDAwNGU2MDdiNTNmODMyNGFhZjM3MDFiZmJl MmI5ZjNiNzA0YyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE4IEBACisyMDEyLTA2LTI4ICBLd2Fu ZyBZdWwgU2VvICA8c2t5dWxAY29tcGFueTEwMC5uZXQ+CisKKyAgICAgICAgVXNlIFN0cmluZ0J1 aWxkZXIgaW4gU2VnbWVudGVkU3RyaW5nOjp0b1N0cmluZygpCisgICAgICAgIGh0dHBzOi8vYnVn cy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD05MDI0NworCisgICAgICAgIFJldmlld2VkIGJ5 IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFVzZSBhIFN0cmluZ0J1aWxkZXIgaW5zdGVhZCBv ZiBTdHJpbmcgY29uY2F0ZW5hdGlvbiBiZWNhdXNlIFN0cmluZ0J1aWxkZXIgaXMgZ2VuZXJhbGx5 IGZhc3Rlci4KKyAgICAgICAgTm8gbmV3IHRlc3RzLiBDb3ZlcmVkIGJ5IGV4aXN0aW5nIHRlc3Rz LgorCisgICAgICAgICogcGxhdGZvcm0vdGV4dC9TZWdtZW50ZWRTdHJpbmcuY3BwOgorICAgICAg ICAoV2ViQ29yZTo6U2VnbWVudGVkU3RyaW5nOjp0b1N0cmluZyk6CisgICAgICAgICogcGxhdGZv cm0vdGV4dC9TZWdtZW50ZWRTdHJpbmcuaDoKKyAgICAgICAgKFdlYkNvcmU6OlNlZ21lbnRlZFN1 YnN0cmluZzo6YXBwZW5kVG8pOgorCiAyMDEyLTA2LTI4ICBTdGVwaGVuIFdoaXRlICA8c2Vub3Ji bGFuY29AY2hyb21pdW0ub3JnPgogCiAgICAgICAgIEltcGxlbWVudCBmaWx0ZXIgdXJsKCkgZnVu Y3Rpb24uCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS90ZXh0L1NlZ21lbnRl ZFN0cmluZy5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS90ZXh0L1NlZ21lbnRlZFN0cmlu Zy5jcHAKaW5kZXggNTkxZTI0OTQxZjU4NWY4YTFlOTFlYjViMWJlNGRmYjEyNDZmZGFiNi4uYzE4 NTI3MTg3YjE4YzE5NDk0N2UxYTY3MjFhNDNkZjA2ZjZjNjI0NCAxMDA2NDQKLS0tIGEvU291cmNl L1dlYkNvcmUvcGxhdGZvcm0vdGV4dC9TZWdtZW50ZWRTdHJpbmcuY3BwCisrKyBiL1NvdXJjZS9X ZWJDb3JlL3BsYXRmb3JtL3RleHQvU2VnbWVudGVkU3RyaW5nLmNwcApAQCAtMTg0LDcgKzE4NCw3 IEBAIHZvaWQgU2VnbWVudGVkU3RyaW5nOjphZHZhbmNlU3Vic3RyaW5nKCkKIAogU3RyaW5nIFNl Z21lbnRlZFN0cmluZzo6dG9TdHJpbmcoKSBjb25zdAogewotICAgIFN0cmluZyByZXN1bHQ7Cisg ICAgU3RyaW5nQnVpbGRlciByZXN1bHQ7CiAgICAgaWYgKG1fcHVzaGVkQ2hhcjEpIHsKICAgICAg ICAgcmVzdWx0LmFwcGVuZChtX3B1c2hlZENoYXIxKTsKICAgICAgICAgaWYgKG1fcHVzaGVkQ2hh cjIpCkBAIC0xOTcsNyArMTk3LDcgQEAgU3RyaW5nIFNlZ21lbnRlZFN0cmluZzo6dG9TdHJpbmco KSBjb25zdAogICAgICAgICBmb3IgKDsgaXQgIT0gZTsgKytpdCkKICAgICAgICAgICAgIGl0LT5h cHBlbmRUbyhyZXN1bHQpOwogICAgIH0KLSAgICByZXR1cm4gcmVzdWx0OworICAgIHJldHVybiBy ZXN1bHQudG9TdHJpbmcoKTsKIH0KIAogdm9pZCBTZWdtZW50ZWRTdHJpbmc6OmFkdmFuY2UodW5z aWduZWQgY291bnQsIFVDaGFyKiBjb25zdW1lZENoYXJhY3RlcnMpCmRpZmYgLS1naXQgYS9Tb3Vy Y2UvV2ViQ29yZS9wbGF0Zm9ybS90ZXh0L1NlZ21lbnRlZFN0cmluZy5oIGIvU291cmNlL1dlYkNv cmUvcGxhdGZvcm0vdGV4dC9TZWdtZW50ZWRTdHJpbmcuaAppbmRleCA0NDYyMTgzODhkMWZjNzU2 OGI4Y2YxMDJiYTJkZjU2MzE5MWFjYjk2Li41ZTBiNTkxZmY1OGZkNDA4MTI0ZWQxZTM0ZmZhZjZm MjdlZTViMDIzIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS90ZXh0L1NlZ21l bnRlZFN0cmluZy5oCisrKyBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL3RleHQvU2VnbWVudGVk U3RyaW5nLmgKQEAgLTIyLDYgKzIyLDcgQEAKIAogI2luY2x1ZGUgIlBsYXRmb3JtU3RyaW5nLmgi CiAjaW5jbHVkZSA8d3RmL0RlcXVlLmg+CisjaW5jbHVkZSA8d3RmL3RleHQvU3RyaW5nQnVpbGRl ci5oPgogI2luY2x1ZGUgPHd0Zi90ZXh0L1RleHRQb3NpdGlvbi5oPgogCiBuYW1lc3BhY2UgV2Vi Q29yZSB7CkBAIC01NCwxNSArNTUsMTIgQEAgcHVibGljOgogCiAgICAgaW50IG51bWJlck9mQ2hh cmFjdGVyc0NvbnN1bWVkKCkgY29uc3QgeyByZXR1cm4gbV9zdHJpbmcubGVuZ3RoKCkgLSBtX2xl bmd0aDsgfQogCi0gICAgdm9pZCBhcHBlbmRUbyhTdHJpbmcmIHN0cikgY29uc3QKKyAgICB2b2lk IGFwcGVuZFRvKFN0cmluZ0J1aWxkZXImIGJ1aWxkZXIpIGNvbnN0CiAgICAgewotICAgICAgICBp ZiAobV9zdHJpbmcuY2hhcmFjdGVycygpID09IG1fY3VycmVudCkgewotICAgICAgICAgICAgaWYg KHN0ci5pc0VtcHR5KCkpCi0gICAgICAgICAgICAgICAgc3RyID0gbV9zdHJpbmc7Ci0gICAgICAg ICAgICBlbHNlCi0gICAgICAgICAgICAgICAgc3RyLmFwcGVuZChtX3N0cmluZyk7Ci0gICAgICAg IH0gZWxzZQotICAgICAgICAgICAgc3RyLmFwcGVuZChTdHJpbmcobV9jdXJyZW50LCBtX2xlbmd0 aCkpOworICAgICAgICBpZiAobV9zdHJpbmcuY2hhcmFjdGVycygpID09IG1fY3VycmVudCkKKyAg ICAgICAgICAgIGJ1aWxkZXIuYXBwZW5kKG1fc3RyaW5nKTsKKyAgICAgICAgZWxzZQorICAgICAg ICAgICAgYnVpbGRlci5hcHBlbmQoU3RyaW5nKG1fY3VycmVudCwgbV9sZW5ndGgpKTsKICAgICB9 CiAKIHB1YmxpYzoK