52167 2011-01-10 13:04:34 -0800 Remove unused isString() case in JSString::toPrimitiveString() 2011-01-10 13:28:59 -0800 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P2 Normal --- 1 xan.lopez webkit-unassigned xan.lopez oldest_to_newest 331867 0 xan.lopez 2011-01-10 13:04:34 -0800 JSString::toPrimitiveString() is called from two places in JSC, one of them very hot according to profiles. In both of them the code does something like: v.isString() ? doSomething(v.asString()) : doSomething(v.toPrimitiveString()); so the method is never called with a string. However, this is the first thing that the code checks, so it seems we are wasting some cycles here. Removing it yields a significant perf win: TEST COMPARISON FROM TO DETAILS ============================================================================= ** TOTAL **: 1.007x as fast 264.5ms +/- 0.5% 262.7ms +/- 0.4% significant ============================================================================= 3d: - 37.7ms +/- 1.8% 37.7ms +/- 1.5% cube: ?? 14.0ms +/- 4.1% 14.3ms +/- 3.6% not conclusive: might be *1.021x as slow* morph: - 10.8ms +/- 1.7% 10.8ms +/- 1.8% raytrace: 1.022x as fast 12.9ms +/- 1.3% 12.6ms +/- 1.0% significant access: 1.013x as fast 36.9ms +/- 0.7% 36.5ms +/- 0.5% significant binary-trees: - 3.3ms +/- 2.7% 3.2ms +/- 2.4% fannkuch: 1.010x as fast 18.3ms +/- 0.8% 18.1ms +/- 0.4% significant nbody: 1.019x as fast 10.3ms +/- 0.9% 10.1ms +/- 0.6% significant nsieve: - 5.0ms +/- 0.0% 5.0ms +/- 0.0% bitops: - 23.0ms +/- 0.7% 22.8ms +/- 0.7% 3bit-bits-in-byte: ?? 3.0ms +/- 0.0% 3.0ms +/- 0.7% not conclusive: might be *1.003x as slow* bits-in-byte: - 5.7ms +/- 1.7% 5.6ms +/- 1.8% bitwise-and: - 5.3ms +/- 1.7% 5.2ms +/- 1.5% nsieve-bits: - 9.0ms +/- 0.0% 9.0ms +/- 0.0% controlflow: - 2.0ms +/- 0.0% 2.0ms +/- 0.0% recursive: - 2.0ms +/- 0.0% 2.0ms +/- 0.0% crypto: 1.012x as fast 15.3ms +/- 0.7% 15.1ms +/- 0.4% significant aes: 1.019x as fast 9.2ms +/- 1.0% 9.1ms +/- 0.6% significant md5: - 3.0ms +/- 0.9% 3.0ms +/- 0.7% sha1: - 3.0ms +/- 0.0% 3.0ms +/- 0.0% date: - 29.6ms +/- 0.6% 29.4ms +/- 0.5% format-tofte: - 15.3ms +/- 0.6% 15.2ms +/- 0.5% format-xparb: - 14.3ms +/- 0.8% 14.3ms +/- 0.7% math: - 26.8ms +/- 0.9% 26.5ms +/- 0.8% cordic: 1.015x as fast 8.2ms +/- 1.0% 8.1ms +/- 0.8% significant partial-sums: - 14.3ms +/- 0.7% 14.2ms +/- 0.8% spectral-norm: - 4.3ms +/- 2.0% 4.2ms +/- 1.8% regexp: ?? 11.9ms +/- 0.8% 12.0ms +/- 0.7% not conclusive: might be *1.006x as slow* dna: ?? 11.9ms +/- 0.8% 12.0ms +/- 0.7% not conclusive: might be *1.006x as slow* string: 1.008x as fast 81.4ms +/- 0.4% 80.8ms +/- 0.4% significant base64: 1.032x as fast 9.0ms +/- 0.4% 8.7ms +/- 1.0% significant fasta: - 11.1ms +/- 0.5% 11.1ms +/- 0.5% tagcloud: - 20.3ms +/- 0.5% 20.2ms +/- 0.4% unpack-code: - 30.4ms +/- 0.5% 30.3ms +/- 0.5% validate-input: 1.022x as fast 10.7ms +/- 0.8% 10.5ms +/- 0.9% significant If for some reason it's desired for the method to contemplate this case, I guess at minimum we should move the branch at the end. Patch coming. 331870 1 78437 xan.lopez 2011-01-10 13:07:40 -0800 Created attachment 78437 toprimitivestring.diff 331877 2 78437 barraclough 2011-01-10 13:11:43 -0800 Comment on attachment 78437 toprimitivestring.diff Looks great! - ooi what's the perf win? - would be nice to mention in ChangeLog. 331891 3 78437 xan.lopez 2011-01-10 13:28:47 -0800 Comment on attachment 78437 toprimitivestring.diff Landed in http://trac.webkit.org/changeset/75427 331892 4 xan.lopez 2011-01-10 13:28:59 -0800 Closing bug. 78437 2011-01-10 13:07:40 -0800 2011-01-10 13:28:46 -0800 toprimitivestring.diff toprimitivestring.diff text/plain 2208 xan.lopez RnJvbSBjMjM0ZTA5YmQxODc3OGFiNTAzNzBhMzUxNGJhMzJhMmQwM2NjZmQwIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBYYW4gTG9wZXogPHhhbkBnbm9tZS5vcmc+CkRhdGU6IE1vbiwg MTAgSmFuIDIwMTEgMjI6MDY6NTkgKzAxMDAKU3ViamVjdDogW1BBVENIXSAyMDExLTAxLTEwICBY YW4gTG9wZXogIDx4bG9wZXpAaWdhbGlhLmNvbT4KCiAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZ IChPT1BTISkuCgogICAgICAgIFJlbW92ZSB1bnVzZWQgaXNTdHJpbmcoKSBjYXNlIGluIEpTU3Ry aW5nOjp0b1ByaW1pdGl2ZVN0cmluZygpCiAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcv c2hvd19idWcuY2dpP2lkPTUyMTY3CgogICAgICAgIFdlIG5ldmVyIGNhbGwgdG9QcmltaXRpdmVT dHJpbmcoKSB3aXRoIHN0cmluZ3MsIHNvIHJlbW92ZSB0aGUKICAgICAgICBjaGVjayBhbmQgYWRk IGFuIEFTU0VSVCBlbnN1cmluZyB0aGlzIG5ldmVyIGhhcHBlbnMuIFNpZ25pZmljYW50CiAgICAg ICAgd2luIGluIHN1bnNwaWRlciwgc2luY2Ugb25lIG9mIHRoZSBjYWxsIHNpdGVzIGlzIHZlcnkg aG90LgoKICAgICAgICAqIHJ1bnRpbWUvSlNTdHJpbmcuaDoKICAgICAgICAoSlNDOjpKU1ZhbHVl Ojp0b1ByaW1pdGl2ZVN0cmluZyk6Ci0tLQogU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxv ZyAgICAgICAgICB8ICAgMTQgKysrKysrKysrKysrKysKIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9y dW50aW1lL0pTU3RyaW5nLmggfCAgICAzICstLQogMiBmaWxlcyBjaGFuZ2VkLCAxNSBpbnNlcnRp b25zKCspLCAyIGRlbGV0aW9ucygtKQoKZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29y ZS9DaGFuZ2VMb2cgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCmluZGV4IDQxNDI2 ZjEuLjNlMGMyNTkgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cK KysrIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3IEBACisy MDExLTAxLTEwICBYYW4gTG9wZXogIDx4bG9wZXpAaWdhbGlhLmNvbT4KKworICAgICAgICBSZXZp ZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBSZW1vdmUgdW51c2VkIGlzU3RyaW5n KCkgY2FzZSBpbiBKU1N0cmluZzo6dG9QcmltaXRpdmVTdHJpbmcoKQorICAgICAgICBodHRwczov L2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NTIxNjcKKworICAgICAgICBXZSBuZXZl ciBjYWxsIHRvUHJpbWl0aXZlU3RyaW5nKCkgd2l0aCBzdHJpbmdzLCBzbyByZW1vdmUgdGhlCisg ICAgICAgIGNoZWNrIGFuZCBhZGQgYW4gQVNTRVJUIGVuc3VyaW5nIHRoaXMgbmV2ZXIgaGFwcGVu cy4gU2lnbmlmaWNhbnQKKyAgICAgICAgd2luIGluIHN1bnNwaWRlciwgc2luY2Ugb25lIG9mIHRo ZSBjYWxsIHNpdGVzIGlzIHZlcnkgaG90LgorCisgICAgICAgICogcnVudGltZS9KU1N0cmluZy5o OgorICAgICAgICAoSlNDOjpKU1ZhbHVlOjp0b1ByaW1pdGl2ZVN0cmluZyk6CisKIDIwMTEtMDEt MDggIFBhdHJpY2sgR2Fuc3RlcmVyICA8cGFyb2dhQHdlYmtpdC5vcmc+CiAKICAgICAgICAgUmV2 aWV3ZWQgYnkgRGFyaW4gQWRsZXIuCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUv cnVudGltZS9KU1N0cmluZy5oIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvSlNTdHJp bmcuaAppbmRleCBmZWZmZmRlLi5iZTlmODgyIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvcnVudGltZS9KU1N0cmluZy5oCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50 aW1lL0pTU3RyaW5nLmgKQEAgLTYyNCw4ICs2MjQsNyBAQCBuYW1lc3BhY2UgSlNDIHsKIAogICAg IGlubGluZSBVU3RyaW5nIEpTVmFsdWU6OnRvUHJpbWl0aXZlU3RyaW5nKEV4ZWNTdGF0ZSogZXhl YykgY29uc3QKICAgICB7Ci0gICAgICAgIGlmIChpc1N0cmluZygpKQotICAgICAgICAgICAgcmV0 dXJuIHN0YXRpY19jYXN0PEpTU3RyaW5nKj4oYXNDZWxsKCkpLT52YWx1ZShleGVjKTsKKyAgICAg ICAgQVNTRVJUKCFpc1N0cmluZygpKTsKICAgICAgICAgaWYgKGlzSW50MzIoKSkKICAgICAgICAg ICAgIHJldHVybiBleGVjLT5nbG9iYWxEYXRhKCkubnVtZXJpY1N0cmluZ3MuYWRkKGFzSW50MzIo KSk7CiAgICAgICAgIGlmIChpc0RvdWJsZSgpKQotLSAKMS43LjMuNAoK