82028 2012-03-23 00:07:09 -0700 cssText should use StringBuilder 2012-03-23 04:37:33 -0700 1 1 1 Unclassified WebKit CSS 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 81737 1 rniwa rniwa abarth eric kling koivisto macpherson menard webkit.review.bot zimmermann oldest_to_newest 586136 0 rniwa 2012-03-23 00:07:09 -0700 StylePropertySet::asText should use StringBuilder instead of adding bunch of string literals and String by operator+ and operator+=. 586178 1 133446 rniwa 2012-03-23 02:06:51 -0700 Created attachment 133446 Cleanup 586193 2 rniwa 2012-03-23 02:52:36 -0700 Committed r111845: <http://trac.webkit.org/changeset/111845> 586211 3 zimmermann 2012-03-23 04:08:55 -0700 Just came across your commit message: "Make StylePropertySet::asText more efficient by deploying StringBuilder; avoids heap churn by String::operator+ and String::operator+=." First of all this change is great and correct. For cases like this StringBuilder should be used (if we don't know before of many parts it consists, aka. how big the final string is). But "String a = stringB + "C" + "D" + stringC" doesn't involve any heap churns anymore, only String::operator+= is inefficient, String::operator+ uses magic, to avoid any immediate heap allocations, by figuring out the final buffer size before. So whenever you want to concat three strings (not necessarily of the same types, eg. a const char* with a String and an AtomicString), using String::operator+ is the easiest and most performant way. (Have a look at the StringAppend<> template stuff). Just wanted to let you know :-) 586226 4 rniwa 2012-03-23 04:29:43 -0700 (In reply to comment #3) > Just came across your commit message: > "Make StylePropertySet::asText more efficient by deploying StringBuilder; avoids heap churn by String::operator+ and String::operator+=." > > First of all this change is great and correct. For cases like this StringBuilder should be used (if we don't know before of many parts it consists, aka. how big the final string is). > > But "String a = stringB + "C" + "D" + stringC" doesn't involve any heap churns anymore, only String::operator+= is inefficient, String::operator+ uses magic, to avoid any immediate heap allocations, by figuring out the final buffer size before. Okay, I wasn't sure if that optimization had been landed or not. Good to know it's in there :) Is it defined in WTFString.h? 586228 5 zimmermann 2012-03-23 04:37:33 -0700 (In reply to comment #4) > Okay, I wasn't sure if that optimization had been landed or not. Good to know it's in there :) Is it defined in WTFString.h? wtf/text/text/StringOperators.h and StringConcatenate.h contain the code. 133446 2012-03-23 02:06:51 -0700 2012-03-23 02:21:31 -0700 Cleanup bug-82028-20120323020650.patch text/plain 5292 rniwa SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDExMTgzOCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBACisyMDEyLTAzLTIzICBSeW9zdWtl IE5pd2EgIDxybml3YUB3ZWJraXQub3JnPgorCisgICAgICAgIGNzc1RleHQgc2hvdWxkIHVzZSBT dHJpbmdCdWlsZGVyCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNn aT9pZD04MjAyOAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgIE1ha2UgU3R5bGVQcm9wZXJ0eVNldDo6YXNUZXh0IG1vcmUgZWZmaWNpZW50IGJ5IGRlcGxv eWluZyBTdHJpbmdCdWlsZGVyOworICAgICAgICBhdm9pZHMgaGVhcCBjaHVybiBieSBTdHJpbmc6 Om9wZXJhdG9yKyBhbmQgU3RyaW5nOjpvcGVyYXRvcis9LgorCisgICAgICAgICogY3NzL1N0eWxl UHJvcGVydHlTZXQuY3BwOgorICAgICAgICAoV2ViQ29yZTo6U3R5bGVQcm9wZXJ0eVNldDo6YXNU ZXh0KToKKwogMjAxMi0wMy0yMyAgQWRhbSBCYXJ0aCAgPGFiYXJ0aEB3ZWJraXQub3JnPgogCiAg ICAgICAgIE1vdmUgTm90aWZpY2F0aW9ucyBBUElzIGZyb20gRE9NV2luZG93LmlkbCB0byBET01X aW5kb3dOb3RpZmljYXRpb25zLmlkbCAoUGFydCAyKQpJbmRleDogU291cmNlL1dlYkNvcmUvY3Nz L1N0eWxlUHJvcGVydHlTZXQuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL2Nzcy9T dHlsZVByb3BlcnR5U2V0LmNwcAkocmV2aXNpb24gMTExODMzKQorKysgU291cmNlL1dlYkNvcmUv Y3NzL1N0eWxlUHJvcGVydHlTZXQuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC02NDcsNyArNjQ3LDcg QEAgdm9pZCBTdHlsZVByb3BlcnR5U2V0OjphZGRQYXJzZWRQcm9wZXJ0eQogCiBTdHJpbmcgU3R5 bGVQcm9wZXJ0eVNldDo6YXNUZXh0KCkgY29uc3QKIHsKLSAgICBTdHJpbmcgcmVzdWx0ID0gIiI7 CisgICAgU3RyaW5nQnVpbGRlciByZXN1bHQ7CiAKICAgICBjb25zdCBDU1NQcm9wZXJ0eSogcG9z aXRpb25YUHJvcCA9IDA7CiAgICAgY29uc3QgQ1NTUHJvcGVydHkqIHBvc2l0aW9uWVByb3AgPSAw OwpAQCAtNjU3LDE2ICs2NTcsMjIgQEAgU3RyaW5nIFN0eWxlUHJvcGVydHlTZXQ6OmFzVGV4dCgp IGNvbnN0CiAgICAgdW5zaWduZWQgc2l6ZSA9IG1fcHJvcGVydGllcy5zaXplKCk7CiAgICAgZm9y ICh1bnNpZ25lZCBuID0gMDsgbiA8IHNpemU7ICsrbikgewogICAgICAgICBjb25zdCBDU1NQcm9w ZXJ0eSYgcHJvcCA9IG1fcHJvcGVydGllc1tuXTsKLSAgICAgICAgaWYgKHByb3AuaWQoKSA9PSBD U1NQcm9wZXJ0eUJhY2tncm91bmRQb3NpdGlvblgpCisgICAgICAgIHN3aXRjaCAocHJvcC5pZCgp KSB7CisgICAgICAgIGNhc2UgQ1NTUHJvcGVydHlCYWNrZ3JvdW5kUG9zaXRpb25YOgogICAgICAg ICAgICAgcG9zaXRpb25YUHJvcCA9ICZwcm9wOwotICAgICAgICBlbHNlIGlmIChwcm9wLmlkKCkg PT0gQ1NTUHJvcGVydHlCYWNrZ3JvdW5kUG9zaXRpb25ZKQorICAgICAgICAgICAgYnJlYWs7Cisg ICAgICAgIGNhc2UgQ1NTUHJvcGVydHlCYWNrZ3JvdW5kUG9zaXRpb25ZOgogICAgICAgICAgICAg cG9zaXRpb25ZUHJvcCA9ICZwcm9wOwotICAgICAgICBlbHNlIGlmIChwcm9wLmlkKCkgPT0gQ1NT UHJvcGVydHlCYWNrZ3JvdW5kUmVwZWF0WCkKKyAgICAgICAgICAgIGJyZWFrOworICAgICAgICBj YXNlIENTU1Byb3BlcnR5QmFja2dyb3VuZFJlcGVhdFg6CiAgICAgICAgICAgICByZXBlYXRYUHJv cCA9ICZwcm9wOwotICAgICAgICBlbHNlIGlmIChwcm9wLmlkKCkgPT0gQ1NTUHJvcGVydHlCYWNr Z3JvdW5kUmVwZWF0WSkKKyAgICAgICAgICAgIGJyZWFrOworICAgICAgICBjYXNlIENTU1Byb3Bl cnR5QmFja2dyb3VuZFJlcGVhdFk6CiAgICAgICAgICAgICByZXBlYXRZUHJvcCA9ICZwcm9wOwot ICAgICAgICBlbHNlCi0gICAgICAgICAgICByZXN1bHQgKz0gcHJvcC5jc3NUZXh0KCk7CisgICAg ICAgICAgICBicmVhazsKKyAgICAgICAgZGVmYXVsdDoKKyAgICAgICAgICAgIHJlc3VsdC5hcHBl bmQocHJvcC5jc3NUZXh0KCkpOworICAgICAgICB9CiAgICAgfQogCiAgICAgLy8gRklYTUU6IFRo aXMgaXMgYSBub3Qtc28tbmljZSB3YXkgdG8gdHVybiB4L3kgcG9zaXRpb25zIGludG8gc2luZ2xl IGJhY2tncm91bmQtcG9zaXRpb24gaW4gb3V0cHV0LgpAQCAtNjc2LDM1ICs2ODIsNDcgQEAgU3Ry aW5nIFN0eWxlUHJvcGVydHlTZXQ6OmFzVGV4dCgpIGNvbnN0CiAgICAgaWYgKHBvc2l0aW9uWFBy b3AgJiYgcG9zaXRpb25ZUHJvcCAmJiBwb3NpdGlvblhQcm9wLT5pc0ltcG9ydGFudCgpID09IHBv c2l0aW9uWVByb3AtPmlzSW1wb3J0YW50KCkpIHsKICAgICAgICAgU3RyaW5nIHBvc2l0aW9uVmFs dWU7CiAgICAgICAgIGNvbnN0IGludCBwcm9wZXJ0aWVzWzJdID0geyBDU1NQcm9wZXJ0eUJhY2tn cm91bmRQb3NpdGlvblgsIENTU1Byb3BlcnR5QmFja2dyb3VuZFBvc2l0aW9uWSB9OworICAgICAg ICByZXN1bHQuYXBwZW5kKCJiYWNrZ3JvdW5kLXBvc2l0aW9uOiAiKTsKICAgICAgICAgaWYgKHBv c2l0aW9uWFByb3AtPnZhbHVlKCktPmlzVmFsdWVMaXN0KCkgfHwgcG9zaXRpb25ZUHJvcC0+dmFs dWUoKS0+aXNWYWx1ZUxpc3QoKSkKLSAgICAgICAgICAgIHBvc2l0aW9uVmFsdWUgPSBnZXRMYXll cmVkU2hvcnRoYW5kVmFsdWUocHJvcGVydGllcyk7Ci0gICAgICAgIGVsc2UKLSAgICAgICAgICAg IHBvc2l0aW9uVmFsdWUgPSBwb3NpdGlvblhQcm9wLT52YWx1ZSgpLT5jc3NUZXh0KCkgKyAiICIg KyBwb3NpdGlvbllQcm9wLT52YWx1ZSgpLT5jc3NUZXh0KCk7Ci0gICAgICAgIHJlc3VsdCArPSAi YmFja2dyb3VuZC1wb3NpdGlvbjogIiArIHBvc2l0aW9uVmFsdWUgKyAocG9zaXRpb25YUHJvcC0+ aXNJbXBvcnRhbnQoKSA/ICIgIWltcG9ydGFudCIgOiAiIikgKyAiOyAiOworICAgICAgICAgICAg cmVzdWx0LmFwcGVuZChnZXRMYXllcmVkU2hvcnRoYW5kVmFsdWUocHJvcGVydGllcykpOworICAg ICAgICBlbHNlIHsKKyAgICAgICAgICAgIHJlc3VsdC5hcHBlbmQocG9zaXRpb25YUHJvcC0+dmFs dWUoKS0+Y3NzVGV4dCgpKTsKKyAgICAgICAgICAgIHJlc3VsdC5hcHBlbmQoIiAiKTsKKyAgICAg ICAgICAgIHJlc3VsdC5hcHBlbmQocG9zaXRpb25ZUHJvcC0+dmFsdWUoKS0+Y3NzVGV4dCgpKTsK KyAgICAgICAgfQorICAgICAgICBpZiAocG9zaXRpb25YUHJvcC0+aXNJbXBvcnRhbnQoKSkKKyAg ICAgICAgICAgIHJlc3VsdC5hcHBlbmQoIiAhaW1wb3J0YW50Iik7CisgICAgICAgIHJlc3VsdC5h cHBlbmQoIjsgIik7CiAgICAgfSBlbHNlIHsKICAgICAgICAgaWYgKHBvc2l0aW9uWFByb3ApCi0g ICAgICAgICAgICByZXN1bHQgKz0gcG9zaXRpb25YUHJvcC0+Y3NzVGV4dCgpOworICAgICAgICAg ICAgcmVzdWx0LmFwcGVuZChwb3NpdGlvblhQcm9wLT5jc3NUZXh0KCkpOwogICAgICAgICBpZiAo cG9zaXRpb25ZUHJvcCkKLSAgICAgICAgICAgIHJlc3VsdCArPSBwb3NpdGlvbllQcm9wLT5jc3NU ZXh0KCk7CisgICAgICAgICAgICByZXN1bHQuYXBwZW5kKHBvc2l0aW9uWVByb3AtPmNzc1RleHQo KSk7CiAgICAgfQogCiAgICAgLy8gRklYTUU6IFdlIG5lZWQgdG8gZG8gdGhlIHNhbWUgZm9yIGJh Y2tncm91bmQtcmVwZWF0LgogICAgIGlmIChyZXBlYXRYUHJvcCAmJiByZXBlYXRZUHJvcCAmJiBy ZXBlYXRYUHJvcC0+aXNJbXBvcnRhbnQoKSA9PSByZXBlYXRZUHJvcC0+aXNJbXBvcnRhbnQoKSkg ewogICAgICAgICBTdHJpbmcgcmVwZWF0VmFsdWU7CiAgICAgICAgIGNvbnN0IGludCByZXBlYXRQ cm9wZXJ0aWVzWzJdID0geyBDU1NQcm9wZXJ0eUJhY2tncm91bmRSZXBlYXRYLCBDU1NQcm9wZXJ0 eUJhY2tncm91bmRSZXBlYXRZIH07CisgICAgICAgIHJlc3VsdC5hcHBlbmQoImJhY2tncm91bmQt cmVwZWF0OiAiKTsKICAgICAgICAgaWYgKHJlcGVhdFhQcm9wLT52YWx1ZSgpLT5pc1ZhbHVlTGlz dCgpIHx8IHJlcGVhdFlQcm9wLT52YWx1ZSgpLT5pc1ZhbHVlTGlzdCgpKQotICAgICAgICAgICAg cmVwZWF0VmFsdWUgPSBnZXRMYXllcmVkU2hvcnRoYW5kVmFsdWUocmVwZWF0UHJvcGVydGllcyk7 Ci0gICAgICAgIGVsc2UKLSAgICAgICAgICAgIHJlcGVhdFZhbHVlID0gcmVwZWF0WFByb3AtPnZh bHVlKCktPmNzc1RleHQoKSArICIgIiArIHJlcGVhdFlQcm9wLT52YWx1ZSgpLT5jc3NUZXh0KCk7 Ci0gICAgICAgIHJlc3VsdCArPSAiYmFja2dyb3VuZC1yZXBlYXQ6ICIgKyByZXBlYXRWYWx1ZSAr IChyZXBlYXRYUHJvcC0+aXNJbXBvcnRhbnQoKSA/ICIgIWltcG9ydGFudCIgOiAiIikgKyAiOyAi OworICAgICAgICAgICAgcmVzdWx0LmFwcGVuZChnZXRMYXllcmVkU2hvcnRoYW5kVmFsdWUocmVw ZWF0UHJvcGVydGllcykpOworICAgICAgICBlbHNlIHsKKyAgICAgICAgICAgIHJlc3VsdC5hcHBl bmQocmVwZWF0WFByb3AtPnZhbHVlKCktPmNzc1RleHQoKSk7CisgICAgICAgICAgICByZXN1bHQu YXBwZW5kKCIgIik7CisgICAgICAgICAgICByZXN1bHQuYXBwZW5kKHJlcGVhdFlQcm9wLT52YWx1 ZSgpLT5jc3NUZXh0KCkpOworICAgICAgICB9CisgICAgICAgIGlmIChyZXBlYXRYUHJvcC0+aXNJ bXBvcnRhbnQoKSkKKyAgICAgICAgICAgIHJlc3VsdC5hcHBlbmQoIiAhaW1wb3J0YW50Iik7Cisg ICAgICAgIHJlc3VsdC5hcHBlbmQoIjsgIik7CiAgICAgfSBlbHNlIHsKICAgICAgICAgaWYgKHJl cGVhdFhQcm9wKQotICAgICAgICAgICAgcmVzdWx0ICs9IHJlcGVhdFhQcm9wLT5jc3NUZXh0KCk7 CisgICAgICAgICAgICByZXN1bHQuYXBwZW5kKHJlcGVhdFhQcm9wLT5jc3NUZXh0KCkpOwogICAg ICAgICBpZiAocmVwZWF0WVByb3ApCi0gICAgICAgICAgICByZXN1bHQgKz0gcmVwZWF0WVByb3At PmNzc1RleHQoKTsKKyAgICAgICAgICAgIHJlc3VsdC5hcHBlbmQocmVwZWF0WVByb3AtPmNzc1Rl eHQoKSk7CiAgICAgfQogCi0gICAgcmV0dXJuIHJlc3VsdDsKKyAgICByZXR1cm4gcmVzdWx0LnRv U3RyaW5nKCk7CiB9CiAKIHZvaWQgU3R5bGVQcm9wZXJ0eVNldDo6bWVyZ2UoY29uc3QgU3R5bGVQ cm9wZXJ0eVNldCogb3RoZXIsIGJvb2wgYXJnT3ZlcnJpZGVzT25Db25mbGljdCkK