44884 2010-08-30 12:14:47 -0700 Improve BlobBuilder to combine adjacent strings 2010-08-31 13:50:43 -0700 1 1 1 Unclassified WebKit WebCore JavaScript 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 jianli jianli dimich fishd kinuko levin michaeln oldest_to_newest 271554 0 jianli 2010-08-30 12:14:47 -0700 Improve BlobBuilder to combine adjacent strings. 271556 1 65936 jianli 2010-08-30 12:16:06 -0700 Created attachment 65936 Proposed Patch 271583 2 kinuko 2010-08-30 12:51:03 -0700 (In reply to comment #1) > Created an attachment (id=65936) [details] > Proposed Patch I wonder if this may end up copying strings too many times. What if the user script calls blobBuilder.append(string) hundreds of times? Can't we put a FIXME to optimize the performance later or isn't it better to concatenate the strings at once on getBlob()? 272181 3 jianli 2010-08-31 10:24:19 -0700 Committed as http://trac.webkit.org/changeset/66499. 272183 4 jianli 2010-08-31 10:24:58 -0700 (In reply to comment #2) > (In reply to comment #1) > > Created an attachment (id=65936) [details] [details] > > Proposed Patch > > I wonder if this may end up copying strings too many times. > What if the user script calls blobBuilder.append(string) hundreds of times? > Can't we put a FIXME to optimize the performance later or isn't it better to concatenate the strings at once on getBlob()? I think it is fine since this is what we also do for combining string in form data. 272317 5 kinuko 2010-08-31 13:28:05 -0700 (In reply to comment #4) > (In reply to comment #2) > > (In reply to comment #1) > > > Created an attachment (id=65936) [details] [details] [details] > > > Proposed Patch > > > > I wonder if this may end up copying strings too many times. > > What if the user script calls blobBuilder.append(string) hundreds of times? > > Can't we put a FIXME to optimize the performance later or isn't it better to concatenate the strings at once on getBlob()? > > I think it is fine since this is what we also do for combining string in form data. Do you mean FormData::appendData()? It calls Vector.grow() which utilizes memory (slightly) more efficiently and does not copy the original data if the new size fits in the reserved capacity. (I was wondering why we don't simply use Vector.append() though.) In this patch every time we call append we copy both original and new strings. It may make sense in a bigger redesign but I don't quite get it why we needed this particular small change separately. 272335 6 michaeln 2010-08-31 13:50:43 -0700 > > > I wonder if this may end up copying strings too many times. > > > What if the user script calls blobBuilder.append(string) hundreds of times? > > > Can't we put a FIXME to optimize the performance later or isn't it better to concatenate the strings at once on getBlob()? > > > > I think it is fine since this is what we also do for combining string in form data. > > Do you mean FormData::appendData()? > It calls Vector.grow() which utilizes memory (slightly) more efficiently and does not copy the original data if the new size fits in the reserved capacity. (I was wondering why we don't simply use Vector.append() though.) > In this patch every time we call append we copy both original and new strings. > It may make sense in a bigger redesign but I don't quite get it why we needed this particular small change separately. I agree with kinuko about this particular patch not really helping the matter. Maybe we should leave the code as is until something more thoughtful is put together. 65936 2010-08-30 12:16:06 -0700 2010-08-30 12:19:40 -0700 Proposed Patch 44884 text/plain 1808 jianli ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg YWRjZjNiYy4uYWZiZDNjNiAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNCBAQAorMjAxMC0wOC0zMCAgSmlhbiBMaSAgPGpp YW5saUBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISku CisKKyAgICAgICAgSW1wcm92ZSBCbG9iQnVpbGRlciB0byBjb21iaW5lIGFkamFjZW50IHN0cmlu Z3MuCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Bvc3RfYnVnLmNnaQorCisgICAg ICAgICogaHRtbC9CbG9iQnVpbGRlci5jcHA6CisgICAgICAgIChXZWJDb3JlOjphZGRUd29DU3Ry aW5ncyk6CisgICAgICAgIChXZWJDb3JlOjpCbG9iQnVpbGRlcjo6YXBwZW5kKToKKwogMjAxMC0w OC0yNSAgS2VubmV0aCBSdXNzZWxsICA8a2JyQGdvb2dsZS5jb20+CiAKICAgICAgICAgUmV2aWV3 ZWQgYnkgU2ltb24gRnJhc2VyLgpkaWZmIC0tZ2l0IGEvV2ViQ29yZS9odG1sL0Jsb2JCdWlsZGVy LmNwcCBiL1dlYkNvcmUvaHRtbC9CbG9iQnVpbGRlci5jcHAKaW5kZXggMDhkZWQxYi4uZmYwMjRh YiAxMDA2NDQKLS0tIGEvV2ViQ29yZS9odG1sL0Jsb2JCdWlsZGVyLmNwcAorKysgYi9XZWJDb3Jl L2h0bWwvQmxvYkJ1aWxkZXIuY3BwCkBAIC01Nyw2ICs1NywyMCBAQCBzdGF0aWMgQ1N0cmluZyBj b252ZXJ0VG9DU3RyaW5nKGNvbnN0IFN0cmluZyYgdGV4dCwgY29uc3QgU3RyaW5nJiBlbmRpbmdU eXBlLCBFeAogICAgIHJldHVybiBDU3RyaW5nKCk7CiB9CiAKK3N0YXRpYyBDU3RyaW5nIGFkZFR3 b0NTdHJpbmdzKGNvbnN0IENTdHJpbmcmIGEsIGNvbnN0IENTdHJpbmcmIGIpCit7CisgICAgaWYg KGEuaXNOdWxsKCkgJiYgYi5pc051bGwoKSkKKyAgICAgICAgcmV0dXJuIENTdHJpbmcoKTsKKwor ICAgIGNoYXIqIHE7CisgICAgQ1N0cmluZyByZXN1bHQgPSBDU3RyaW5nOjpuZXdVbmluaXRpYWxp emVkKGEubGVuZ3RoKCkgKyBiLmxlbmd0aCgpLCBxKTsKKyAgICBpZiAoYS5sZW5ndGgoKSkKKyAg ICAgICAgbWVtY3B5KHEsIGEuZGF0YSgpLCBhLmxlbmd0aCgpKTsKKyAgICBpZiAoYi5sZW5ndGgo KSkKKyAgICAgICAgbWVtY3B5KHEgKyBhLmxlbmd0aCgpLCBiLmRhdGEoKSwgYi5sZW5ndGgoKSk7 CisgICAgcmV0dXJuIHJlc3VsdDsKK30KKwogQmxvYkJ1aWxkZXI6OkJsb2JCdWlsZGVyKCkKICAg ICA6IG1fc2l6ZSgwKQogewpAQCAtNjksNyArODMsMTIgQEAgYm9vbCBCbG9iQnVpbGRlcjo6YXBw ZW5kKGNvbnN0IFN0cmluZyYgdGV4dCwgY29uc3QgU3RyaW5nJiBlbmRpbmdUeXBlLCBFeGNlcHRp b24KICAgICAgICAgcmV0dXJuIGZhbHNlOwogCiAgICAgbV9zaXplICs9IGNzdHIubGVuZ3RoKCk7 Ci0gICAgbV9pdGVtcy5hcHBlbmQoQmxvYkRhdGFJdGVtKGNzdHIpKTsKKworICAgIC8vIElmIHRo ZSBsYXN0IGl0ZW0gaXMgYSBzdHJpbmcsIGNvbmNhdGVuYXRlIGl0IHdpdGggY3VycmVudCBzdHJp bmcuCisgICAgaWYgKCFtX2l0ZW1zLmlzRW1wdHkoKSAmJiBtX2l0ZW1zW21faXRlbXMuc2l6ZSgp IC0gMV0udHlwZSA9PSBCbG9iRGF0YUl0ZW06OkRhdGEpCisgICAgICAgIG1faXRlbXNbbV9pdGVt cy5zaXplKCkgLSAxXS5kYXRhID0gYWRkVHdvQ1N0cmluZ3MobV9pdGVtc1ttX2l0ZW1zLnNpemUo KSAtIDFdLmRhdGEsIGNzdHIpOworICAgIGVsc2UKKyAgICAgICAgbV9pdGVtcy5hcHBlbmQoQmxv YkRhdGFJdGVtKGNzdHIpKTsKICAgICByZXR1cm4gdHJ1ZTsKIH0KIAo=