64745 2011-07-18 13:18:35 -0700 JSC GC lazy sweep does not inline the common cases of cell destruction 2011-07-18 15:56:04 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 0 fpizlo webkit-unassigned fpizlo oliver webkit.review.bot oldest_to_newest 438658 0 fpizlo 2011-07-18 13:18:35 -0700 Most objects in the heap are instances of JSFinalObject, which has a relatively straight-forward destructor. This destructor's common case is to do nothing: the property storage is not inline, so there is nothing to destroy. However, the lazy sweep will currently always make a slow virtual call because it doesn't special-case the common cases. 438666 1 101197 fpizlo 2011-07-18 13:27:16 -0700 Created attachment 101197 the patch 438695 2 101197 oliver 2011-07-18 13:53:48 -0700 Comment on attachment 101197 the patch View in context: https://bugs.webkit.org/attachment.cgi?id=101197&action=review r- :( > Source/JavaScriptCore/heap/MarkedBlock.cpp:96 > + if (*reinterpret_cast<void**>(cell) == jsFinalObjectVPtr) { use cell->vptr(); > Source/JavaScriptCore/heap/MarkedBlock.cpp:99 > + if (!object->isUsingInlineStorage()) > + delete [] object->m_propertyStorage; object->JSFinalObject::~JSFinalObject() didn't work? 438698 3 fpizlo 2011-07-18 13:59:07 -0700 (In reply to comment #2) > (From update of attachment 101197 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=101197&action=review > > r- :( > > > Source/JavaScriptCore/heap/MarkedBlock.cpp:96 > > + if (*reinterpret_cast<void**>(cell) == jsFinalObjectVPtr) { > > use cell->vptr(); Ah, that's handy! > > Source/JavaScriptCore/heap/MarkedBlock.cpp:99 > > + if (!object->isUsingInlineStorage()) > > + delete [] object->m_propertyStorage; > > object->JSFinalObject::~JSFinalObject() didn't work? I tried object->~JSFinalObject() but that didn't give a speed-up. I forgot the object->JSFinalObject::~JSFinalObject syntax. That works great. Doing some quick tests and will resubmit shortly... 438729 4 101205 fpizlo 2011-07-18 14:33:43 -0700 Created attachment 101205 the patch (fix review) 438734 5 101205 darin 2011-07-18 14:41:07 -0700 Comment on attachment 101205 the patch (fix review) View in context: https://bugs.webkit.org/attachment.cgi?id=101205&action=review > Source/JavaScriptCore/heap/MarkedBlock.cpp:98 > + JSFinalObject* object = reinterpret_cast<JSFinalObject*>(cell); > + object->JSFinalObject::~JSFinalObject(); I would have done this as a one liner: reinterpret_cast<JSFinalObject*>(cell)->object->JSFinalObject::~JSFinalObject() I also think it deserves a “why” comment, stating this is a special case for speed. 438789 6 101205 webkit.review.bot 2011-07-18 15:56:00 -0700 Comment on attachment 101205 the patch (fix review) Clearing flags on attachment: 101205 Committed r91218: <http://trac.webkit.org/changeset/91218> 438790 7 webkit.review.bot 2011-07-18 15:56:04 -0700 All reviewed patches have been landed. Closing bug. 101197 2011-07-18 13:27:16 -0700 2011-07-18 14:33:43 -0700 the patch fastdestroy_patch_1.diff text/plain 2166 fpizlo SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gOTEyMDMpCisrKyBTb3VyY2Uv SmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTggQEAK KzIwMTEtMDctMTggIEZpbGlwIFBpemxvICA8ZnBpemxvQGFwcGxlLmNvbT4KKworICAgICAgICBK U0MgR0MgbGF6eSBzd2VlcCBkb2VzIG5vdCBpbmxpbmUgdGhlIGNvbW1vbiBjYXNlcyBvZiBjZWxs IGRlc3RydWN0aW9uLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9NjQ3NDUKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKyAgICAg ICAgCisgICAgICAgIFRoaXMgY2hhbmdlIGhhbmQtaW5saW5lcyB0aGUgY2FzZSBvZiBKU0ZpbmFs T2JqZWN0IGRlc3RydWN0aW9uLiAgQWx0aG91Z2ggdGhlCisgICAgICAgIEpTT2JqZWN0IGRlc3Ry dWN0b3IgaXMgaW5saW5lLCB0aGlzIHdhcyBoYW5kLWlubGluZWQgYW55d2F5IGJlY2F1c2UgY2Fs bGluZworICAgICAgICB0aGUgaW5saW5lIGRlc3RydWN0b3IgZGlkIG5vdCBhcHBlYXIgdG8gYmUg YXMgbXVjaCBvZiBhIHBlcmZvcm1hbmNlIHdpbi4KKyAgICAgICAgVGhpcyBwYXRjaCBpbXByb3Zl cyBWOCBhdmVyYWdlIHBlcmZvcm1hbmNlIGJ5IHJvdWdobHkgMS40JS4KKworICAgICAgICAqIGhl YXAvTWFya2VkQmxvY2suY3BwOgorICAgICAgICAoSlNDOjpNYXJrZWRCbG9jazo6bGF6eVN3ZWVw KToKKwogMjAxMS0wNy0xOCAgRmlsaXAgUGl6bG8gIDxmcGl6bG9AYXBwbGUuY29tPgogCiAgICAg ICAgIEpTQyBKSVQgZG9lcyBub3QgaW5saW5lIEdDIGFsbG9jYXRpb24gZmFzdCBwYXRocwpJbmRl eDogU291cmNlL0phdmFTY3JpcHRDb3JlL2hlYXAvTWFya2VkQmxvY2suY3BwCj09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0K LS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9oZWFwL01hcmtlZEJsb2NrLmNwcAkocmV2aXNpb24g OTEyMDEpCisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvaGVhcC9NYXJrZWRCbG9jay5jcHAJKHdv cmtpbmcgY29weSkKQEAgLTg2LDEyICs4NiwxOSBAQCBNYXJrZWRCbG9jazo6RnJlZUNlbGwqIE1h cmtlZEJsb2NrOjpsYXp5CiAgICAgLy8gVGhpcyBpcyBmaW5lLCBzaW5jZSB0aGUgYWxsb2NhdGlv biBjb2RlIG1ha2VzIG5vIGFzc3VtcHRpb25zIGFib3V0IHRoZQogICAgIC8vIG9yZGVyIG9mIHRo ZSBmcmVlIGxpc3QuCiAgICAgCisgICAgdm9pZCoganNGaW5hbE9iamVjdFZQdHIgPSBtX2hlYXAt Pmdsb2JhbERhdGEoKS0+anNGaW5hbE9iamVjdFZQdHI7CisgICAgCiAgICAgRnJlZUNlbGwqIHJl c3VsdCA9IDA7CiAgICAgCiAgICAgZm9yIChzaXplX3QgaSA9IGZpcnN0QXRvbSgpOyBpIDwgbV9l bmRBdG9tOyBpICs9IG1fYXRvbXNQZXJDZWxsKSB7CiAgICAgICAgIGlmICghbV9tYXJrcy50ZXN0 QW5kU2V0KGkpKSB7CiAgICAgICAgICAgICBKU0NlbGwqIGNlbGwgPSByZWludGVycHJldF9jYXN0 PEpTQ2VsbCo+KCZhdG9tcygpW2ldKTsKLSAgICAgICAgICAgIGNlbGwtPn5KU0NlbGwoKTsKKyAg ICAgICAgICAgIGlmICgqcmVpbnRlcnByZXRfY2FzdDx2b2lkKio+KGNlbGwpID09IGpzRmluYWxP YmplY3RWUHRyKSB7CisgICAgICAgICAgICAgICAgSlNGaW5hbE9iamVjdCogb2JqZWN0ID0gcmVp bnRlcnByZXRfY2FzdDxKU0ZpbmFsT2JqZWN0Kj4oY2VsbCk7CisgICAgICAgICAgICAgICAgaWYg KCFvYmplY3QtPmlzVXNpbmdJbmxpbmVTdG9yYWdlKCkpCisgICAgICAgICAgICAgICAgICAgIGRl bGV0ZSBbXSBvYmplY3QtPm1fcHJvcGVydHlTdG9yYWdlOworICAgICAgICAgICAgfSBlbHNlCisg ICAgICAgICAgICAgICAgY2VsbC0+fkpTQ2VsbCgpOwogICAgICAgICAgICAgRnJlZUNlbGwqIGZy ZWVDZWxsID0gcmVpbnRlcnByZXRfY2FzdDxGcmVlQ2VsbCo+KGNlbGwpOwogICAgICAgICAgICAg ZnJlZUNlbGwtPm5leHQgPSByZXN1bHQ7CiAgICAgICAgICAgICByZXN1bHQgPSBmcmVlQ2VsbDsK 101205 2011-07-18 14:33:43 -0700 2011-07-18 15:56:00 -0700 the patch (fix review) fastdestroy_patch_2.diff text/plain 1831 fpizlo SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gOTEyMDMpCisrKyBTb3VyY2Uv SmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTUgQEAK KzIwMTEtMDctMTggIEZpbGlwIFBpemxvICA8ZnBpemxvQGFwcGxlLmNvbT4KKworICAgICAgICBK U0MgR0MgbGF6eSBzd2VlcCBkb2VzIG5vdCBpbmxpbmUgdGhlIGNvbW1vbiBjYXNlcyBvZiBjZWxs IGRlc3RydWN0aW9uLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9NjQ3NDUKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKyAgICAg ICAgCisgICAgICAgIFRoaXMgaW5saW5lcyB0aGUgY2FzZSBvZiBKU0ZpbmFsT2JqZWN0IGRlc3Ry dWN0aW9uLgorCisgICAgICAgICogaGVhcC9NYXJrZWRCbG9jay5jcHA6CisgICAgICAgIChKU0M6 Ok1hcmtlZEJsb2NrOjpsYXp5U3dlZXApOgorCiAyMDExLTA3LTE4ICBGaWxpcCBQaXpsbyAgPGZw aXpsb0BhcHBsZS5jb20+CiAKICAgICAgICAgSlNDIEpJVCBkb2VzIG5vdCBpbmxpbmUgR0MgYWxs b2NhdGlvbiBmYXN0IHBhdGhzCkluZGV4OiBTb3VyY2UvSmF2YVNjcmlwdENvcmUvaGVhcC9NYXJr ZWRCbG9jay5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL0phdmFTY3JpcHRDb3JlL2hlYXAvTWFy a2VkQmxvY2suY3BwCShyZXZpc2lvbiA5MTIwMSkKKysrIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9o ZWFwL01hcmtlZEJsb2NrLmNwcAkod29ya2luZyBjb3B5KQpAQCAtODYsMTIgKzg2LDE4IEBAIE1h cmtlZEJsb2NrOjpGcmVlQ2VsbCogTWFya2VkQmxvY2s6OmxhenkKICAgICAvLyBUaGlzIGlzIGZp bmUsIHNpbmNlIHRoZSBhbGxvY2F0aW9uIGNvZGUgbWFrZXMgbm8gYXNzdW1wdGlvbnMgYWJvdXQg dGhlCiAgICAgLy8gb3JkZXIgb2YgdGhlIGZyZWUgbGlzdC4KICAgICAKKyAgICB2b2lkKiBqc0Zp bmFsT2JqZWN0VlB0ciA9IG1faGVhcC0+Z2xvYmFsRGF0YSgpLT5qc0ZpbmFsT2JqZWN0VlB0cjsK KyAgICAKICAgICBGcmVlQ2VsbCogcmVzdWx0ID0gMDsKICAgICAKICAgICBmb3IgKHNpemVfdCBp ID0gZmlyc3RBdG9tKCk7IGkgPCBtX2VuZEF0b207IGkgKz0gbV9hdG9tc1BlckNlbGwpIHsKICAg ICAgICAgaWYgKCFtX21hcmtzLnRlc3RBbmRTZXQoaSkpIHsKICAgICAgICAgICAgIEpTQ2VsbCog Y2VsbCA9IHJlaW50ZXJwcmV0X2Nhc3Q8SlNDZWxsKj4oJmF0b21zKClbaV0pOwotICAgICAgICAg ICAgY2VsbC0+fkpTQ2VsbCgpOworICAgICAgICAgICAgaWYgKGNlbGwtPnZwdHIoKSA9PSBqc0Zp bmFsT2JqZWN0VlB0cikgeworICAgICAgICAgICAgICAgIEpTRmluYWxPYmplY3QqIG9iamVjdCA9 IHJlaW50ZXJwcmV0X2Nhc3Q8SlNGaW5hbE9iamVjdCo+KGNlbGwpOworICAgICAgICAgICAgICAg IG9iamVjdC0+SlNGaW5hbE9iamVjdDo6fkpTRmluYWxPYmplY3QoKTsKKyAgICAgICAgICAgIH0g ZWxzZQorICAgICAgICAgICAgICAgIGNlbGwtPn5KU0NlbGwoKTsKICAgICAgICAgICAgIEZyZWVD ZWxsKiBmcmVlQ2VsbCA9IHJlaW50ZXJwcmV0X2Nhc3Q8RnJlZUNlbGwqPihjZWxsKTsKICAgICAg ICAgICAgIGZyZWVDZWxsLT5uZXh0ID0gcmVzdWx0OwogICAgICAgICAgICAgcmVzdWx0ID0gZnJl ZUNlbGw7Cg==