208725 2020-03-06 11:33:28 -0800 JSON.stringify should call replacer on deleted properties 2020-03-07 10:18:40 -0800 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build All All RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=205769 https://bugs.webkit.org/show_bug.cgi?id=208766 InRadar P2 Trivial --- 1 ashvayka ashvayka commit-queue ews-watchlist keith_miller mark.lam msaboff ross.kirsling saam tzagallo webkit-bug-importer oldest_to_newest 1626767 0 ashvayka 2020-03-06 11:33:28 -0800 ECMA262: https://tc39.es/ecma262/#sec-serializejsonobject (steps 6, 8.a) Test262: https://test262.report/browse/built-ins/JSON/stringify/replacer-function-object-deleted-property.js 1626848 1 392766 ashvayka 2020-03-06 13:41:57 -0800 Created attachment 392766 Patch 1626855 2 ashvayka 2020-03-06 13:52:21 -0800 `toJSONImpl` inlining, cold runs, --outer 4: TipOfTree patch json-stringify-many-objects 554.1745+-7.1931 555.3961+-5.6955 json-stringify-many-objects-to-json 498.5819+-6.1926 495.1950+-6.4387 <geometric> 525.6438+-6.6741 524.4324+-6.0557 1626932 3 392766 ross.kirsling 2020-03-06 15:52:28 -0800 Comment on attachment 392766 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392766&action=review This seems reasonable, particularly given the discussion about `get` in the previous patch. I'm not used to practices surrounding microbenchmarks, but they seem reasonable to me based on existing ones. > Source/JavaScriptCore/ChangeLog:15 > + [1]: https://tc39.es/ecma262/#sec-serializejsonobject (steps 6, 8.a) Is this sufficient/correct as a spec link? It seems like all three cases are removing early outs, but this just covers the object case (though I guess that's the one case with the visible conformance failure). 1626954 4 ashvayka 2020-03-06 16:21:44 -0800 (In reply to Ross Kirsling from comment #3) > I'm not used to practices surrounding microbenchmarks, but they seem reasonable to me based on existing ones. Given the possibility that `toJSONImpl` was extracted for performance reasons, these microbenchmarks are designed to spot any regression in "toJSON" lookup. > Is this sufficient/correct as a spec link? It seems like all three cases are > removing early outs, but this just covers the object case (though I guess > that's the one case with the visible conformance failure). Right, object case is the only one that was not spec-conformant because of ``` if (!hasProperty) return true; ``` The other two were 100% equivalent to `get`. 1627097 5 392766 commit-queue 2020-03-06 19:02:48 -0800 Comment on attachment 392766 Patch Clearing flags on attachment: 392766 Committed r258049: <https://trac.webkit.org/changeset/258049> 1627098 6 commit-queue 2020-03-06 19:02:49 -0800 All reviewed patches have been landed. Closing bug. 1627100 7 webkit-bug-importer 2020-03-06 19:03:14 -0800 <rdar://problem/60179986> 1627236 8 ryanhaddad 2020-03-07 10:18:40 -0800 This change cappears to have caused assertion failures on the Debug JSC bot: https://bugs.webkit.org/show_bug.cgi?id=208766 Can this be addressed quickly? 392766 2020-03-06 13:41:57 -0800 2020-03-06 19:02:48 -0800 Patch bug-208725-20200306234156.patch text/plain 7013 ashvayka SW5kZXg6IEpTVGVzdHMvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIEpTVGVzdHMvQ2hhbmdlTG9n CShyZXZpc2lvbiAyNTgwMjcpCisrKyBKU1Rlc3RzL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpA QCAtMSwzICsxLDE0IEBACisyMDIwLTAzLTA2ICBBbGV4ZXkgU2h2YXlrYSAgPHNodmFpa2FsZXNo QGdtYWlsLmNvbT4KKworICAgICAgICBKU09OLnN0cmluZ2lmeSBzaG91bGQgY2FsbCByZXBsYWNl ciBvbiBkZWxldGVkIHByb3BlcnRpZXMKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcv c2hvd19idWcuY2dpP2lkPTIwODcyNQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgICogbWljcm9iZW5jaG1hcmtzL2pzb24tc3RyaW5naWZ5LW1hbnktb2Jq ZWN0cy10by1qc29uLmpzOiBBZGRlZC4KKyAgICAgICAgKiBtaWNyb2JlbmNobWFya3MvanNvbi1z dHJpbmdpZnktbWFueS1vYmplY3RzLmpzOiBBZGRlZC4KKyAgICAgICAgKiB0ZXN0MjYyL2V4cGVj dGF0aW9ucy55YW1sOiBNYXJrIDIgdGVzdCBjYXNlcyBhcyBwYXNzaW5nLgorCiAyMDIwLTAzLTA0 ICBNYXJrIExhbSAgPG1hcmsubGFtQGFwcGxlLmNvbT4KIAogICAgICAgICBIYW5kbGUgYW4gb3V0 IG9mIG1lbW9yeSBlcnJvciB3aGlsZSBjb25zdHJ1Y3RpbmcgdGhlIEJ5dGVjb2RlR2VuZXJhdG9y LgpJbmRleDogSlNUZXN0cy9taWNyb2JlbmNobWFya3MvanNvbi1zdHJpbmdpZnktbWFueS1vYmpl Y3RzLXRvLWpzb24uanMKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gSlNUZXN0cy9taWNyb2JlbmNobWFya3MvanNv bi1zdHJpbmdpZnktbWFueS1vYmplY3RzLXRvLWpzb24uanMJKG5vbmV4aXN0ZW50KQorKysgSlNU ZXN0cy9taWNyb2JlbmNobWFya3MvanNvbi1zdHJpbmdpZnktbWFueS1vYmplY3RzLXRvLWpzb24u anMJKHdvcmtpbmcgY29weSkKQEAgLTAsMCArMSw2IEBACitjb25zdCB0b0pTT04gPSAoKSA9PiAn JzsKK2NvbnN0IHZhbHVlID0ge307Citmb3IgKGxldCBpID0gMDsgaSA8IDEwMDsgKytpKQorICAg IHZhbHVlW2ldID0ge3RvSlNPTn07Citmb3IgKGxldCBpID0gMDsgaSA8IDFlNSAvIDQ7ICsraSkK KyAgICBKU09OLnN0cmluZ2lmeSh2YWx1ZSk7CkluZGV4OiBKU1Rlc3RzL21pY3JvYmVuY2htYXJr cy9qc29uLXN0cmluZ2lmeS1tYW55LW9iamVjdHMuanMKPT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gSlNUZXN0cy9t aWNyb2JlbmNobWFya3MvanNvbi1zdHJpbmdpZnktbWFueS1vYmplY3RzLmpzCShub25leGlzdGVu dCkKKysrIEpTVGVzdHMvbWljcm9iZW5jaG1hcmtzL2pzb24tc3RyaW5naWZ5LW1hbnktb2JqZWN0 cy5qcwkod29ya2luZyBjb3B5KQpAQCAtMCwwICsxLDUgQEAKK2NvbnN0IHZhbHVlID0ge307Citm b3IgKGxldCBpID0gMDsgaSA8IDEwMDsgKytpKQorICAgIHZhbHVlW2ldID0ge307Citmb3IgKGxl dCBpID0gMDsgaSA8IDFlNSAvIDQ7ICsraSkKKyAgICBKU09OLnN0cmluZ2lmeSh2YWx1ZSk7Cklu ZGV4OiBKU1Rlc3RzL3Rlc3QyNjIvZXhwZWN0YXRpb25zLnlhbWwKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gSlNU ZXN0cy90ZXN0MjYyL2V4cGVjdGF0aW9ucy55YW1sCShyZXZpc2lvbiAyNTc5MTMpCisrKyBKU1Rl c3RzL3Rlc3QyNjIvZXhwZWN0YXRpb25zLnlhbWwJKHdvcmtpbmcgY29weSkKQEAgLTEwOTUsOSAr MTA5NSw2IEBAIHRlc3QvYnVpbHQtaW5zL0pTT04vcGFyc2UvcmV2aXZlci1hcnJheS0KIHRlc3Qv YnVpbHQtaW5zL0pTT04vcGFyc2UvcmV2aXZlci1vYmplY3Qtbm9uLWNvbmZpZ3VyYWJsZS1wcm9w LWNyZWF0ZS5qczoKICAgZGVmYXVsdDogJ1Rlc3QyNjJFcnJvcjogRXhwZWN0ZWQgU2FtZVZhbHVl KMKrMjLCuywgwqsywrspIHRvIGJlIHRydWUnCiAgIHN0cmljdCBtb2RlOiAnVGVzdDI2MkVycm9y OiBFeHBlY3RlZCBTYW1lVmFsdWUowqsyMsK7LCDCqzLCuykgdG8gYmUgdHJ1ZScKLXRlc3QvYnVp bHQtaW5zL0pTT04vc3RyaW5naWZ5L3JlcGxhY2VyLWZ1bmN0aW9uLW9iamVjdC1kZWxldGVkLXBy b3BlcnR5LmpzOgotICBkZWZhdWx0OiAnVGVzdDI2MkVycm9yOiBFeHBlY3RlZCBTYW1lVmFsdWUo wqt7ImEiOjF9wrssIMKreyJhIjoxLCJiIjoiPHJlcGxhY2VkPiJ9wrspIHRvIGJlIHRydWUnCi0g IHN0cmljdCBtb2RlOiAnVGVzdDI2MkVycm9yOiBFeHBlY3RlZCBTYW1lVmFsdWUowqt7ImEiOjF9 wrssIMKreyJhIjoxLCJiIjoiPHJlcGxhY2VkPiJ9wrspIHRvIGJlIHRydWUnCiB0ZXN0L2J1aWx0 LWlucy9NYXAvcHJvdG8tZnJvbS1jdG9yLXJlYWxtLmpzOgogICBkZWZhdWx0OiAnVGVzdDI2MkVy cm9yOiBFeHBlY3RlZCBTYW1lVmFsdWUowqtbb2JqZWN0IE1hcF3Cuywgwqtbb2JqZWN0IE1hcF3C uykgdG8gYmUgdHJ1ZScKICAgc3RyaWN0IG1vZGU6ICdUZXN0MjYyRXJyb3I6IEV4cGVjdGVkIFNh bWVWYWx1ZSjCq1tvYmplY3QgTWFwXcK7LCDCq1tvYmplY3QgTWFwXcK7KSB0byBiZSB0cnVlJwpJ bmRleDogU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3Vy Y2UvSmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCShyZXZpc2lvbiAyNTc5MTMpCisrKyBTb3VyY2Uv SmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMjQgQEAK KzIwMjAtMDMtMDYgIEFsZXhleSBTaHZheWthICA8c2h2YWlrYWxlc2hAZ21haWwuY29tPgorCisg ICAgICAgIEpTT04uc3RyaW5naWZ5IHNob3VsZCBjYWxsIHJlcGxhY2VyIG9uIGRlbGV0ZWQgcHJv cGVydGllcworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9 MjA4NzI1CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAg VGhpcyBjaGFuZ2UgcmVtb3ZlcyBleHRyYSBgaGFzUHJvcGVydHlgIGNoZWNrIGZyb20gYGFwcGVu ZE5leHRQcm9wZXJ0eWAgYXMKKyAgICAgICAgaXQgZG9lcyBub3QgZXhpc3QgaW4gdGhlIHNwZWMg WzFdLCBhbGlnbmluZyBKU0Mgd2l0aCBWOCBhbmQgU3BpZGVyTW9ua2V5LgorCisgICAgICAgIFRo aXMgcGF0Y2ggYWxzbyByZXBsYWNlcyAzIHVzYWdlcyBvZiBgZ2V0UHJvcGVydHlTbG90YCB3aXRo IHNlbWFudGljYWxseQorICAgICAgICBlcXVpdmFsZW50ICh5ZXQgbW9yZSBjb25jaXNlKSBgZ2V0 YCBhbmQgaW5saW5lcyBgdG9KU09OSW1wbGAgKHRoaXMgY2hhbmdlCisgICAgICAgIGlzIHBlcmZv cm1hbmNlLW5ldXRyYWwpLgorCisgICAgICAgIFsxXTogaHR0cHM6Ly90YzM5LmVzL2VjbWEyNjIv I3NlYy1zZXJpYWxpemVqc29ub2JqZWN0IChzdGVwcyA2LCA4LmEpCisKKyAgICAgICAgKiBydW50 aW1lL0pTT05PYmplY3QuY3BwOgorICAgICAgICAoSlNDOjpTdHJpbmdpZmllcjo6dG9KU09OKToK KyAgICAgICAgKEpTQzo6U3RyaW5naWZpZXI6OkhvbGRlcjo6YXBwZW5kTmV4dFByb3BlcnR5KToK KyAgICAgICAgKEpTQzo6U3RyaW5naWZpZXI6OnRvSlNPTkltcGwpOiBEZWxldGVkLgorCiAyMDIw LTAzLTA1ICBQYXVsbyBNYXRvcyAgPHBtYXRvc0BpZ2FsaWEuY29tPgogCiAgICAgICAgIFtKU0NP bmx5XSAzMi1iaXRzIHdhcm5pbmcgb24gbWVtc2V0IG9mIEpTVmFsdWUKSW5kZXg6IFNvdXJjZS9K YXZhU2NyaXB0Q29yZS9ydW50aW1lL0pTT05PYmplY3QuY3BwCj09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJj ZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0pTT05PYmplY3QuY3BwCShyZXZpc2lvbiAyNTc5MTMp CisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9KU09OT2JqZWN0LmNwcAkod29ya2lu ZyBjb3B5KQpAQCAtMTEzLDcgKzExMyw2IEBAIHByaXZhdGU6CiAgICAgZnJpZW5kIGNsYXNzIEhv bGRlcjsKIAogICAgIEpTVmFsdWUgdG9KU09OKEpTVmFsdWUsIGNvbnN0IFByb3BlcnR5TmFtZUZv ckZ1bmN0aW9uQ2FsbCYpOwotICAgIEpTVmFsdWUgdG9KU09OSW1wbChWTSYsIEpTVmFsdWUsIEpT VmFsdWUgdG9KU09ORnVuY3Rpb24sIGNvbnN0IFByb3BlcnR5TmFtZUZvckZ1bmN0aW9uQ2FsbCYp OwogCiAgICAgZW51bSBTdHJpbmdpZnlSZXN1bHQgeyBTdHJpbmdpZnlGYWlsZWQsIFN0cmluZ2lm eVN1Y2NlZWRlZCwgU3RyaW5naWZ5RmFpbGVkRHVlVG9VbmRlZmluZWRPclN5bWJvbFZhbHVlIH07 CiAgICAgU3RyaW5naWZ5UmVzdWx0IGFwcGVuZFN0cmluZ2lmaWVkVmFsdWUoU3RyaW5nQnVpbGRl ciYsIEpTVmFsdWUsIGNvbnN0IEhvbGRlciYsIGNvbnN0IFByb3BlcnR5TmFtZUZvckZ1bmN0aW9u Q2FsbCYpOwpAQCAtMzAyLDE5ICszMDEsOSBAQCBBTFdBWVNfSU5MSU5FIEpTVmFsdWUgU3RyaW5n aWZpZXI6OnRvSlNPCiAgICAgYXV0byBzY29wZSA9IERFQ0xBUkVfVEhST1dfU0NPUEUodm0pOwog ICAgIHNjb3BlLmFzc2VydE5vRXhjZXB0aW9uKCk7CiAKLSAgICBQcm9wZXJ0eVNsb3Qgc2xvdChi YXNlVmFsdWUsIFByb3BlcnR5U2xvdDo6SW50ZXJuYWxNZXRob2RUeXBlOjpHZXQpOwotICAgIGJv b2wgaGFzUHJvcGVydHkgPSBiYXNlVmFsdWUuZ2V0UHJvcGVydHlTbG90KG1fZ2xvYmFsT2JqZWN0 LCB2bS5wcm9wZXJ0eU5hbWVzLT50b0pTT04sIHNsb3QpOwotICAgIEVYQ0VQVElPTl9BU1NFUlQo IXNjb3BlLmV4Y2VwdGlvbigpIHx8ICFoYXNQcm9wZXJ0eSk7Ci0gICAgaWYgKCFoYXNQcm9wZXJ0 eSkKLSAgICAgICAgcmV0dXJuIGJhc2VWYWx1ZTsKLQotICAgIEpTVmFsdWUgdG9KU09ORnVuY3Rp b24gPSBzbG90LmdldFZhbHVlKG1fZ2xvYmFsT2JqZWN0LCB2bS5wcm9wZXJ0eU5hbWVzLT50b0pT T04pOworICAgIEpTVmFsdWUgdG9KU09ORnVuY3Rpb24gPSBiYXNlVmFsdWUuZ2V0KG1fZ2xvYmFs T2JqZWN0LCB2bS5wcm9wZXJ0eU5hbWVzLT50b0pTT04pOwogICAgIFJFVFVSTl9JRl9FWENFUFRJ T04oc2NvcGUsIHsgfSk7Ci0gICAgUkVMRUFTRV9BTkRfUkVUVVJOKHNjb3BlLCB0b0pTT05JbXBs KHZtLCBiYXNlVmFsdWUsIHRvSlNPTkZ1bmN0aW9uLCBwcm9wZXJ0eU5hbWUpKTsKLX0KIAotSlNW YWx1ZSBTdHJpbmdpZmllcjo6dG9KU09OSW1wbChWTSYgdm0sIEpTVmFsdWUgYmFzZVZhbHVlLCBK U1ZhbHVlIHRvSlNPTkZ1bmN0aW9uLCBjb25zdCBQcm9wZXJ0eU5hbWVGb3JGdW5jdGlvbkNhbGwm IHByb3BlcnR5TmFtZSkKLXsKICAgICBDYWxsVHlwZSBjYWxsVHlwZTsKICAgICBDYWxsRGF0YSBj YWxsRGF0YTsKICAgICBpZiAoIXRvSlNPTkZ1bmN0aW9uLmlzQ2FsbGFibGUodm0sIGNhbGxUeXBl LCBjYWxsRGF0YSkpCkBAIC01MjgsMTMgKzUxNyw3IEBAIGJvb2wgU3RyaW5naWZpZXI6OkhvbGRl cjo6YXBwZW5kTmV4dFByb3AKICAgICAgICAgaWYgKG1faXNKU0FycmF5ICYmIG1fb2JqZWN0LT5j YW5HZXRJbmRleFF1aWNrbHkoaW5kZXgpKQogICAgICAgICAgICAgdmFsdWUgPSBtX29iamVjdC0+ Z2V0SW5kZXhRdWlja2x5KGluZGV4KTsKICAgICAgICAgZWxzZSB7Ci0gICAgICAgICAgICBQcm9w ZXJ0eVNsb3Qgc2xvdChtX29iamVjdCwgUHJvcGVydHlTbG90OjpJbnRlcm5hbE1ldGhvZFR5cGU6 OkdldCk7Ci0gICAgICAgICAgICBib29sIGhhc1Byb3BlcnR5ID0gbV9vYmplY3QtPmdldFByb3Bl cnR5U2xvdChnbG9iYWxPYmplY3QsIGluZGV4LCBzbG90KTsKLSAgICAgICAgICAgIEVYQ0VQVElP Tl9BU1NFUlQoIXNjb3BlLmV4Y2VwdGlvbigpIHx8ICFoYXNQcm9wZXJ0eSk7Ci0gICAgICAgICAg ICBpZiAoaGFzUHJvcGVydHkpCi0gICAgICAgICAgICAgICAgdmFsdWUgPSBzbG90LmdldFZhbHVl KGdsb2JhbE9iamVjdCwgaW5kZXgpOwotICAgICAgICAgICAgZWxzZQotICAgICAgICAgICAgICAg IHZhbHVlID0ganNVbmRlZmluZWQoKTsKKyAgICAgICAgICAgIHZhbHVlID0gbV9vYmplY3QtPmdl dChnbG9iYWxPYmplY3QsIGluZGV4KTsKICAgICAgICAgICAgIFJFVFVSTl9JRl9FWENFUFRJT04o c2NvcGUsIGZhbHNlKTsKICAgICAgICAgfQogCkBAIC01NDgsMTMgKzUzMSw4IEBAIGJvb2wgU3Ry aW5naWZpZXI6OkhvbGRlcjo6YXBwZW5kTmV4dFByb3AKICAgICAgICAgQVNTRVJUKHN0cmluZ2lm eVJlc3VsdCAhPSBTdHJpbmdpZnlGYWlsZWREdWVUb1VuZGVmaW5lZE9yU3ltYm9sVmFsdWUpOwog ICAgIH0gZWxzZSB7CiAgICAgICAgIC8vIEdldCB0aGUgdmFsdWUuCi0gICAgICAgIFByb3BlcnR5 U2xvdCBzbG90KG1fb2JqZWN0LCBQcm9wZXJ0eVNsb3Q6OkludGVybmFsTWV0aG9kVHlwZTo6R2V0 KTsKICAgICAgICAgSWRlbnRpZmllciYgcHJvcGVydHlOYW1lID0gbV9wcm9wZXJ0eU5hbWVzLT5w cm9wZXJ0eU5hbWVWZWN0b3IoKVtpbmRleF07Ci0gICAgICAgIGJvb2wgaGFzUHJvcGVydHkgPSBt X29iamVjdC0+Z2V0UHJvcGVydHlTbG90KGdsb2JhbE9iamVjdCwgcHJvcGVydHlOYW1lLCBzbG90 KTsKLSAgICAgICAgRVhDRVBUSU9OX0FTU0VSVCghc2NvcGUuZXhjZXB0aW9uKCkgfHwgIWhhc1By b3BlcnR5KTsKLSAgICAgICAgaWYgKCFoYXNQcm9wZXJ0eSkKLSAgICAgICAgICAgIHJldHVybiB0 cnVlOwotICAgICAgICBKU1ZhbHVlIHZhbHVlID0gc2xvdC5nZXRWYWx1ZShnbG9iYWxPYmplY3Qs IHByb3BlcnR5TmFtZSk7CisgICAgICAgIEpTVmFsdWUgdmFsdWUgPSBtX29iamVjdC0+Z2V0KGds b2JhbE9iamVjdCwgcHJvcGVydHlOYW1lKTsKICAgICAgICAgUkVUVVJOX0lGX0VYQ0VQVElPTihz Y29wZSwgZmFsc2UpOwogCiAgICAgICAgIHJvbGxCYWNrUG9pbnQgPSBidWlsZGVyLmxlbmd0aCgp Owo=