163490 2016-10-15 13:32:34 -0700 Build fix for HasOwnPropertyCache::Entry caused slowdown by introducing a constructor that doesn't use move semantics for the RefPtr<UniquedStringImpl> parameter 2016-10-17 13:37:02 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 saam saam benjamin commit-queue darin fpizlo ggaren gskachkov jfbastien keith_miller mark.lam msaboff oliver ossy ticaiolima ysuzuki oldest_to_newest 1240622 0 saam 2016-10-15 13:32:34 -0700 ... 1241059 1 291843 saam 2016-10-17 11:33:23 -0700 Created attachment 291843 patch 1241067 2 291843 darin 2016-10-17 11:38:18 -0700 Comment on attachment 291843 patch View in context: https://bugs.webkit.org/attachment.cgi?id=291843&action=review > Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:48 > + Entry(RefPtr<UniquedStringImpl>&& impl, StructureID structureID, bool result) > + : impl(WTFMove(impl)) Looks good. > Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:124 > - bitwise_cast<Entry*>(this)[index] = Entry{ RefPtr<UniquedStringImpl>(impl), id, result }; > + bitwise_cast<Entry*>(this)[index] = Entry(RefPtr<UniquedStringImpl>(impl), id, result); I suggest not making this change. Using the function-call-style () syntax instead of the initialization-style {} syntax allows some kinds of implicit type conversions and it’s nice to not allow them. Generally, I think the { } syntax should be preferred in new code. In addition, Anders Carlsson convinced me that we should have a space after the type name, but I won’t try to convince you of that right now. 1241072 3 291847 saam 2016-10-17 11:42:07 -0700 Created attachment 291847 patch for landing 1241074 4 291843 saam 2016-10-17 11:44:07 -0700 Comment on attachment 291843 patch View in context: https://bugs.webkit.org/attachment.cgi?id=291843&action=review >> Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:124 >> + bitwise_cast<Entry*>(this)[index] = Entry(RefPtr<UniquedStringImpl>(impl), id, result); > > I suggest not making this change. > > Using the function-call-style () syntax instead of the initialization-style {} syntax allows some kinds of implicit type conversions and it’s nice to not allow them. Generally, I think the { } syntax should be preferred in new code. In addition, Anders Carlsson convinced me that we should have a space after the type name, but I won’t try to convince you of that right now. Ok, I've changed it back, and with the space between the type name and the "{". I actually prefer the space, however, I've never been aware of any style rules for WebKit itself. If we haven't decided on an official style, we probably should, and put it in the style guide. I like the property of not allowing type conversions and allowing the caller to control that instead of the callee. 1241127 5 291847 commit-queue 2016-10-17 13:36:57 -0700 Comment on attachment 291847 patch for landing Clearing flags on attachment: 291847 Committed r207425: <http://trac.webkit.org/changeset/207425> 1241128 6 commit-queue 2016-10-17 13:37:02 -0700 All reviewed patches have been landed. Closing bug. 291843 2016-10-17 11:33:23 -0700 2016-10-17 11:42:07 -0700 patch a-backup.diff text/plain 1964 saam SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMjA3NDE3KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE0IEBA CisyMDE2LTEwLTE3ICBTYWFtIEJhcmF0aSAgPHNiYXJhdGlAYXBwbGUuY29tPgorCisgICAgICAg IEJ1aWxkIGZpeCBmb3IgSGFzT3duUHJvcGVydHlDYWNoZTo6RW50cnkgY2F1c2VkIHNsb3dkb3du IGJ5IGludHJvZHVjaW5nIGEgY29uc3RydWN0b3IgdGhhdCBkb2Vzbid0IHVzZSBtb3ZlIHNlbWFu dGljcyBmb3IgdGhlIFJlZlB0cjxVbmlxdWVkU3RyaW5nSW1wbD4gcGFyYW1ldGVyCisgICAgICAg IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNjM0OTAKKworICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIHJ1bnRpbWUvSGFzT3du UHJvcGVydHlDYWNoZS5oOgorICAgICAgICAoSlNDOjpIYXNPd25Qcm9wZXJ0eUNhY2hlOjpFbnRy eTo6RW50cnkpOgorICAgICAgICAoSlNDOjpIYXNPd25Qcm9wZXJ0eUNhY2hlOjp0cnlBZGQpOgor CiAyMDE2LTEwLTE3ICBNYXJrIExhbSAgPG1hcmsubGFtQGFwcGxlLmNvbT4KIAogICAgICAgICBV c2UgdGhlIHJlamVjdCgpIGhlbHBlciBmdW5jdGlvbiBmb3IgY29uZGl0aW9uYWxseSB0aHJvd2lu ZyBUeXBlRXJyb3JzLgpJbmRleDogU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvSGFzT3du UHJvcGVydHlDYWNoZS5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50 aW1lL0hhc093blByb3BlcnR5Q2FjaGUuaAkocmV2aXNpb24gMjA3NDE3KQorKysgU291cmNlL0ph dmFTY3JpcHRDb3JlL3J1bnRpbWUvSGFzT3duUHJvcGVydHlDYWNoZS5oCSh3b3JraW5nIGNvcHkp CkBAIC00NCwxMiArNDQsMTEgQEAgcHVibGljOgogCiAgICAgICAgIEVudHJ5KCkgPSBkZWZhdWx0 OwogCi0gICAgICAgIEVudHJ5KFJlZlB0cjxVbmlxdWVkU3RyaW5nSW1wbD4gaW1wbCwgU3RydWN0 dXJlSUQgc3RydWN0dXJlSUQsIGJvb2wgcmVzdWx0KQotICAgICAgICAgICAgOiBpbXBsKGltcGwp CisgICAgICAgIEVudHJ5KFJlZlB0cjxVbmlxdWVkU3RyaW5nSW1wbD4mJiBpbXBsLCBTdHJ1Y3R1 cmVJRCBzdHJ1Y3R1cmVJRCwgYm9vbCByZXN1bHQpCisgICAgICAgICAgICA6IGltcGwoV1RGTW92 ZShpbXBsKSkKICAgICAgICAgICAgICwgc3RydWN0dXJlSUQoc3RydWN0dXJlSUQpCiAgICAgICAg ICAgICAsIHJlc3VsdChyZXN1bHQpCi0gICAgICAgIHsKLSAgICAgICAgfQorICAgICAgICB7IH0K IAogICAgICAgICBFbnRyeSYgb3BlcmF0b3I9KEVudHJ5JiYgb3RoZXIpCiAgICAgICAgIHsKQEAg LTEyMiw3ICsxMjEsNyBAQCBwdWJsaWM6CiAgICAgICAgICAgICBVbmlxdWVkU3RyaW5nSW1wbCog aW1wbCA9IHByb3BOYW1lLnVpZCgpOwogICAgICAgICAgICAgU3RydWN0dXJlSUQgaWQgPSBzdHJ1 Y3R1cmUtPmlkKCk7CiAgICAgICAgICAgICB1aW50MzJfdCBpbmRleCA9IEhhc093blByb3BlcnR5 Q2FjaGU6Omhhc2goaWQsIGltcGwpICYgbWFzazsKLSAgICAgICAgICAgIGJpdHdpc2VfY2FzdDxF bnRyeSo+KHRoaXMpW2luZGV4XSA9IEVudHJ5eyBSZWZQdHI8VW5pcXVlZFN0cmluZ0ltcGw+KGlt cGwpLCBpZCwgcmVzdWx0IH07CisgICAgICAgICAgICBiaXR3aXNlX2Nhc3Q8RW50cnkqPih0aGlz KVtpbmRleF0gPSBFbnRyeShSZWZQdHI8VW5pcXVlZFN0cmluZ0ltcGw+KGltcGwpLCBpZCwgcmVz dWx0KTsKICAgICAgICAgfQogICAgIH0KIAo= 291847 2016-10-17 11:42:07 -0700 2016-10-17 13:36:57 -0700 patch for landing a-backup.diff text/plain 1964 saam SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMjA3NDE3KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE0IEBA CisyMDE2LTEwLTE3ICBTYWFtIEJhcmF0aSAgPHNiYXJhdGlAYXBwbGUuY29tPgorCisgICAgICAg IEJ1aWxkIGZpeCBmb3IgSGFzT3duUHJvcGVydHlDYWNoZTo6RW50cnkgY2F1c2VkIHNsb3dkb3du IGJ5IGludHJvZHVjaW5nIGEgY29uc3RydWN0b3IgdGhhdCBkb2Vzbid0IHVzZSBtb3ZlIHNlbWFu dGljcyBmb3IgdGhlIFJlZlB0cjxVbmlxdWVkU3RyaW5nSW1wbD4gcGFyYW1ldGVyCisgICAgICAg IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNjM0OTAKKworICAgICAg ICBSZXZpZXdlZCBieSBEYXJpbiBBZGxlci4KKworICAgICAgICAqIHJ1bnRpbWUvSGFzT3duUHJv cGVydHlDYWNoZS5oOgorICAgICAgICAoSlNDOjpIYXNPd25Qcm9wZXJ0eUNhY2hlOjpFbnRyeTo6 RW50cnkpOgorICAgICAgICAoSlNDOjpIYXNPd25Qcm9wZXJ0eUNhY2hlOjp0cnlBZGQpOgorCiAy MDE2LTEwLTE3ICBNYXJrIExhbSAgPG1hcmsubGFtQGFwcGxlLmNvbT4KIAogICAgICAgICBVc2Ug dGhlIHJlamVjdCgpIGhlbHBlciBmdW5jdGlvbiBmb3IgY29uZGl0aW9uYWxseSB0aHJvd2luZyBU eXBlRXJyb3JzLgpJbmRleDogU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvSGFzT3duUHJv cGVydHlDYWNoZS5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1l L0hhc093blByb3BlcnR5Q2FjaGUuaAkocmV2aXNpb24gMjA3NDE3KQorKysgU291cmNlL0phdmFT Y3JpcHRDb3JlL3J1bnRpbWUvSGFzT3duUHJvcGVydHlDYWNoZS5oCSh3b3JraW5nIGNvcHkpCkBA IC00NCwxMiArNDQsMTEgQEAgcHVibGljOgogCiAgICAgICAgIEVudHJ5KCkgPSBkZWZhdWx0Owog Ci0gICAgICAgIEVudHJ5KFJlZlB0cjxVbmlxdWVkU3RyaW5nSW1wbD4gaW1wbCwgU3RydWN0dXJl SUQgc3RydWN0dXJlSUQsIGJvb2wgcmVzdWx0KQotICAgICAgICAgICAgOiBpbXBsKGltcGwpCisg ICAgICAgIEVudHJ5KFJlZlB0cjxVbmlxdWVkU3RyaW5nSW1wbD4mJiBpbXBsLCBTdHJ1Y3R1cmVJ RCBzdHJ1Y3R1cmVJRCwgYm9vbCByZXN1bHQpCisgICAgICAgICAgICA6IGltcGwoV1RGTW92ZShp bXBsKSkKICAgICAgICAgICAgICwgc3RydWN0dXJlSUQoc3RydWN0dXJlSUQpCiAgICAgICAgICAg ICAsIHJlc3VsdChyZXN1bHQpCi0gICAgICAgIHsKLSAgICAgICAgfQorICAgICAgICB7IH0KIAog ICAgICAgICBFbnRyeSYgb3BlcmF0b3I9KEVudHJ5JiYgb3RoZXIpCiAgICAgICAgIHsKQEAgLTEy Miw3ICsxMjEsNyBAQCBwdWJsaWM6CiAgICAgICAgICAgICBVbmlxdWVkU3RyaW5nSW1wbCogaW1w bCA9IHByb3BOYW1lLnVpZCgpOwogICAgICAgICAgICAgU3RydWN0dXJlSUQgaWQgPSBzdHJ1Y3R1 cmUtPmlkKCk7CiAgICAgICAgICAgICB1aW50MzJfdCBpbmRleCA9IEhhc093blByb3BlcnR5Q2Fj aGU6Omhhc2goaWQsIGltcGwpICYgbWFzazsKLSAgICAgICAgICAgIGJpdHdpc2VfY2FzdDxFbnRy eSo+KHRoaXMpW2luZGV4XSA9IEVudHJ5eyBSZWZQdHI8VW5pcXVlZFN0cmluZ0ltcGw+KGltcGwp LCBpZCwgcmVzdWx0IH07CisgICAgICAgICAgICBiaXR3aXNlX2Nhc3Q8RW50cnkqPih0aGlzKVtp bmRleF0gPSBFbnRyeSB7IFJlZlB0cjxVbmlxdWVkU3RyaW5nSW1wbD4oaW1wbCksIGlkLCByZXN1 bHQgfTsKICAgICAgICAgfQogICAgIH0KIAo=