130857 2014-03-27 13:45:16 -0700 Avoid fetching JSObject::structure() repeatedly in putDirectInternal. 2014-03-27 22:34:10 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified RESOLVED WONTFIX P2 Normal --- 130887 1 kling kling commit-queue kling oldest_to_newest 995039 0 kling 2014-03-27 13:45:16 -0700 Avoid fetching JSObject::structure() repeatedly in putDirectInternal. 995040 1 227974 kling 2014-03-27 13:45:51 -0700 Created attachment 227974 Patch 995092 2 227974 mhahnenberg 2014-03-27 15:20:33 -0700 Comment on attachment 227974 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=227974&action=review r=me > Source/JavaScriptCore/runtime/JSObject.h:1346 > + if (Structure* newStructure = Structure::addPropertyTransitionToExistingStructure(this->structure(), propertyName, attributes, specificFunction, offset)) { s/this->structure()/structure/ right? 995107 3 kling 2014-03-27 15:39:49 -0700 Committed r166376: <http://trac.webkit.org/changeset/166376> 995155 4 227974 fpizlo 2014-03-27 16:51:37 -0700 Comment on attachment 227974 Patch This really should have asserted, in all of the places that you removed a load of the structure, that the structure didn't change. That's very important since there are subtle ways in which things could *become* effectful and obviously this code could break if that happened. 995157 5 kling 2014-03-27 16:54:20 -0700 (In reply to comment #4) > (From update of attachment 227974 [details]) > This really should have asserted, in all of the places that you removed a load of the structure, that the structure didn't change. That's very important since there are subtle ways in which things could *become* effectful and obviously this code could break if that happened. Sure, I'll slap that in there. 995241 6 commit-queue 2014-03-27 22:21:48 -0700 Re-opened since this is blocked by bug 130887 995246 7 kling 2014-03-27 22:33:39 -0700 (In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 227974 [details] [details]) > > This really should have asserted, in all of the places that you removed a load of the structure, that the structure didn't change. That's very important since there are subtle ways in which things could *become* effectful and obviously this code could break if that happened. > > Sure, I'll slap that in there. Ended up rolling this out instead. I had misread the profile that lead to making this change. 227974 2014-03-27 13:45:51 -0700 2014-03-27 16:51:37 -0700 Patch bug-130857.diff text/plain 3924 kling ZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cgYi9Tb3VyY2UvSmF2 YVNjcmlwdENvcmUvQ2hhbmdlTG9nCmluZGV4IGM1OGY2ZGEuLmM1YzA4OTkgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL0phdmFTY3JpcHRD b3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2IEBACisyMDE0LTAzLTI3ICBBbmRyZWFzIEtsaW5n ICA8YWtsaW5nQGFwcGxlLmNvbT4KKworICAgICAgICBBdm9pZCBmZXRjaGluZyBKU09iamVjdDo6 c3RydWN0dXJlKCkgcmVwZWF0ZWRseSBpbiBwdXREaXJlY3RJbnRlcm5hbC4KKyAgICAgICAgPGh0 dHBzOi8vd2Via2l0Lm9yZy9iLzEzMDg1Nz4KKworICAgICAgICBVc2UgdGhlIGNhY2hlZCBTdHJ1 Y3R1cmUqIGluc3RlYWQgb2YgcmUtZmV0Y2hpbmcgaXQgb3ZlciBhbmQgb3ZlciBzaW5jZQorICAg ICAgICB0aGF0J3MgYSBub24tdHJpdmlhbCBvcGVyYXRpb24gdGhlc2UgZGF5cy4KKworICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIHJ1bnRpbWUvSlNPYmpl Y3QuaDoKKyAgICAgICAgKEpTQzo6SlNPYmplY3Q6OnB1dERpcmVjdEludGVybmFsKToKKwogMjAx NC0wMy0yNyAgTWFyayBIYWhuZW5iZXJnICA8bWhhaG5lbmJlcmdAYXBwbGUuY29tPgogCiAgICAg ICAgIENsZWFuIHVwIHF1ZXN0aW9uYWJsZSBzdHlsZSBpbiBTY3JpcHRFeGVjdXRhYmxlOjpwcmVw YXJlRm9yRXhlY3V0aW9uSW1wbApkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFTY3JpcHRDb3JlL3J1 bnRpbWUvSlNPYmplY3QuaCBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0pTT2JqZWN0 LmgKaW5kZXggMjg2NmZhNS4uZWZlYWQ1YSAxMDA2NDQKLS0tIGEvU291cmNlL0phdmFTY3JpcHRD b3JlL3J1bnRpbWUvSlNPYmplY3QuaAorKysgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGlt ZS9KU09iamVjdC5oCkBAIC0xMzI1LDM1ICsxMzI1LDM1IEBAIGlubGluZSBib29sIEpTT2JqZWN0 OjpwdXREaXJlY3RJbnRlcm5hbChWTSYgdm0sIFByb3BlcnR5TmFtZSBwcm9wZXJ0eU5hbWUsIEpT VmFsCiAKICAgICAgICAgRGVmZXJHQyBkZWZlckdDKHZtLmhlYXApOwogICAgICAgICBCdXR0ZXJm bHkqIG5ld0J1dHRlcmZseSA9IGJ1dHRlcmZseSgpOwotICAgICAgICBpZiAodGhpcy0+c3RydWN0 dXJlKCktPnB1dFdpbGxHcm93T3V0T2ZMaW5lU3RvcmFnZSgpKQotICAgICAgICAgICAgbmV3QnV0 dGVyZmx5ID0gZ3Jvd091dE9mTGluZVN0b3JhZ2Uodm0sIHRoaXMtPnN0cnVjdHVyZSgpLT5vdXRP ZkxpbmVDYXBhY2l0eSgpLCB0aGlzLT5zdHJ1Y3R1cmUoKS0+c3VnZ2VzdGVkTmV3T3V0T2ZMaW5l U3RvcmFnZUNhcGFjaXR5KCkpOwotICAgICAgICBvZmZzZXQgPSB0aGlzLT5zdHJ1Y3R1cmUoKS0+ YWRkUHJvcGVydHlXaXRob3V0VHJhbnNpdGlvbih2bSwgcHJvcGVydHlOYW1lLCBhdHRyaWJ1dGVz LCBzcGVjaWZpY0Z1bmN0aW9uKTsKLSAgICAgICAgc2V0U3RydWN0dXJlQW5kQnV0dGVyZmx5KHZt LCB0aGlzLT5zdHJ1Y3R1cmUoKSwgbmV3QnV0dGVyZmx5KTsKKyAgICAgICAgaWYgKHN0cnVjdHVy ZS0+cHV0V2lsbEdyb3dPdXRPZkxpbmVTdG9yYWdlKCkpCisgICAgICAgICAgICBuZXdCdXR0ZXJm bHkgPSBncm93T3V0T2ZMaW5lU3RvcmFnZSh2bSwgc3RydWN0dXJlLT5vdXRPZkxpbmVDYXBhY2l0 eSgpLCBzdHJ1Y3R1cmUtPnN1Z2dlc3RlZE5ld091dE9mTGluZVN0b3JhZ2VDYXBhY2l0eSgpKTsK KyAgICAgICAgb2Zmc2V0ID0gc3RydWN0dXJlLT5hZGRQcm9wZXJ0eVdpdGhvdXRUcmFuc2l0aW9u KHZtLCBwcm9wZXJ0eU5hbWUsIGF0dHJpYnV0ZXMsIHNwZWNpZmljRnVuY3Rpb24pOworICAgICAg ICBzZXRTdHJ1Y3R1cmVBbmRCdXR0ZXJmbHkodm0sIHN0cnVjdHVyZSwgbmV3QnV0dGVyZmx5KTsK IAogICAgICAgICB2YWxpZGF0ZU9mZnNldChvZmZzZXQpOwotICAgICAgICBBU1NFUlQodGhpcy0+ c3RydWN0dXJlKCktPmlzVmFsaWRPZmZzZXQob2Zmc2V0KSk7CisgICAgICAgIEFTU0VSVChzdHJ1 Y3R1cmUtPmlzVmFsaWRPZmZzZXQob2Zmc2V0KSk7CiAgICAgICAgIHB1dERpcmVjdCh2bSwgb2Zm c2V0LCB2YWx1ZSk7CiAgICAgICAgIC8vIFNlZSBjb21tZW50IG9uIHNldE5ld1Byb3BlcnR5IGNh bGwgYmVsb3cuCiAgICAgICAgIGlmICghc3BlY2lmaWNGdW5jdGlvbikKICAgICAgICAgICAgIHNs b3Quc2V0TmV3UHJvcGVydHkodGhpcywgb2Zmc2V0KTsKICAgICAgICAgaWYgKGF0dHJpYnV0ZXMg JiBSZWFkT25seSkKLSAgICAgICAgICAgIHRoaXMtPnN0cnVjdHVyZSgpLT5zZXRDb250YWluc1Jl YWRPbmx5UHJvcGVydGllcygpOworICAgICAgICAgICAgc3RydWN0dXJlLT5zZXRDb250YWluc1Jl YWRPbmx5UHJvcGVydGllcygpOwogICAgICAgICByZXR1cm4gdHJ1ZTsKICAgICB9CiAKICAgICBQ cm9wZXJ0eU9mZnNldCBvZmZzZXQ7Ci0gICAgc2l6ZV90IGN1cnJlbnRDYXBhY2l0eSA9IHRoaXMt PnN0cnVjdHVyZSgpLT5vdXRPZkxpbmVDYXBhY2l0eSgpOwotICAgIGlmIChTdHJ1Y3R1cmUqIHN0 cnVjdHVyZSA9IFN0cnVjdHVyZTo6YWRkUHJvcGVydHlUcmFuc2l0aW9uVG9FeGlzdGluZ1N0cnVj dHVyZSh0aGlzLT5zdHJ1Y3R1cmUoKSwgcHJvcGVydHlOYW1lLCBhdHRyaWJ1dGVzLCBzcGVjaWZp Y0Z1bmN0aW9uLCBvZmZzZXQpKSB7CisgICAgc2l6ZV90IGN1cnJlbnRDYXBhY2l0eSA9IHN0cnVj dHVyZS0+b3V0T2ZMaW5lQ2FwYWNpdHkoKTsKKyAgICBpZiAoU3RydWN0dXJlKiBuZXdTdHJ1Y3R1 cmUgPSBTdHJ1Y3R1cmU6OmFkZFByb3BlcnR5VHJhbnNpdGlvblRvRXhpc3RpbmdTdHJ1Y3R1cmUo dGhpcy0+c3RydWN0dXJlKCksIHByb3BlcnR5TmFtZSwgYXR0cmlidXRlcywgc3BlY2lmaWNGdW5j dGlvbiwgb2Zmc2V0KSkgewogICAgICAgICBEZWZlckdDIGRlZmVyR0Modm0uaGVhcCk7CiAgICAg ICAgIEJ1dHRlcmZseSogbmV3QnV0dGVyZmx5ID0gYnV0dGVyZmx5KCk7Ci0gICAgICAgIGlmIChj dXJyZW50Q2FwYWNpdHkgIT0gc3RydWN0dXJlLT5vdXRPZkxpbmVDYXBhY2l0eSgpKSB7Ci0gICAg ICAgICAgICBBU1NFUlQoc3RydWN0dXJlICE9IHRoaXMtPnN0cnVjdHVyZSgpKTsKLSAgICAgICAg ICAgIG5ld0J1dHRlcmZseSA9IGdyb3dPdXRPZkxpbmVTdG9yYWdlKHZtLCBjdXJyZW50Q2FwYWNp dHksIHN0cnVjdHVyZS0+b3V0T2ZMaW5lQ2FwYWNpdHkoKSk7CisgICAgICAgIGlmIChjdXJyZW50 Q2FwYWNpdHkgIT0gbmV3U3RydWN0dXJlLT5vdXRPZkxpbmVDYXBhY2l0eSgpKSB7CisgICAgICAg ICAgICBBU1NFUlQobmV3U3RydWN0dXJlICE9IHN0cnVjdHVyZSk7CisgICAgICAgICAgICBuZXdC dXR0ZXJmbHkgPSBncm93T3V0T2ZMaW5lU3RvcmFnZSh2bSwgY3VycmVudENhcGFjaXR5LCBuZXdT dHJ1Y3R1cmUtPm91dE9mTGluZUNhcGFjaXR5KCkpOwogICAgICAgICB9CiAKICAgICAgICAgdmFs aWRhdGVPZmZzZXQob2Zmc2V0KTsKLSAgICAgICAgQVNTRVJUKHN0cnVjdHVyZS0+aXNWYWxpZE9m ZnNldChvZmZzZXQpKTsKLSAgICAgICAgc2V0U3RydWN0dXJlQW5kQnV0dGVyZmx5KHZtLCBzdHJ1 Y3R1cmUsIG5ld0J1dHRlcmZseSk7CisgICAgICAgIEFTU0VSVChuZXdTdHJ1Y3R1cmUtPmlzVmFs aWRPZmZzZXQob2Zmc2V0KSk7CisgICAgICAgIHNldFN0cnVjdHVyZUFuZEJ1dHRlcmZseSh2bSwg bmV3U3RydWN0dXJlLCBuZXdCdXR0ZXJmbHkpOwogICAgICAgICBwdXREaXJlY3Qodm0sIG9mZnNl dCwgdmFsdWUpOwogICAgICAgICAvLyBUaGlzIGlzIGEgbmV3IHByb3BlcnR5OyB0cmFuc2l0aW9u cyB3aXRoIHNwZWNpZmljIHZhbHVlcyBhcmUgbm90IGN1cnJlbnRseSBjYWNoYWJsZSwKICAgICAg ICAgLy8gc28gbGVhdmUgdGhlIHNsb3QgaW4gYW4gdW5jYWNoYWJsZSBzdGF0ZS4K