23128 2009-01-05 22:32:21 -0800 get/put_by_val need to respecialise in the face of ByteArray 2009-01-13 18:12:36 -0800 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Mac OS X 10.5 CLOSED FIXED http://nerget.com/canvas/perfTest.html P2 Normal --- 1 oliver oliver barraclough ggaren oldest_to_newest 104532 0 oliver 2009-01-05 22:32:21 -0800 Some rather ad hoc testing shows that if bytearray accesses hit the bytearray check first, image data access time would be improved by ~10-15% 105212 1 26605 oliver 2009-01-11 07:13:24 -0800 Created attachment 26605 Specialise get/put_by_val Nice and simple 105220 2 26605 andersca 2009-01-11 08:41:59 -0800 Comment on attachment 26605 Specialise get/put_by_val r=me 105221 3 oliver 2009-01-11 08:49:08 -0800 Committing to http://svn.webkit.org/repository/webkit/trunk ... M JavaScriptCore/ChangeLog M JavaScriptCore/interpreter/Interpreter.cpp M JavaScriptCore/interpreter/Interpreter.h Committed r39796 105226 4 26605 darin 2009-01-11 09:04:08 -0800 Comment on attachment 26605 Specialise get/put_by_val I know this was already reviewed, but I have a few comments. > + JSValuePtr result; > + unsigned i; > + > + bool isUInt32 = JSImmediate::getUInt32(subscript, i); I usually like to group the declaration of an out argument like "i" with the function call it's coming out of, rather than having it separated by a blank line. > + if (LIKELY(isUInt32)) { > + if (interpreter->isJSByteArray(baseValue) && asByteArray(baseValue)->canAccessIndex(i)) > + return JSValuePtr::encode(asByteArray(baseValue)->getIndex(i)); You need a comment explaining why this case does a return when all the other cases use "result" instead. I have no doubt there's a good reason, perhaps to save code since there can't be an exception in this case, but withut a comment it's a subtle point many people might missing. > double dValue = 0; > + ctiPatchCallByReturnAddress(STUB_RETURN_ADDRESS, reinterpret_cast<void*>(cti_op_put_by_val_byte_array)); > if (JSImmediate::isNumber(value)) { > jsByteArray->setIndex(i, JSImmediate::getTruncatedInt32(value)); > return; It seems a real shame that this code defines and initialized a double that's not used in the isNumber fast case. Can you move the definition of dValue down a bit? > + double dValue = 0; > + if (JSImmediate::isNumber(value)) { > + jsByteArray->setIndex(i, JSImmediate::getTruncatedInt32(value)); > + return; > + } else if (fastIsNumber(value, dValue)) { > + jsByteArray->setIndex(i, dValue); > + return; > + } else > + baseValue->put(callFrame, i, value); We normally don't do else after return. And again, I think those "return" need brief comments. 26605 2009-01-11 07:13:24 -0800 2009-01-11 08:41:59 -0800 Specialise get/put_by_val bug23128.patch text/plain 7001 oliver ZGlmZiAtLWdpdCBhL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZyBiL0phdmFTY3JpcHRDb3JlL0No YW5nZUxvZwppbmRleCBmNzdhMTZkLi5kYmM5Njc2IDEwMDY0NAotLS0gYS9KYXZhU2NyaXB0Q29y ZS9DaGFuZ2VMb2cKKysrIGIvSmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjMg QEAKKzIwMDktMDEtMTEgIE9saXZlciBIdW50ICA8b2xpdmVyQGFwcGxlLmNvbT4KKworICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBCdWcgMjMxMjg6IGdldC9w dXRfYnlfdmFsIG5lZWQgdG8gcmVzcGVjaWFsaXNlIGluIHRoZSBmYWNlIG9mIEJ5dGVBcnJheQor ICAgICAgICA8aHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTIzMTI4Pgor CisgICAgICAgIEZhaXJseSBzaW1wbGUgcGF0Y2gsIGFkZCBzcGVjaWFsaXNlZCB2ZXJzaW9ucyBv ZiBjdGlfb3BfZ2V0L3B1dF9ieV92YWwKKyAgICAgICAgdGhhdCBhc3N1bWUgQnl0ZUFycmF5LCB0 aHVzIGF2b2lkaW5nIGEgZmV3IGJyYW5jaGVzIGluIHRoZSBjYXNlIG9mIGJ5dGVhcnJheQorICAg ICAgICBtYW5pcHVsYXRpb24uCisKKyAgICAgICAgTm8gZWZmZWN0IG9uIFN1blNwaWRlci4gIDE1 JSB3aW4gb24gdGhlIG9yaWdpbmFsIHRlc3RjYXNlLgorCisgICAgICAgICogaW50ZXJwcmV0ZXIv SW50ZXJwcmV0ZXIuY3BwOgorICAgICAgICAoSlNDOjpJbnRlcnByZXRlcjo6Y3RpX29wX2dldF9i eV92YWwpOgorICAgICAgICAoSlNDOjpJbnRlcnByZXRlcjo6Y3RpX29wX2dldF9ieV92YWxfYnl0 ZV9hcnJheSk6CisgICAgICAgIChKU0M6OkludGVycHJldGVyOjpjdGlfb3BfcHV0X2J5X3ZhbCk6 CisgICAgICAgIChKU0M6OkludGVycHJldGVyOjpjdGlfb3BfcHV0X2J5X3ZhbF9ieXRlX2FycmF5 KToKKyAgICAgICAgKiBpbnRlcnByZXRlci9JbnRlcnByZXRlci5oOgorCiAyMDA5LTAxLTExICBB bGV4ZXkgUHJvc2t1cnlha292ICA8YXBAd2Via2l0Lm9yZz4KIAogICAgICAgICBUcnkgdG8gZml4 IFdpbmRvd3MgYnVpbGQuCmRpZmYgLS1naXQgYS9KYXZhU2NyaXB0Q29yZS9pbnRlcnByZXRlci9J bnRlcnByZXRlci5jcHAgYi9KYXZhU2NyaXB0Q29yZS9pbnRlcnByZXRlci9JbnRlcnByZXRlci5j cHAKaW5kZXggM2U1ZmZiMS4uYjBkNjE1ZiAxMDA2NDQKLS0tIGEvSmF2YVNjcmlwdENvcmUvaW50 ZXJwcmV0ZXIvSW50ZXJwcmV0ZXIuY3BwCisrKyBiL0phdmFTY3JpcHRDb3JlL2ludGVycHJldGVy L0ludGVycHJldGVyLmNwcApAQCAtNTExNyw5ICs1MTE3LDEwIEBAIEpTVmFsdWVFbmNvZGVkQXNQ b2ludGVyKiBJbnRlcnByZXRlcjo6Y3RpX29wX2dldF9ieV92YWwoU1RVQl9BUkdTKQogICAgICAg ICAgICAgICAgIHJlc3VsdCA9IGpzQXJyYXktPkpTQXJyYXk6OmdldChjYWxsRnJhbWUsIGkpOwog ICAgICAgICB9IGVsc2UgaWYgKGludGVycHJldGVyLT5pc0pTU3RyaW5nKGJhc2VWYWx1ZSkgJiYg YXNTdHJpbmcoYmFzZVZhbHVlKS0+Y2FuR2V0SW5kZXgoaSkpCiAgICAgICAgICAgICByZXR1cm4g SlNWYWx1ZVB0cjo6ZW5jb2RlKGFzU3RyaW5nKGJhc2VWYWx1ZSktPmdldEluZGV4KEFSR19nbG9i YWxEYXRhLCBpKSk7Ci0gICAgICAgIGVsc2UgaWYgKGludGVycHJldGVyLT5pc0pTQnl0ZUFycmF5 KGJhc2VWYWx1ZSkgJiYgYXNCeXRlQXJyYXkoYmFzZVZhbHVlKS0+Y2FuQWNjZXNzSW5kZXgoaSkp CisgICAgICAgIGVsc2UgaWYgKGludGVycHJldGVyLT5pc0pTQnl0ZUFycmF5KGJhc2VWYWx1ZSkg JiYgYXNCeXRlQXJyYXkoYmFzZVZhbHVlKS0+Y2FuQWNjZXNzSW5kZXgoaSkpIHsKKyAgICAgICAg ICAgIGN0aVBhdGNoQ2FsbEJ5UmV0dXJuQWRkcmVzcyhTVFVCX1JFVFVSTl9BRERSRVNTLCByZWlu dGVycHJldF9jYXN0PHZvaWQqPihjdGlfb3BfZ2V0X2J5X3ZhbF9ieXRlX2FycmF5KSk7CiAgICAg ICAgICAgICByZXR1cm4gSlNWYWx1ZVB0cjo6ZW5jb2RlKGFzQnl0ZUFycmF5KGJhc2VWYWx1ZSkt PmdldEluZGV4KGkpKTsKLSAgICAgICAgZWxzZQorICAgICAgICB9IGVsc2UKICAgICAgICAgICAg IHJlc3VsdCA9IGJhc2VWYWx1ZS0+Z2V0KGNhbGxGcmFtZSwgaSk7CiAgICAgfSBlbHNlIHsKICAg ICAgICAgSWRlbnRpZmllciBwcm9wZXJ0eShjYWxsRnJhbWUsIHN1YnNjcmlwdC0+dG9TdHJpbmco Y2FsbEZyYW1lKSk7CkBAIC01MTMwLDYgKzUxMzEsMzcgQEAgSlNWYWx1ZUVuY29kZWRBc1BvaW50 ZXIqIEludGVycHJldGVyOjpjdGlfb3BfZ2V0X2J5X3ZhbChTVFVCX0FSR1MpCiAgICAgcmV0dXJu IEpTVmFsdWVQdHI6OmVuY29kZShyZXN1bHQpOwogfQogCitKU1ZhbHVlRW5jb2RlZEFzUG9pbnRl ciogSW50ZXJwcmV0ZXI6OmN0aV9vcF9nZXRfYnlfdmFsX2J5dGVfYXJyYXkoU1RVQl9BUkdTKQor eworICAgIEJFR0lOX1NUVUJfRlVOQ1RJT04oKTsKKyAgICAKKyAgICBDYWxsRnJhbWUqIGNhbGxG cmFtZSA9IEFSR19jYWxsRnJhbWU7CisgICAgSW50ZXJwcmV0ZXIqIGludGVycHJldGVyID0gQVJH X2dsb2JhbERhdGEtPmludGVycHJldGVyOworICAgIAorICAgIEpTVmFsdWVQdHIgYmFzZVZhbHVl ID0gQVJHX3NyYzE7CisgICAgSlNWYWx1ZVB0ciBzdWJzY3JpcHQgPSBBUkdfc3JjMjsKKyAgICAK KyAgICBKU1ZhbHVlUHRyIHJlc3VsdDsKKyAgICB1bnNpZ25lZCBpOworICAgIAorICAgIGJvb2wg aXNVSW50MzIgPSBKU0ltbWVkaWF0ZTo6Z2V0VUludDMyKHN1YnNjcmlwdCwgaSk7CisgICAgaWYg KExJS0VMWShpc1VJbnQzMikpIHsKKyAgICAgICAgaWYgKGludGVycHJldGVyLT5pc0pTQnl0ZUFy cmF5KGJhc2VWYWx1ZSkgJiYgYXNCeXRlQXJyYXkoYmFzZVZhbHVlKS0+Y2FuQWNjZXNzSW5kZXgo aSkpCisgICAgICAgICAgICByZXR1cm4gSlNWYWx1ZVB0cjo6ZW5jb2RlKGFzQnl0ZUFycmF5KGJh c2VWYWx1ZSktPmdldEluZGV4KGkpKTsKKyAgICAgICAgZWxzZSB7CisgICAgICAgICAgICByZXN1 bHQgPSBiYXNlVmFsdWUtPmdldChjYWxsRnJhbWUsIGkpOworICAgICAgICAgICAgaWYgKCFpbnRl cnByZXRlci0+aXNKU0J5dGVBcnJheShiYXNlVmFsdWUpKQorICAgICAgICAgICAgICAgIGN0aVBh dGNoQ2FsbEJ5UmV0dXJuQWRkcmVzcyhTVFVCX1JFVFVSTl9BRERSRVNTLCByZWludGVycHJldF9j YXN0PHZvaWQqPihjdGlfb3BfZ2V0X2J5X3ZhbCkpOworICAgICAgICB9CisgICAgfSBlbHNlIHsK KyAgICAgICAgSWRlbnRpZmllciBwcm9wZXJ0eShjYWxsRnJhbWUsIHN1YnNjcmlwdC0+dG9TdHJp bmcoY2FsbEZyYW1lKSk7CisgICAgICAgIHJlc3VsdCA9IGJhc2VWYWx1ZS0+Z2V0KGNhbGxGcmFt ZSwgcHJvcGVydHkpOworICAgIH0KKyAgICAKKyAgICBDSEVDS19GT1JfRVhDRVBUSU9OX0FUX0VO RCgpOworICAgIHJldHVybiBKU1ZhbHVlUHRyOjplbmNvZGUocmVzdWx0KTsKK30KKwogVm9pZFB0 clBhaXIgSW50ZXJwcmV0ZXI6OmN0aV9vcF9yZXNvbHZlX2Z1bmMoU1RVQl9BUkdTKQogewogICAg IEJFR0lOX1NUVUJfRlVOQ1RJT04oKTsKQEAgLTUyMTQsNiArNTI0Niw3IEBAIHZvaWQgSW50ZXJw cmV0ZXI6OmN0aV9vcF9wdXRfYnlfdmFsKFNUVUJfQVJHUykKICAgICAgICAgfSBlbHNlIGlmIChp bnRlcnByZXRlci0+aXNKU0J5dGVBcnJheShiYXNlVmFsdWUpICYmIGFzQnl0ZUFycmF5KGJhc2VW YWx1ZSktPmNhbkFjY2Vzc0luZGV4KGkpKSB7CiAgICAgICAgICAgICBKU0J5dGVBcnJheSoganNC eXRlQXJyYXkgPSBhc0J5dGVBcnJheShiYXNlVmFsdWUpOwogICAgICAgICAgICAgZG91YmxlIGRW YWx1ZSA9IDA7CisgICAgICAgICAgICBjdGlQYXRjaENhbGxCeVJldHVybkFkZHJlc3MoU1RVQl9S RVRVUk5fQUREUkVTUywgcmVpbnRlcnByZXRfY2FzdDx2b2lkKj4oY3RpX29wX3B1dF9ieV92YWxf Ynl0ZV9hcnJheSkpOwogICAgICAgICAgICAgaWYgKEpTSW1tZWRpYXRlOjppc051bWJlcih2YWx1 ZSkpIHsKICAgICAgICAgICAgICAgICBqc0J5dGVBcnJheS0+c2V0SW5kZXgoaSwgSlNJbW1lZGlh dGU6OmdldFRydW5jYXRlZEludDMyKHZhbHVlKSk7CiAgICAgICAgICAgICAgICAgcmV0dXJuOwpA QCAtNTI2MSw2ICs1Mjk0LDQ4IEBAIHZvaWQgSW50ZXJwcmV0ZXI6OmN0aV9vcF9wdXRfYnlfdmFs X2FycmF5KFNUVUJfQVJHUykKICAgICBDSEVDS19GT1JfRVhDRVBUSU9OX0FUX0VORCgpOwogfQog Cit2b2lkIEludGVycHJldGVyOjpjdGlfb3BfcHV0X2J5X3ZhbF9ieXRlX2FycmF5KFNUVUJfQVJH UykKK3sKKyAgICBCRUdJTl9TVFVCX0ZVTkNUSU9OKCk7CisgICAgCisgICAgQ2FsbEZyYW1lKiBj YWxsRnJhbWUgPSBBUkdfY2FsbEZyYW1lOworICAgIEludGVycHJldGVyKiBpbnRlcnByZXRlciA9 IEFSR19nbG9iYWxEYXRhLT5pbnRlcnByZXRlcjsKKyAgICAKKyAgICBKU1ZhbHVlUHRyIGJhc2VW YWx1ZSA9IEFSR19zcmMxOworICAgIEpTVmFsdWVQdHIgc3Vic2NyaXB0ID0gQVJHX3NyYzI7Cisg ICAgSlNWYWx1ZVB0ciB2YWx1ZSA9IEFSR19zcmMzOworICAgIAorICAgIHVuc2lnbmVkIGk7Cisg ICAgCisgICAgYm9vbCBpc1VJbnQzMiA9IEpTSW1tZWRpYXRlOjpnZXRVSW50MzIoc3Vic2NyaXB0 LCBpKTsKKyAgICBpZiAoTElLRUxZKGlzVUludDMyKSkgeworICAgICAgICBpZiAoaW50ZXJwcmV0 ZXItPmlzSlNCeXRlQXJyYXkoYmFzZVZhbHVlKSAmJiBhc0J5dGVBcnJheShiYXNlVmFsdWUpLT5j YW5BY2Nlc3NJbmRleChpKSkgeworICAgICAgICAgICAgSlNCeXRlQXJyYXkqIGpzQnl0ZUFycmF5 ID0gYXNCeXRlQXJyYXkoYmFzZVZhbHVlKTsKKyAgICAgICAgICAgIGRvdWJsZSBkVmFsdWUgPSAw OworICAgICAgICAgICAgaWYgKEpTSW1tZWRpYXRlOjppc051bWJlcih2YWx1ZSkpIHsKKyAgICAg ICAgICAgICAgICBqc0J5dGVBcnJheS0+c2V0SW5kZXgoaSwgSlNJbW1lZGlhdGU6OmdldFRydW5j YXRlZEludDMyKHZhbHVlKSk7CisgICAgICAgICAgICAgICAgcmV0dXJuOworICAgICAgICAgICAg fSBlbHNlIGlmIChmYXN0SXNOdW1iZXIodmFsdWUsIGRWYWx1ZSkpIHsKKyAgICAgICAgICAgICAg ICBqc0J5dGVBcnJheS0+c2V0SW5kZXgoaSwgZFZhbHVlKTsKKyAgICAgICAgICAgICAgICByZXR1 cm47CisgICAgICAgICAgICB9IGVsc2UKKyAgICAgICAgICAgICAgICBiYXNlVmFsdWUtPnB1dChj YWxsRnJhbWUsIGksIHZhbHVlKTsKKyAgICAgICAgfSBlbHNlIHsKKyAgICAgICAgICAgIGlmICgh aW50ZXJwcmV0ZXItPmlzSlNCeXRlQXJyYXkoYmFzZVZhbHVlKSkKKyAgICAgICAgICAgICAgICBj dGlQYXRjaENhbGxCeVJldHVybkFkZHJlc3MoU1RVQl9SRVRVUk5fQUREUkVTUywgcmVpbnRlcnBy ZXRfY2FzdDx2b2lkKj4oY3RpX29wX3B1dF9ieV92YWwpKTsKKyAgICAgICAgICAgIGJhc2VWYWx1 ZS0+cHV0KGNhbGxGcmFtZSwgaSwgdmFsdWUpOworICAgICAgICB9CisgICAgfSBlbHNlIHsKKyAg ICAgICAgSWRlbnRpZmllciBwcm9wZXJ0eShjYWxsRnJhbWUsIHN1YnNjcmlwdC0+dG9TdHJpbmco Y2FsbEZyYW1lKSk7CisgICAgICAgIGlmICghQVJHX2dsb2JhbERhdGEtPmV4Y2VwdGlvbikgeyAv LyBEb24ndCBwdXQgdG8gYW4gb2JqZWN0IGlmIHRvU3RyaW5nIHRocmV3IGFuIGV4Y2VwdGlvbi4K KyAgICAgICAgICAgIFB1dFByb3BlcnR5U2xvdCBzbG90OworICAgICAgICAgICAgYmFzZVZhbHVl LT5wdXQoY2FsbEZyYW1lLCBwcm9wZXJ0eSwgdmFsdWUsIHNsb3QpOworICAgICAgICB9CisgICAg fQorICAgIAorICAgIENIRUNLX0ZPUl9FWENFUFRJT05fQVRfRU5EKCk7Cit9CisKIEpTVmFsdWVF bmNvZGVkQXNQb2ludGVyKiBJbnRlcnByZXRlcjo6Y3RpX29wX2xlc3NlcShTVFVCX0FSR1MpCiB7 CiAgICAgQkVHSU5fU1RVQl9GVU5DVElPTigpOwpkaWZmIC0tZ2l0IGEvSmF2YVNjcmlwdENvcmUv aW50ZXJwcmV0ZXIvSW50ZXJwcmV0ZXIuaCBiL0phdmFTY3JpcHRDb3JlL2ludGVycHJldGVyL0lu dGVycHJldGVyLmgKaW5kZXggMjZkNjczMi4uZWJhMWI5NCAxMDA2NDQKLS0tIGEvSmF2YVNjcmlw dENvcmUvaW50ZXJwcmV0ZXIvSW50ZXJwcmV0ZXIuaAorKysgYi9KYXZhU2NyaXB0Q29yZS9pbnRl cnByZXRlci9JbnRlcnByZXRlci5oCkBAIC0yMjEsMTAgKzIyMSwxMiBAQCBuYW1lc3BhY2UgSlND IHsKICAgICAgICAgc3RhdGljIEpTT2JqZWN0KiBKSVRfU1RVQiBjdGlfb3BfY29uc3RydWN0X0pT Q29uc3RydWN0KFNUVUJfQVJHUyk7CiAgICAgICAgIHN0YXRpYyBKU1ZhbHVlRW5jb2RlZEFzUG9p bnRlciogSklUX1NUVUIgY3RpX29wX2NvbnN0cnVjdF9Ob3RKU0NvbnN0cnVjdChTVFVCX0FSR1Mp OwogICAgICAgICBzdGF0aWMgSlNWYWx1ZUVuY29kZWRBc1BvaW50ZXIqIEpJVF9TVFVCIGN0aV9v cF9nZXRfYnlfdmFsKFNUVUJfQVJHUyk7CisgICAgICAgIHN0YXRpYyBKU1ZhbHVlRW5jb2RlZEFz UG9pbnRlciogSklUX1NUVUIgY3RpX29wX2dldF9ieV92YWxfYnl0ZV9hcnJheShTVFVCX0FSR1Mp OwogICAgICAgICBzdGF0aWMgVm9pZFB0clBhaXIgSklUX1NUVUIgY3RpX29wX3Jlc29sdmVfZnVu YyhTVFVCX0FSR1MpOwogICAgICAgICBzdGF0aWMgSlNWYWx1ZUVuY29kZWRBc1BvaW50ZXIqIEpJ VF9TVFVCIGN0aV9vcF9zdWIoU1RVQl9BUkdTKTsKICAgICAgICAgc3RhdGljIHZvaWQgSklUX1NU VUIgY3RpX29wX3B1dF9ieV92YWwoU1RVQl9BUkdTKTsKICAgICAgICAgc3RhdGljIHZvaWQgSklU X1NUVUIgY3RpX29wX3B1dF9ieV92YWxfYXJyYXkoU1RVQl9BUkdTKTsKKyAgICAgICAgc3RhdGlj IHZvaWQgSklUX1NUVUIgY3RpX29wX3B1dF9ieV92YWxfYnl0ZV9hcnJheShTVFVCX0FSR1MpOwog ICAgICAgICBzdGF0aWMgSlNWYWx1ZUVuY29kZWRBc1BvaW50ZXIqIEpJVF9TVFVCIGN0aV9vcF9s ZXNzZXEoU1RVQl9BUkdTKTsKICAgICAgICAgc3RhdGljIGludCBKSVRfU1RVQiBjdGlfb3BfbG9v cF9pZl90cnVlKFNUVUJfQVJHUyk7CiAgICAgICAgIHN0YXRpYyBKU1ZhbHVlRW5jb2RlZEFzUG9p bnRlciogSklUX1NUVUIgY3RpX29wX3Jlc29sdmVfYmFzZShTVFVCX0FSR1MpOwo=