20209 2008-07-29 06:52:25 -0700 Atomize constant strings 2008-07-30 08:35:33 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Mac OS X 10.5 RESOLVED FIXED P2 Normal --- 1 barraclough barraclough ap ggaren oliver oldest_to_newest 87265 0 barraclough 2008-07-29 06:52:25 -0700 Example of code that currently performs poorly: function get(self, keyword) { // do stuff return self[keyword]; } function f1() { var localGet = get; var object = {}; var start = new Date; for (var i =0; i < 10000000; i++) get(object, 'foo'); print("f1 time taken: " + (new Date - start)/1000 + "s"); } function f2() { var object = {}; var startTime = new Date; for (var i =0; i < 10000000; i++) object['foo']; print("f2 time taken: " + (new Date - startTime)/1000 + "s"); } f1(); f2(); The problem in the above script is that the string 'foo' is used twice as an identifier. Only 'foo' from f1() will be added to the identifier map, degrading the performance of f2(). Fix this by turning constant strings into identifiers during code generation. (patch following). 87266 1 22537 barraclough 2008-07-29 06:53:21 -0700 Created attachment 22537 fix 87269 2 ap 2008-07-29 08:24:11 -0700 I'm getting some weird results on the above test with a r35249 nightly. f1 takes ~2.3s, while f2 alternates between 1.6s and 0.9s (odd runs are slow, even runs are fast, no values in between). On a Shark profile of a slow run, 36% of time is spent in Identifier::addSlowCase, and for fast runs, this function is not on the profile. 87335 3 ggaren 2008-07-29 15:44:42 -0700 > I'm getting some weird results on the above test with a r35249 nightly. f1 > takes ~2.3s, while f2 alternates between 1.6s and 0.9s (odd runs are slow, even > runs are fast, no values in between). Perhaps the garbage collector's effect on the lifetime of an identifier is in play here. 87336 4 22537 ggaren 2008-07-29 15:52:22 -0700 Comment on attachment 22537 fix Note that the patch I suggested to convert a['s'] to a.s during codegen would also fix this particular test case. + // String literals are atomized (internalized) in the identifier map. Probably better to state the why here, rather than the what. For example, "We atomize constant strings, in case they're later used in property lookup." + a script contains multiple identical strings that are used as keys keys to identify Typo: "keys keys" should be "keys." Please add a bugzilla link to the ChangeLog. r=me 87384 5 barraclough 2008-07-30 07:56:55 -0700 (In reply to comment #4) > (From update of attachment 22537 [edit]) > Note that the patch I suggested to convert a['s'] to a.s during codegen would > also fix this particular test case. Ah yes, that's quite true, thanks – I had writing a test case for this on my todo list, since we don't hit it in sunspider – that's one problem solved! – will raise a bug & submit a patch. G. 87385 6 barraclough 2008-07-30 07:59:24 -0700 Sending JavaScriptCore/ChangeLog Sending JavaScriptCore/kjs/nodes.cpp Transmitting file data .. Committed revision 35446. 87388 7 barraclough 2008-07-30 08:35:33 -0700 Optimize [ "constant string" ] into an identifier access. https://bugs.webkit.org/show_bug.cgi?id=20227 22537 2008-07-29 06:53:21 -0700 2008-07-29 15:52:22 -0700 fix patch_const_str.txt text/plain 1480 barraclough SW5kZXg6IEphdmFTY3JpcHRDb3JlL2tqcy9ub2Rlcy5jcHAKPT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gSmF2YVNj cmlwdENvcmUva2pzL25vZGVzLmNwcAkocmV2aXNpb24gMzU0MTgpCisrKyBKYXZhU2NyaXB0Q29y ZS9ranMvbm9kZXMuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0yNTIsOCArMjUyLDkgQEAKIHsKICAg ICBpZiAoZHN0ID09IGlnbm9yZWRSZXN1bHQoKSkKICAgICAgICAgcmV0dXJuIDA7Ci0gICAgLy8g RklYTUU6IHNob3VsZCB3ZSB0cnkgdG8gYXRvbWl6ZSBjb25zdGFudCBzdHJpbmdzPwotICAgIHJl dHVybiBnZW5lcmF0b3IuZW1pdExvYWQoZ2VuZXJhdG9yLmZpbmFsRGVzdGluYXRpb24oZHN0KSwg anNPd25lZFN0cmluZyhnZW5lcmF0b3IuZ2xvYmFsRXhlYygpLCBtX3ZhbHVlKSk7CisKKyAgICAv LyBTdHJpbmcgbGl0ZXJhbHMgYXJlIGF0b21pemVkIChpbnRlcm5hbGl6ZWQpIGluIHRoZSBpZGVu dGlmaWVyIG1hcC4KKyAgICByZXR1cm4gZ2VuZXJhdG9yLmVtaXRMb2FkKGdlbmVyYXRvci5maW5h bERlc3RpbmF0aW9uKGRzdCksIGpzT3duZWRTdHJpbmcoZ2VuZXJhdG9yLmdsb2JhbEV4ZWMoKSwg SWRlbnRpZmllcihnZW5lcmF0b3IuZ2xvYmFsRXhlYygpLCBtX3ZhbHVlKS51c3RyaW5nKCkpKTsK IH0KIAogLy8gLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tIFJlZ0V4cE5vZGUgLS0tLS0t LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0KSW5kZXg6IEphdmFTY3JpcHRDb3JlL0NoYW5n ZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09Ci0tLSBKYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDM1 NDE4KQorKysgSmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMg KzEsMTUgQEAKKzIwMDgtMDctMjkgIEdhdmluIEJhcnJhY2xvdWdoICA8YmFycmFjbG91Z2hAYXBw bGUuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAg IEF0b21pemUgY29uc3RhbnQgc3RyaW5ncy4gIFByZXZlbnRzIHNpZ25pZmljYW50IHBlcmZvcm1h bmNlIGRlZ3JhZGF0aW9uIHNlZW4gd2hlbgorICAgICAgICBhIHNjcmlwdCBjb250YWlucyBtdWx0 aXBsZSBpZGVudGljYWwgc3RyaW5ncyB0aGF0IGFyZSB1c2VkIGFzIGtleXMga2V5cyB0byBpZGVu dGlmeQorICAgICAgICBwcm9wZXJ0aWVzIG9uIG9iamVjdHMuCisKKyAgICAgICAgTm8gcGVyZm9y bWFuY2UgY2hhbmdlIG9uIFN1blNwaWRlci4KKworICAgICAgICAqIGtqcy9ub2Rlcy5jcHA6IEF0 b21pemUgY29uc3RhbnQgc3RyaW5ncy4KKwogMjAwOC0wNy0yOSAgQWxleGV5IFByb3NrdXJ5YWtv diAgPGFwQHdlYmtpdC5vcmc+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgT2xpdmVyIEh1bnQuCg==