136340 2014-08-28 06:42:39 -0700 GC length unit is invalid 2014-08-28 07:24:20 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 jbriance webkit-unassigned ggaren mhahnenb msaboff oldest_to_newest 1031975 0 jbriance 2014-08-28 06:42:39 -0700 In showObjectStatistics, GC length unit is "s" and not "ms". Also, division by zero may occur when showObjectStatistics is called without any object stored in heap. 1031976 1 237309 jbriance 2014-08-28 06:44:27 -0700 Created attachment 237309 Correct GC length unit and prevent div by 0 in showObjectStatistics 1031980 2 237309 mhahnenb 2014-08-28 06:55:05 -0700 Comment on attachment 237309 Correct GC length unit and prevent div by 0 in showObjectStatistics View in context: https://bugs.webkit.org/attachment.cgi?id=237309&action=review This looks like a good change, but I think we can make it even better and be good citizens in the process :-) > Source/JavaScriptCore/heap/HeapStatistics.cpp:256 > + if ((storageStatistics.storageCapacity() > 0) && (storageStatistics.objectCount() > 0)) { > + dataLogF("wasted .property storage: %ldkB (%ld%%)\n", > + static_cast<long>( > + (storageStatistics.storageCapacity() - storageStatistics.storageSize()) / KB), > + static_cast<long>( > + (storageStatistics.storageCapacity() - storageStatistics.storageSize()) * 100 > + / storageStatistics.storageCapacity())); > + dataLogF("objects with out-of-line .property storage: %ld (%ld%%)\n", > + static_cast<long>( > + storageStatistics.objectWithOutOfLineStorageCount()), > + static_cast<long>( > + storageStatistics.objectWithOutOfLineStorageCount() * 100 > + / storageStatistics.objectCount())); > + } This is very weirdly formatted code. Maybe we could clean it up a little since we're already here. I'd suggest computing the wasted property storage and storing it in a local variable, then printing it out in a normal looking dataLogF below. You can use control flow to avoid the div-by-zero issue when computing that value. Ditto for objects w/ out-of-line property storage. 1031981 3 mhahnenb 2014-08-28 06:59:36 -0700 To be a little clearer with what I mean: long wastedPropertyStorageBytes = 0; long wastedPropertyStoragePercent = 0; long objectsWithOutOfLineStoragePercent = 0; if (/* we won't div-by-zero */) wastedPropertyStorageBytes = /* weird casts and computation here */; wastedPropertyStoragePercent = /* more weird casts and computation here */; objectsWithOutOfLineStoragePercent = /* even more weird casts and computation here */; } dataLogF("waste property storage: ....", wastedPropertyStorageBytes, wastedPropertyStoragePercent); dataLogF(...); 1031982 4 237310 jbriance 2014-08-28 07:19:02 -0700 Created attachment 237310 Correct GC length unit and prevent div by 0 in showObjectStatistics (v2) 1031983 5 237310 mhahnenb 2014-08-28 07:20:18 -0700 Comment on attachment 237310 Correct GC length unit and prevent div by 0 in showObjectStatistics (v2) Awesome, looks great. r=me 1031984 6 jbriance 2014-08-28 07:24:20 -0700 Manually committed r173062: http://trac.webkit.org/changeset/173062 237309 2014-08-28 06:44:27 -0700 2014-08-28 07:19:02 -0700 Correct GC length unit and prevent div by 0 in showObjectStatistics bug-136340.patch text/plain 3124 jbriance SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTczMDYxKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDEzIEBA CisyMDE0LTA4LTI4ICBKdWxpZW4gQnJpYW5jZWF1ICAgPGpicmlhbmNlQGNpc2NvLmNvbT4KKwor ICAgICAgICBDb3JyZWN0IEdDIGxlbmd0aCB1bml0IGFuZCBwcmV2ZW50IGRpdmlzaW9uIGJ5IDAg aW4gc2hvd09iamVjdFN0YXRpc3RpY3MuCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD0xMzYzNDAKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICAqIGhlYXAvSGVhcFN0YXRpc3RpY3MuY3BwOgorICAgICAgICAoSlND OjpIZWFwU3RhdGlzdGljczo6c2hvd09iamVjdFN0YXRpc3RpY3MpOgorCiAyMDE0LTA4LTI3ICBB a29zIEtpc3MgIDxha2lzc0BpbmYudS1zemVnZWQuaHU+CiAKICAgICAgICAgRW5zdXJlIHRoYXQg dGhlIGNhbGwgZnJhbWUgcGFzc2VkIGZyb20gSklUIGNvZGUgdmlhIEpTQzo6b3BlcmF0aW9uQ2Fs bEV2YWwgdG8gSlNDOjpldmFsIGFsd2F5cyBjb250YWlucyB0aGUgdmFsaWQgc2NvcGUgY2hhaW4u CkluZGV4OiBTb3VyY2UvSmF2YVNjcmlwdENvcmUvaGVhcC9IZWFwU3RhdGlzdGljcy5jcHAKPT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PQotLS0gU291cmNlL0phdmFTY3JpcHRDb3JlL2hlYXAvSGVhcFN0YXRpc3RpY3MuY3Bw CShyZXZpc2lvbiAxNzMwNTkpCisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvaGVhcC9IZWFwU3Rh dGlzdGljcy5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTIzMywyNSArMjMzLDI3IEBAIHZvaWQgSGVh cFN0YXRpc3RpY3M6OnNob3dPYmplY3RTdGF0aXN0aWMKICAgICBkYXRhTG9nRigiXG49PT0gSGVh cCBTdGF0aXN0aWNzOiA9PT1cbiIpOwogICAgIGRhdGFMb2dGKCJzaXplOiAlbGRrQlxuIiwgc3Rh dGljX2Nhc3Q8bG9uZz4oaGVhcC0+bV9zaXplQWZ0ZXJMYXN0Q29sbGVjdCAvIEtCKSk7CiAgICAg ZGF0YUxvZ0YoImNhcGFjaXR5OiAlbGRrQlxuIiwgc3RhdGljX2Nhc3Q8bG9uZz4oaGVhcC0+Y2Fw YWNpdHkoKSAvIEtCKSk7Ci0gICAgZGF0YUxvZ0YoInBhdXNlIHRpbWU6ICVsZm1zXG5cbiIsIGhl YXAtPm1fbGFzdEZ1bGxHQ0xlbmd0aCk7CisgICAgZGF0YUxvZ0YoInBhdXNlIHRpbWU6ICVsZnNc blxuIiwgaGVhcC0+bV9sYXN0RnVsbEdDTGVuZ3RoKTsKIAogICAgIFN0b3JhZ2VTdGF0aXN0aWNz IHN0b3JhZ2VTdGF0aXN0aWNzOwogICAgIHsKICAgICAgICAgSGVhcEl0ZXJhdGlvblNjb3BlIGl0 ZXJhdGlvblNjb3BlKCpoZWFwKTsKICAgICAgICAgaGVhcC0+bV9vYmplY3RTcGFjZS5mb3JFYWNo TGl2ZUNlbGwoaXRlcmF0aW9uU2NvcGUsIHN0b3JhZ2VTdGF0aXN0aWNzKTsKICAgICB9Ci0gICAg ZGF0YUxvZ0YoIndhc3RlZCAucHJvcGVydHkgc3RvcmFnZTogJWxka0IgKCVsZCUlKVxuIiwKLSAg ICAgICAgc3RhdGljX2Nhc3Q8bG9uZz4oCi0gICAgICAgICAgICAoc3RvcmFnZVN0YXRpc3RpY3Mu c3RvcmFnZUNhcGFjaXR5KCkgLSBzdG9yYWdlU3RhdGlzdGljcy5zdG9yYWdlU2l6ZSgpKSAvIEtC KSwKLSAgICAgICAgc3RhdGljX2Nhc3Q8bG9uZz4oCi0gICAgICAgICAgICAoc3RvcmFnZVN0YXRp c3RpY3Muc3RvcmFnZUNhcGFjaXR5KCkgLSBzdG9yYWdlU3RhdGlzdGljcy5zdG9yYWdlU2l6ZSgp KSAqIDEwMAotICAgICAgICAgICAgICAgIC8gc3RvcmFnZVN0YXRpc3RpY3Muc3RvcmFnZUNhcGFj aXR5KCkpKTsKLSAgICBkYXRhTG9nRigib2JqZWN0cyB3aXRoIG91dC1vZi1saW5lIC5wcm9wZXJ0 eSBzdG9yYWdlOiAlbGQgKCVsZCUlKVxuIiwKLSAgICAgICAgc3RhdGljX2Nhc3Q8bG9uZz4oCi0g ICAgICAgICAgICBzdG9yYWdlU3RhdGlzdGljcy5vYmplY3RXaXRoT3V0T2ZMaW5lU3RvcmFnZUNv dW50KCkpLAotICAgICAgICBzdGF0aWNfY2FzdDxsb25nPigKLSAgICAgICAgICAgIHN0b3JhZ2VT dGF0aXN0aWNzLm9iamVjdFdpdGhPdXRPZkxpbmVTdG9yYWdlQ291bnQoKSAqIDEwMAotICAgICAg ICAgICAgICAgIC8gc3RvcmFnZVN0YXRpc3RpY3Mub2JqZWN0Q291bnQoKSkpOworICAgIGlmICgo c3RvcmFnZVN0YXRpc3RpY3Muc3RvcmFnZUNhcGFjaXR5KCkgPiAwKSAmJiAoc3RvcmFnZVN0YXRp c3RpY3Mub2JqZWN0Q291bnQoKSA+IDApKSB7CisgICAgICAgIGRhdGFMb2dGKCJ3YXN0ZWQgLnBy b3BlcnR5IHN0b3JhZ2U6ICVsZGtCICglbGQlJSlcbiIsCisgICAgICAgICAgICBzdGF0aWNfY2Fz dDxsb25nPigKKyAgICAgICAgICAgICAgICAoc3RvcmFnZVN0YXRpc3RpY3Muc3RvcmFnZUNhcGFj aXR5KCkgLSBzdG9yYWdlU3RhdGlzdGljcy5zdG9yYWdlU2l6ZSgpKSAvIEtCKSwKKyAgICAgICAg ICAgIHN0YXRpY19jYXN0PGxvbmc+KAorICAgICAgICAgICAgICAgIChzdG9yYWdlU3RhdGlzdGlj cy5zdG9yYWdlQ2FwYWNpdHkoKSAtIHN0b3JhZ2VTdGF0aXN0aWNzLnN0b3JhZ2VTaXplKCkpICog MTAwCisgICAgICAgICAgICAgICAgICAgIC8gc3RvcmFnZVN0YXRpc3RpY3Muc3RvcmFnZUNhcGFj aXR5KCkpKTsKKyAgICAgICAgZGF0YUxvZ0YoIm9iamVjdHMgd2l0aCBvdXQtb2YtbGluZSAucHJv cGVydHkgc3RvcmFnZTogJWxkICglbGQlJSlcbiIsCisgICAgICAgICAgICBzdGF0aWNfY2FzdDxs b25nPigKKyAgICAgICAgICAgICAgICBzdG9yYWdlU3RhdGlzdGljcy5vYmplY3RXaXRoT3V0T2ZM aW5lU3RvcmFnZUNvdW50KCkpLAorICAgICAgICAgICAgc3RhdGljX2Nhc3Q8bG9uZz4oCisgICAg ICAgICAgICAgICAgc3RvcmFnZVN0YXRpc3RpY3Mub2JqZWN0V2l0aE91dE9mTGluZVN0b3JhZ2VD b3VudCgpICogMTAwCisgICAgICAgICAgICAgICAgICAgIC8gc3RvcmFnZVN0YXRpc3RpY3Mub2Jq ZWN0Q291bnQoKSkpOworICAgIH0KIH0KIAogfSAvLyBuYW1lc3BhY2UgSlNDCg== 237310 2014-08-28 07:19:02 -0700 2014-08-28 07:20:18 -0700 Correct GC length unit and prevent div by 0 in showObjectStatistics (v2) bug-136340_2.patch text/plain 3402 jbriance SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTczMDU5KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDEzIEBA CisyMDE0LTA4LTI4ICBKdWxpZW4gQnJpYW5jZWF1ICAgPGpicmlhbmNlQGNpc2NvLmNvbT4KKwor ICAgICAgICBDb3JyZWN0IEdDIGxlbmd0aCB1bml0IGFuZCBwcmV2ZW50IGRpdmlzaW9uIGJ5IDAg aW4gc2hvd09iamVjdFN0YXRpc3RpY3MuCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD0xMzYzNDAKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICAqIGhlYXAvSGVhcFN0YXRpc3RpY3MuY3BwOgorICAgICAgICAoSlND OjpIZWFwU3RhdGlzdGljczo6c2hvd09iamVjdFN0YXRpc3RpY3MpOgorCiAyMDE0LTA4LTI3ICBB a29zIEtpc3MgIDxha2lzc0BpbmYudS1zemVnZWQuaHU+CiAKICAgICAgICAgRW5zdXJlIHRoYXQg dGhlIGNhbGwgZnJhbWUgcGFzc2VkIGZyb20gSklUIGNvZGUgdmlhIEpTQzo6b3BlcmF0aW9uQ2Fs bEV2YWwgdG8gSlNDOjpldmFsIGFsd2F5cyBjb250YWlucyB0aGUgdmFsaWQgc2NvcGUgY2hhaW4u CkluZGV4OiBTb3VyY2UvSmF2YVNjcmlwdENvcmUvaGVhcC9IZWFwU3RhdGlzdGljcy5jcHAKPT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PQotLS0gU291cmNlL0phdmFTY3JpcHRDb3JlL2hlYXAvSGVhcFN0YXRpc3RpY3MuY3Bw CShyZXZpc2lvbiAxNzMwNTkpCisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvaGVhcC9IZWFwU3Rh dGlzdGljcy5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTIzMywyNSArMjMzLDI2IEBAIHZvaWQgSGVh cFN0YXRpc3RpY3M6OnNob3dPYmplY3RTdGF0aXN0aWMKICAgICBkYXRhTG9nRigiXG49PT0gSGVh cCBTdGF0aXN0aWNzOiA9PT1cbiIpOwogICAgIGRhdGFMb2dGKCJzaXplOiAlbGRrQlxuIiwgc3Rh dGljX2Nhc3Q8bG9uZz4oaGVhcC0+bV9zaXplQWZ0ZXJMYXN0Q29sbGVjdCAvIEtCKSk7CiAgICAg ZGF0YUxvZ0YoImNhcGFjaXR5OiAlbGRrQlxuIiwgc3RhdGljX2Nhc3Q8bG9uZz4oaGVhcC0+Y2Fw YWNpdHkoKSAvIEtCKSk7Ci0gICAgZGF0YUxvZ0YoInBhdXNlIHRpbWU6ICVsZm1zXG5cbiIsIGhl YXAtPm1fbGFzdEZ1bGxHQ0xlbmd0aCk7CisgICAgZGF0YUxvZ0YoInBhdXNlIHRpbWU6ICVsZnNc blxuIiwgaGVhcC0+bV9sYXN0RnVsbEdDTGVuZ3RoKTsKIAogICAgIFN0b3JhZ2VTdGF0aXN0aWNz IHN0b3JhZ2VTdGF0aXN0aWNzOwogICAgIHsKICAgICAgICAgSGVhcEl0ZXJhdGlvblNjb3BlIGl0 ZXJhdGlvblNjb3BlKCpoZWFwKTsKICAgICAgICAgaGVhcC0+bV9vYmplY3RTcGFjZS5mb3JFYWNo TGl2ZUNlbGwoaXRlcmF0aW9uU2NvcGUsIHN0b3JhZ2VTdGF0aXN0aWNzKTsKICAgICB9Ci0gICAg ZGF0YUxvZ0YoIndhc3RlZCAucHJvcGVydHkgc3RvcmFnZTogJWxka0IgKCVsZCUlKVxuIiwKLSAg ICAgICAgc3RhdGljX2Nhc3Q8bG9uZz4oCi0gICAgICAgICAgICAoc3RvcmFnZVN0YXRpc3RpY3Mu c3RvcmFnZUNhcGFjaXR5KCkgLSBzdG9yYWdlU3RhdGlzdGljcy5zdG9yYWdlU2l6ZSgpKSAvIEtC KSwKLSAgICAgICAgc3RhdGljX2Nhc3Q8bG9uZz4oCi0gICAgICAgICAgICAoc3RvcmFnZVN0YXRp c3RpY3Muc3RvcmFnZUNhcGFjaXR5KCkgLSBzdG9yYWdlU3RhdGlzdGljcy5zdG9yYWdlU2l6ZSgp KSAqIDEwMAotICAgICAgICAgICAgICAgIC8gc3RvcmFnZVN0YXRpc3RpY3Muc3RvcmFnZUNhcGFj aXR5KCkpKTsKLSAgICBkYXRhTG9nRigib2JqZWN0cyB3aXRoIG91dC1vZi1saW5lIC5wcm9wZXJ0 eSBzdG9yYWdlOiAlbGQgKCVsZCUlKVxuIiwKLSAgICAgICAgc3RhdGljX2Nhc3Q8bG9uZz4oCi0g ICAgICAgICAgICBzdG9yYWdlU3RhdGlzdGljcy5vYmplY3RXaXRoT3V0T2ZMaW5lU3RvcmFnZUNv dW50KCkpLAotICAgICAgICBzdGF0aWNfY2FzdDxsb25nPigKLSAgICAgICAgICAgIHN0b3JhZ2VT dGF0aXN0aWNzLm9iamVjdFdpdGhPdXRPZkxpbmVTdG9yYWdlQ291bnQoKSAqIDEwMAotICAgICAg ICAgICAgICAgIC8gc3RvcmFnZVN0YXRpc3RpY3Mub2JqZWN0Q291bnQoKSkpOworICAgIGxvbmcg d2FzdGVkUHJvcGVydHlTdG9yYWdlQnl0ZXMgPSAwOworICAgIGxvbmcgd2FzdGVkUHJvcGVydHlT dG9yYWdlUGVyY2VudCA9IDA7CisgICAgbG9uZyBvYmplY3RXaXRoT3V0T2ZMaW5lU3RvcmFnZUNv dW50ID0gMDsKKyAgICBsb25nIG9iamVjdHNXaXRoT3V0T2ZMaW5lU3RvcmFnZVBlcmNlbnQgPSAw OworICAgIGlmICgoc3RvcmFnZVN0YXRpc3RpY3Muc3RvcmFnZUNhcGFjaXR5KCkgPiAwKSAmJiAo c3RvcmFnZVN0YXRpc3RpY3Mub2JqZWN0Q291bnQoKSA+IDApKSB7CisgICAgICAgIHdhc3RlZFBy b3BlcnR5U3RvcmFnZUJ5dGVzID0gc3RhdGljX2Nhc3Q8bG9uZz4oKHN0b3JhZ2VTdGF0aXN0aWNz LnN0b3JhZ2VDYXBhY2l0eSgpIC0gc3RvcmFnZVN0YXRpc3RpY3Muc3RvcmFnZVNpemUoKSkgLyBL Qik7CisgICAgICAgIHdhc3RlZFByb3BlcnR5U3RvcmFnZVBlcmNlbnQgPSBzdGF0aWNfY2FzdDxs b25nPigKKyAgICAgICAgICAgIChzdG9yYWdlU3RhdGlzdGljcy5zdG9yYWdlQ2FwYWNpdHkoKSAt IHN0b3JhZ2VTdGF0aXN0aWNzLnN0b3JhZ2VTaXplKCkpICogMTAwIC8gc3RvcmFnZVN0YXRpc3Rp Y3Muc3RvcmFnZUNhcGFjaXR5KCkpOworICAgICAgICBvYmplY3RXaXRoT3V0T2ZMaW5lU3RvcmFn ZUNvdW50ID0gc3RhdGljX2Nhc3Q8bG9uZz4oc3RvcmFnZVN0YXRpc3RpY3Mub2JqZWN0V2l0aE91 dE9mTGluZVN0b3JhZ2VDb3VudCgpKTsKKyAgICAgICAgb2JqZWN0c1dpdGhPdXRPZkxpbmVTdG9y YWdlUGVyY2VudCA9IG9iamVjdFdpdGhPdXRPZkxpbmVTdG9yYWdlQ291bnQgKiAxMDAgLyBzdG9y YWdlU3RhdGlzdGljcy5vYmplY3RDb3VudCgpOworICAgIH0KKyAgICBkYXRhTG9nRigid2FzdGVk IC5wcm9wZXJ0eSBzdG9yYWdlOiAlbGRrQiAoJWxkJSUpXG4iLCB3YXN0ZWRQcm9wZXJ0eVN0b3Jh Z2VCeXRlcywgd2FzdGVkUHJvcGVydHlTdG9yYWdlUGVyY2VudCk7CisgICAgZGF0YUxvZ0YoIm9i amVjdHMgd2l0aCBvdXQtb2YtbGluZSAucHJvcGVydHkgc3RvcmFnZTogJWxkICglbGQlJSlcbiIs IG9iamVjdFdpdGhPdXRPZkxpbmVTdG9yYWdlQ291bnQsIG9iamVjdHNXaXRoT3V0T2ZMaW5lU3Rv cmFnZVBlcmNlbnQpOwogfQogCiB9IC8vIG5hbWVzcGFjZSBKU0MK