31839 2009-11-24 10:27:35 -0800 JSON.stringify performance on undefined is very poor 2009-11-30 13:52:38 -0800 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 joepeck webkit-unassigned aroben darin eric ggaren joepeck oliver oldest_to_newest 166152 0 43784 joepeck 2009-11-24 10:27:35 -0800 Created attachment 43784 [TEST CASE] Shows Poor Performance of Stringify with undefined I recently ran some benchmarks for native JSON in a few browsers and stumbled on the fact that WebKit's performance with the "undefined" value was considerably slow. A version of my test case is attached which highlights the issues. Here is a screenshot showing the improvement I got: http://grab.by/MjK Safari 4.0.4 ~120ms WebKit Nightly ~249ms My Build ~10ms My change still passes all tests. No tests are added, since this is really a benchmark. 166156 1 43785 joepeck 2009-11-24 10:32:18 -0800 Created attachment 43785 [PATCH] Replace substr() with truncate() in the rollback case I'm new to this area of WebKit, so please review carefully! Also, should I add an assert that the new truncate length is less then the existing length? 166163 2 eric 2009-11-24 10:39:24 -0800 Does Dromero test JSON parsing yet? Would be nice if it did. I suspect this would work fine, I don't know if this breaks a UString design principle though. I'm not sure if there are other cases where UString::Rep has more data that its len member claims it does. 166230 3 43785 oliver 2009-11-24 12:10:52 -0800 Comment on attachment 43785 [PATCH] Replace substr() with truncate() in the rollback case > + void truncate(int length) const { m_rep->len = length; } I am concerned about this change -- i'm not convinced that it's safe (as rep's may be shared). What is the object structure that leads to us need to roll back the string? I'm also wondering if maybe it's worth just switching to Vector<UChar> while we build the string, which will remove any data sharing issues we could hit. 166238 4 joepeck 2009-11-24 12:19:44 -0800 (In reply to comment #3) > What is the object structure that leads to us need to roll > back the string? The JSON result is continually built by appending onto a string builder (UString). JSON specifies that we should not put undefined values in the resulting sting. The case where it is rolled back is when contents were already put on the builder string, and then we encountered an "undefined" value, and we have to remove the contents we already put on the string. So in the following case: { a: undefined } The builder would follow these steps: Action Builder So Far ----------- -------------- 1. start object { 2. put key and seperator {"a": 3. encounter undefined value... rollback { 4. finish properties, close object {} 5. done 166244 5 oliver 2009-11-24 12:28:51 -0800 (In reply to comment #4) > (In reply to comment #3) > > What is the object structure that leads to us need to roll > > back the string? > > The JSON result is continually built by appending onto a string builder > (UString). JSON specifies that we should not put undefined values in the > resulting sting. > > The case where it is rolled back is when contents were already put on the > builder string, and then we encountered an "undefined" value, and we have to > remove the contents we already put on the string. So in the following case: > > { a: undefined } > > The builder would follow these steps: > > Action Builder So Far > ----------- -------------- > 1. start object { > 2. put key and seperator {"a": > 3. encounter undefined value... rollback { > 4. finish properties, close object {} > 5. done Yeah i saw it once i actually looked at the code. I'm just testing a patch that switches stringification over to basically using a Vector<UChar>. I'm testing perf, etc now. 166252 6 43802 oliver 2009-11-24 13:15:15 -0800 Created attachment 43802 Patch 166261 7 43802 ap 2009-11-24 13:30:58 -0800 Comment on attachment 43802 Patch > + class StringBuilder : public Vector<UChar> { Subclassing Vector is unsafe, because it does not have a virtual destructor. Maybe adding private operator delete would make it a little safer? I'm not 100% clear on what the best way to make it safe is. > + for (size_t i = 0; i < len; i++) > + Vector<UChar>::append(str[i]); Should we ASSERT that the only low ASCII is used here? Otherwise, conversion from signed to unsigned could go badly (not to mention that we don't know what encoding it is). > + inline void append(const char ch) Someone could ask you why you don't use full words instead of those "ch", "len" and "str". > + Vector<UChar>::append(ch); Would a simple "using Vector<UChar>::append" achieve the same result? r=me 166265 8 oliver 2009-11-24 13:42:20 -0800 Committed r51352 167415 9 darin 2009-11-30 13:50:00 -0800 (In reply to comment #7) > (From update of attachment 43802 [details]) > > + class StringBuilder : public Vector<UChar> { > > Subclassing Vector is unsafe, because it does not have a virtual destructor. > Maybe adding private operator delete would make it a little safer? I'm not 100% > clear on what the best way to make it safe is. Private inheritance might work better. Then you can expose Vector functions with "using" individually. > Should we ASSERT that the only low ASCII is used here? Otherwise, conversion > from signed to unsigned could go badly (not to mention that we don't know what > encoding it is). Yes. > > + inline void append(const char ch) > > Someone could ask you why you don't use full words instead of those "ch", "len" > and "str". Like me. > Would a simple "using Vector<UChar>::append" achieve the same result? Yes. 167416 10 darin 2009-11-30 13:52:38 -0800 (In reply to comment #9) > > Subclassing Vector is unsafe, because it does not have a virtual destructor. > > Maybe adding private operator delete would make it a little safer? I'm not 100% > > clear on what the best way to make it safe is. Since StringBuilder doesn't add any data members or override any virtual functions, I think it's not unsafe. 43784 2009-11-24 10:27:35 -0800 2009-11-24 10:27:35 -0800 [TEST CASE] Shows Poor Performance of Stringify with undefined json-time-test.html text/html 1049 joepeck PCFET0NUWVBFIGh0bWw+CjxodG1sPgo8aGVhZD4KCTx0aXRsZT5UaW1pbmcgSlNPTjwvdGl0bGU+ CiAgPHNjcmlwdCB0eXBlPSJ0ZXh0L2phdmFzY3JpcHQiPgogICAgd2luZG93Lm9ubG9hZCA9IGZ1 bmN0aW9uKCkgewoKICAgICAgLy8gSlNPTiBQYXJ0CiAgICAgIHZhciBwYXJ0ID0gewogICAgICAg ICJ4IjogMTIzZTQsCiAgICAgICAgInkiOiAtMTIwLjEyLAogICAgICAgICJ6IjogOCwKICAgICAg ICAiXyI6IHVuZGVmaW5lZAogICAgICB9OwoKICAgICAgLy8gQnVpbGQgYSBiaWdnZXIgb2JqZWN0 CiAgICAgIHZhciBvYmpzID0gW107CiAgICAgIHZhciBUT1RBTCA9IDUwMDA7CiAgICAgIGZvciAo dmFyIGk9MDsgaTxUT1RBTDsgKytpKQogICAgICAgICAgb2Jqcy5wdXNoKHBhcnQpOwoKICAgICAg dmFyIHN0YXJ0LCBzdHIsIGVuZDsKCiAgICAgIC8vIFRpbWUgc3RyaW5naWZpY2F0aW9uCiAgICAg IHN0YXJ0ID0gbmV3IERhdGU7CiAgICAgIHN0ciA9IEpTT04uc3RyaW5naWZ5KG9ianMpOwogICAg ICBlbmQgPSBuZXcgRGF0ZTsKICAgICAgdmFyIHN0cmluZ2lmeVRpbWUgPSBlbmQtc3RhcnQ7Cgog ICAgICAvLyBUaW1lIHBhcnNpbmcKICAgICAgc3RhcnQgPSBuZXcgRGF0ZTsKICAgICAgc3RyID0g SlNPTi5wYXJzZShzdHIpOwogICAgICBlbmQgPSBuZXcgRGF0ZTsKICAgICAgdmFyIHBhcnNlVGlt ZSA9IGVuZC1zdGFydDsKICAgICAgCiAgICAgIC8vIFNob3cgUmVzdWx0cwogICAgICBkb2N1bWVu dC5nZXRFbGVtZW50QnlJZCgnc3RyaW5naWZ5JykuaW5uZXJIVE1MID0gc3RyaW5naWZ5VGltZTsK ICAgICAgZG9jdW1lbnQuZ2V0RWxlbWVudEJ5SWQoJ3BhcnNlJykuaW5uZXJIVE1MID0gcGFyc2VU aW1lOwogCiAgICB9OwogIDwvc2NyaXB0Pgo8L2hlYWQ+Cjxib2R5PgogIDxoMT5UaW1pbmcgSlNP TiBTdHJpbmdpZnkgYW5kIFBhcnNlPC9oMT4KICA8aDI+U3RyaW5naWZ5OiA8c3BhbiBpZD0ic3Ry aW5naWZ5Ij48L3NwYW4+PC9oMj4KICA8aDI+UGFyc2U6IDxzcGFuIGlkPSJwYXJzZSI+PC9zcGFu PjwvaDI+CjwvYm9keT4KPC9odG1sPgo= 43785 2009-11-24 10:32:18 -0800 2009-11-24 13:15:11 -0800 [PATCH] Replace substr() with truncate() in the rollback case json-stringify.patch text/plain 1855 joepeck ZGlmZiAtLWdpdCBhL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZyBiL0phdmFTY3JpcHRDb3JlL0No YW5nZUxvZwppbmRleCBjN2Q1MjBiLi5iNzYwOWRlIDEwMDY0NAotLS0gYS9KYXZhU2NyaXB0Q29y ZS9DaGFuZ2VMb2cKKysrIGIvSmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTUg QEAKKzIwMDktMTEtMjQgIEpvc2VwaCBQZWNvcmFybyAgPGpvZXBlY2tAd2Via2l0Lm9yZz4KKwor ICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBKU09OLnN0cmlu Z2lmeSBwZXJmb3JtYW5jZSBvbiB1bmRlZmluZWQgaXMgdmVyeSBwb29yCisgICAgICAgIGh0dHBz Oi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0zMTgzOQorCisgICAgICAgICogcnVu dGltZS9KU09OT2JqZWN0LmNwcDoKKyAgICAgICAgKEpTQzo6U3RyaW5naWZpZXI6OkhvbGRlcjo6 YXBwZW5kTmV4dFByb3BlcnR5KTogdXNlIHRydW5jYXRlIGluc3RlYWQgb2Ygc3Vic3RyIGluIHJv bGxiYWNrIGNhc2UKKyAgICAgICAgKiBydW50aW1lL1VTdHJpbmcuaDoKKyAgICAgICAgKEpTQzo6 VVN0cmluZzo6dHJ1bmNhdGUpOiBzcGVjaWZ5IHRoZSBsZW5ndGggb2YgdGhlIFJlcAorCiAyMDA5 LTExLTI0ICBNYXJrIFJvd2UgIDxtcm93ZUBhcHBsZS5jb20+CiAKICAgICAgICAgRml4IHByb2R1 Y3Rpb24gYnVpbGRzIHdoZXJlIHRoZSBzb3VyY2UgdHJlZSBtYXkgYmUgcmVhZC1vbmx5LgpkaWZm IC0tZ2l0IGEvSmF2YVNjcmlwdENvcmUvcnVudGltZS9KU09OT2JqZWN0LmNwcCBiL0phdmFTY3Jp cHRDb3JlL3J1bnRpbWUvSlNPTk9iamVjdC5jcHAKaW5kZXggMjk3ZDQ1Ny4uMTM1MDhiMyAxMDA2 NDQKLS0tIGEvSmF2YVNjcmlwdENvcmUvcnVudGltZS9KU09OT2JqZWN0LmNwcAorKysgYi9KYXZh U2NyaXB0Q29yZS9ydW50aW1lL0pTT05PYmplY3QuY3BwCkBAIC01ODYsNyArNTg2LDcgQEAgYm9v bCBTdHJpbmdpZmllcjo6SG9sZGVyOjphcHBlbmROZXh0UHJvcGVydHkoU3RyaW5naWZpZXImIHN0 cmluZ2lmaWVyLCBTdHJpbmdCdWkKICAgICAgICAgICAgIC8vIFRoaXMgb25seSBvY2N1cnMgd2hl biBnZXQgYW4gdW5kZWZpbmVkIHZhbHVlIGZvciBhbiBvYmplY3QgcHJvcGVydHkuCiAgICAgICAg ICAgICAvLyBJbiB0aGlzIGNhc2Ugd2UgZG9uJ3Qgd2FudCB0aGUgc2VwYXJhdG9yIGFuZCBwcm9w ZXJ0eSBuYW1lIHRoYXQgd2UKICAgICAgICAgICAgIC8vIGFscmVhZHkgYXBwZW5kZWQsIHNvIHJv bGwgYmFjay4KLSAgICAgICAgICAgIGJ1aWxkZXIgPSBidWlsZGVyLnN1YnN0cigwLCByb2xsQmFj a1BvaW50KTsKKyAgICAgICAgICAgIGJ1aWxkZXIudHJ1bmNhdGUocm9sbEJhY2tQb2ludCk7CiAg ICAgICAgICAgICBicmVhazsKICAgICB9CiAKZGlmZiAtLWdpdCBhL0phdmFTY3JpcHRDb3JlL3J1 bnRpbWUvVVN0cmluZy5oIGIvSmF2YVNjcmlwdENvcmUvcnVudGltZS9VU3RyaW5nLmgKaW5kZXgg OWIwNDZmMi4uNWQzN2Y2YSAxMDA2NDQKLS0tIGEvSmF2YVNjcmlwdENvcmUvcnVudGltZS9VU3Ry aW5nLmgKKysrIGIvSmF2YVNjcmlwdENvcmUvcnVudGltZS9VU3RyaW5nLmgKQEAgLTMzOSw2ICsz MzksNyBAQCBuYW1lc3BhY2UgSlNDIHsKICAgICAgICAgaW50IHJmaW5kKFVDaGFyLCBpbnQgcG9z KSBjb25zdDsKIAogICAgICAgICBVU3RyaW5nIHN1YnN0cihpbnQgcG9zID0gMCwgaW50IGxlbiA9 IC0xKSBjb25zdDsKKyAgICAgICAgdm9pZCB0cnVuY2F0ZShpbnQgbGVuZ3RoKSBjb25zdCB7IG1f cmVwLT5sZW4gPSBsZW5ndGg7IH0KIAogICAgICAgICBzdGF0aWMgY29uc3QgVVN0cmluZyYgbnVs bCgpIHsgcmV0dXJuICpudWxsVVN0cmluZzsgfQogCg== 43802 2009-11-24 13:15:15 -0800 2009-11-24 13:30:57 -0800 Patch bug-31839-20091124131514.patch text/plain 2804 oliver ZGlmZiAtLWdpdCBhL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZyBiL0phdmFTY3JpcHRDb3JlL0No YW5nZUxvZwppbmRleCBkZmZkNjU3Li5kYzI1YTM2IDEwMDY0NAotLS0gYS9KYXZhU2NyaXB0Q29y ZS9DaGFuZ2VMb2cKKysrIGIvSmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjEg QEAKKzIwMDktMTEtMjQgIE9saXZlciBIdW50ICA8b2xpdmVyQGFwcGxlLmNvbT4KKworICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBKU09OLnN0cmluZ2lmeSBw ZXJmb3JtYW5jZSBvbiB1bmRlZmluZWQgaXMgdmVyeSBwb29yCisgICAgICAgIGh0dHBzOi8vYnVn cy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0zMTgzOQorCisgICAgICAgIFN3aXRjaCBmcm9t IGEgVVN0cmluZyB0byBhIFZlY3RvcjxVQ2hhcj4gd2hlbiBidWlsZGluZworICAgICAgICB0aGUg SlNPTiBzdHJpbmcsIGFsbG93aW5nIHVzIHRvIHNhZmVseSByZW1vdmUgdGhlIHN1YnN0ci1jb3B5 CisgICAgICAgIHdlIG90aGVyd2lzZSBkaWQgd2hlbiB1bndpbmRpbmcgYW4gdW5kZWZpbmVkIHBy b3BlcnR5LgorCisgICAgICAgIEFsc28gdHVybnMgb3V0IHRvIGJlIGEgfjUlIHNwZWVkdXAgb24g c3RyaW5naWZpY2F0aW9uLgorCisgICAgICAgICogcnVudGltZS9KU09OT2JqZWN0LmNwcDoKKyAg ICAgICAgKEpTQzo6U3RyaW5naWZpZXI6OlN0cmluZ0J1aWxkZXI6OmFwcGVuZCk6CisgICAgICAg IChKU0M6OlN0cmluZ2lmaWVyOjpzdHJpbmdpZnkpOgorICAgICAgICAoSlNDOjpTdHJpbmdpZmll cjo6SG9sZGVyOjphcHBlbmROZXh0UHJvcGVydHkpOgorCiAyMDA5LTExLTIzICBMYXN6bG8gR29t Ym9zICA8bGFzemxvLjEuZ29tYm9zQG5va2lhLmNvbT4KIAogICAgICAgICBSZXZpZXdlZCBieSBL ZW5uZXRoIFJvaGRlIENocmlzdGlhbnNlbi4KZGlmZiAtLWdpdCBhL0phdmFTY3JpcHRDb3JlL3J1 bnRpbWUvSlNPTk9iamVjdC5jcHAgYi9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0pTT05PYmplY3Qu Y3BwCmluZGV4IDI5N2Q0NTcuLjY5YjBjZGMgMTAwNjQ0Ci0tLSBhL0phdmFTY3JpcHRDb3JlL3J1 bnRpbWUvSlNPTk9iamVjdC5jcHAKKysrIGIvSmF2YVNjcmlwdENvcmUvcnVudGltZS9KU09OT2Jq ZWN0LmNwcApAQCAtNzAsNyArNzAsMzEgQEAgcHVibGljOgogICAgIHZvaWQgbWFya0FnZ3JlZ2F0 ZShNYXJrU3RhY2smKTsKIAogcHJpdmF0ZToKLSAgICB0eXBlZGVmIFVTdHJpbmcgU3RyaW5nQnVp bGRlcjsKKyAgICBjbGFzcyBTdHJpbmdCdWlsZGVyIDogcHVibGljIFZlY3RvcjxVQ2hhcj4gewor ICAgIHB1YmxpYzoKKyAgICAgICAgaW5saW5lIHZvaWQgYXBwZW5kKGNvbnN0IGNoYXIqIHN0cikK KyAgICAgICAgeworICAgICAgICAgICAgc2l6ZV90IGxlbiA9IHN0cmxlbihzdHIpOworICAgICAg ICAgICAgcmVzZXJ2ZUNhcGFjaXR5KHNpemUoKSArIGxlbik7CisgICAgICAgICAgICBmb3IgKHNp emVfdCBpID0gMDsgaSA8IGxlbjsgaSsrKQorICAgICAgICAgICAgICAgIFZlY3RvcjxVQ2hhcj46 OmFwcGVuZChzdHJbaV0pOworICAgICAgICB9CisKKyAgICAgICAgaW5saW5lIHZvaWQgYXBwZW5k KGNvbnN0IGNoYXIgY2gpCisgICAgICAgIHsKKyAgICAgICAgICAgIFZlY3RvcjxVQ2hhcj46OmFw cGVuZChjaCk7CisgICAgICAgIH0KKworICAgICAgICBpbmxpbmUgdm9pZCBhcHBlbmQoY29uc3Qg VUNoYXIqIHN0ciwgc2l6ZV90IGxlbikKKyAgICAgICAgeworICAgICAgICAgICAgVmVjdG9yPFVD aGFyPjo6YXBwZW5kKHN0ciwgbGVuKTsKKyAgICAgICAgfQorCisgICAgICAgIGlubGluZSB2b2lk IGFwcGVuZChjb25zdCBVU3RyaW5nJiBzdHIpCisgICAgICAgIHsKKyAgICAgICAgICAgIGFwcGVu ZChzdHIuZGF0YSgpLCBzdHIuc2l6ZSgpKTsKKyAgICAgICAgfQorICAgIH07CiAKICAgICBjbGFz cyBIb2xkZXIgewogICAgIHB1YmxpYzoKQEAgLTI2OSw3ICsyOTMsOSBAQCBKU1ZhbHVlIFN0cmlu Z2lmaWVyOjpzdHJpbmdpZnkoSlNWYWx1ZSB2YWx1ZSkKICAgICBpZiAobV9leGVjLT5oYWRFeGNl cHRpb24oKSkKICAgICAgICAgcmV0dXJuIGpzTnVsbCgpOwogCi0gICAgcmV0dXJuIGpzU3RyaW5n KG1fZXhlYywgcmVzdWx0KTsKKyAgICByZXN1bHQuc2hyaW5rVG9GaXQoKTsKKyAgICBzaXplX3Qg bGVuZ3RoID0gcmVzdWx0LnNpemUoKTsKKyAgICByZXR1cm4ganNTdHJpbmcobV9leGVjLCBVU3Ry aW5nKHJlc3VsdC5yZWxlYXNlQnVmZmVyKCksIGxlbmd0aCwgZmFsc2UpKTsKIH0KIAogdm9pZCBT dHJpbmdpZmllcjo6YXBwZW5kUXVvdGVkU3RyaW5nKFN0cmluZ0J1aWxkZXImIGJ1aWxkZXIsIGNv bnN0IFVTdHJpbmcmIHZhbHVlKQpAQCAtNTg2LDcgKzYxMiw3IEBAIGJvb2wgU3RyaW5naWZpZXI6 OkhvbGRlcjo6YXBwZW5kTmV4dFByb3BlcnR5KFN0cmluZ2lmaWVyJiBzdHJpbmdpZmllciwgU3Ry aW5nQnVpCiAgICAgICAgICAgICAvLyBUaGlzIG9ubHkgb2NjdXJzIHdoZW4gZ2V0IGFuIHVuZGVm aW5lZCB2YWx1ZSBmb3IgYW4gb2JqZWN0IHByb3BlcnR5LgogICAgICAgICAgICAgLy8gSW4gdGhp cyBjYXNlIHdlIGRvbid0IHdhbnQgdGhlIHNlcGFyYXRvciBhbmQgcHJvcGVydHkgbmFtZSB0aGF0 IHdlCiAgICAgICAgICAgICAvLyBhbHJlYWR5IGFwcGVuZGVkLCBzbyByb2xsIGJhY2suCi0gICAg ICAgICAgICBidWlsZGVyID0gYnVpbGRlci5zdWJzdHIoMCwgcm9sbEJhY2tQb2ludCk7CisgICAg ICAgICAgICBidWlsZGVyLnJlc2l6ZShyb2xsQmFja1BvaW50KTsKICAgICAgICAgICAgIGJyZWFr OwogICAgIH0KIAo=