120615 2013-09-02 21:51:19 -0700 CodeBlock memory cost reporting should be rationalized 2013-09-03 22:47:31 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 120567 1 fpizlo fpizlo barraclough benjamin cmarcelo commit-queue ggaren mark.lam mhahnenberg msaboff oliver sam oldest_to_newest 923842 0 fpizlo 2013-09-02 21:51:19 -0700 CodeBlock should report the size of the bytecode as part of extra memory cost. Also, CodeBlock should report the extra memory usage during GC as well so we don't forget that it's using memory. 924198 1 210415 fpizlo 2013-09-03 15:26:43 -0700 Created attachment 210415 the patch 924245 2 210415 darin 2013-09-03 19:18:41 -0700 Comment on attachment 210415 the patch View in context: https://bugs.webkit.org/attachment.cgi?id=210415&action=review > Source/JavaScriptCore/bytecode/CodeBlock.cpp:1851 > + m_heap->reportExtraMemoryCost( > + sizeof(CodeBlock) + m_instructions.size() * sizeof(Instruction)); Why the strange line breaking? Looks like this all would fit on one line just fine. > Source/JavaScriptCore/bytecode/CodeBlock.cpp:1940 > + visitor.reportExtraMemoryUsage( > + m_instructions.size() * sizeof(Instruction) / m_instructions.refCount()); Same line breaking comment. Also, the division by reference count is sufficiently non-obvious that I would include a comment, unless this is a common JavaScriptCore idiom that I am unaware of. 924282 3 fpizlo 2013-09-03 22:32:14 -0700 (In reply to comment #2) > (From update of attachment 210415 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=210415&action=review > > > Source/JavaScriptCore/bytecode/CodeBlock.cpp:1851 > > + m_heap->reportExtraMemoryCost( > > + sizeof(CodeBlock) + m_instructions.size() * sizeof(Instruction)); > > Why the strange line breaking? Looks like this all would fit on one line just fine. Heh, you're probably right. I don't like super long lines. But I decide this rather arbitrarily - it depends on how wide my emacs window is at the time. I'll un-wrap these. > > > Source/JavaScriptCore/bytecode/CodeBlock.cpp:1940 > > + visitor.reportExtraMemoryUsage( > > + m_instructions.size() * sizeof(Instruction) / m_instructions.refCount()); > > Same line breaking comment. Also, the division by reference count is sufficiently non-obvious that I would include a comment, unless this is a common JavaScriptCore idiom that I am unaware of. It's pseudo-common. The idea is that the m_instructions is a shared array that is owned by multiple CodeBlocks. Each CodeBlock gets traced by GC, and each one will make a reportExtraMemoryUsage() call. But we only want to count the size of the instructions once. The simplest way of doing that is to have each of them do the reporting but divide by the refCount. We do this in one other place - accounting for memory used by JSStrings. There we do something like this - take the StringImpl's size and divide by refCount. Plus some other cruft to take into account immortal strings... A comment could probably help. It's not super obvious that m_instructions is a shared thing. 924287 4 fpizlo 2013-09-03 22:47:31 -0700 Landed in http://trac.webkit.org/changeset/155021 210415 2013-09-03 15:26:43 -0700 2013-09-03 19:18:41 -0700 the patch blah.patch text/plain 3510 fpizlo SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTU1MDAzKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIzIEBA CisyMDEzLTA5LTAzICBGaWxpcCBQaXpsbyAgPGZwaXpsb0BhcHBsZS5jb20+CisKKyAgICAgICAg Q29kZUJsb2NrIG1lbW9yeSBjb3N0IHJlcG9ydGluZyBzaG91bGQgYmUgcmF0aW9uYWxpemVkCisg ICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMjA2MTUKKwor ICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKyAgICAgICAgCisgICAgICAgIFJl cG9ydCB0aGUgc2l6ZSBvZiB0aGUgaW5zdHJ1Y3Rpb24gc3RyZWFtLCBhbmQgdGhlbiByZW1pbmQg dGhlIEdDIHRoYXQgd2UncmUKKyAgICAgICAgdXNpbmcgbWVtb3J5IHdoZW4gd2UgdHJhY2UuCisg ICAgICAgIAorICAgICAgICBUaGlzIGlzIGEgc2xpZ2h0IHNsb3ctZG93biBvbiBzb21lIEpTQmVu Y2ggdGVzdHMgYmVjYXVzZSBpdCBtYWtlcyB1cyBHQyBhCisgICAgICAgIGJpdCBtb3JlIGZyZXF1 ZW50bHkuIEJ1dCBJIHRoaW5rIGl0J3Mgd2VsbCB3b3J0aCBpdDsgaWYgd2UgcmVhbGx5IHdhbnQg dGhvc2UKKyAgICAgICAgdGVzdHMgdG8gR0MgbGVzcyBmcmVxdWVudGx5IHRoZW4gd2UgY2FuIGFj aGlldmUgdGhhdCB0aHJvdWdoIG90aGVyIGtpbmRzIG9mCisgICAgICAgIHR1bmluZy4gSXQncyBi ZXR0ZXIgdGhhdCB0aGUgR0Mga25vd3MgdGhhdCBDb2RlQmxvY2tzIGRvIGluIGZhY3QgdXNlIG1l bW9yeTsKKyAgICAgICAgd2hhdCBpdCBkb2VzIHdpdGggdGhhdCBpbmZvcm1hdGlvbiBpcyBhIHNv bWV3aGF0IG9ydGhvZ29uYWwgcXVlc3Rpb24uCisKKyAgICAgICAgKiBieXRlY29kZS9Db2RlQmxv Y2suY3BwOgorICAgICAgICAoSlNDOjpDb2RlQmxvY2s6OkNvZGVCbG9jayk6CisgICAgICAgIChK U0M6OkNvZGVCbG9jazo6dmlzaXRBZ2dyZWdhdGUpOgorCiAyMDEzLTA5LTAyICBSeW9zdWtlIE5p d2EgIDxybml3YUB3ZWJraXQub3JnPgogCiAgICAgICAgIFN1cHBvcnQgdGhlICJqc29uIiByZXNw b25zZVR5cGUgYW5kIEpTT04gcmVzcG9uc2UgZW50aXR5IGluIFhIUgpJbmRleDogU291cmNlL0ph dmFTY3JpcHRDb3JlL2J5dGVjb2RlL0NvZGVCbG9jay5jcHAKPT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNl L0phdmFTY3JpcHRDb3JlL2J5dGVjb2RlL0NvZGVCbG9jay5jcHAJKHJldmlzaW9uIDE1NTAwMykK KysrIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9ieXRlY29kZS9Db2RlQmxvY2suY3BwCSh3b3JraW5n IGNvcHkpCkBAIC0xODQ3LDcgKzE4NDcsOCBAQCBDb2RlQmxvY2s6OkNvZGVCbG9jayhTY3JpcHRF eGVjdXRhYmxlKiBvCiAgICAgaWYgKE9wdGlvbnM6OmR1bXBHZW5lcmF0ZWRCeXRlY29kZXMoKSkK ICAgICAgICAgZHVtcEJ5dGVjb2RlKCk7CiAgICAgbV9oZWFwLT5tX2NvZGVCbG9ja3MuYWRkKHRo aXMpOwotICAgIG1faGVhcC0+cmVwb3J0RXh0cmFNZW1vcnlDb3N0KHNpemVvZihDb2RlQmxvY2sp KTsKKyAgICBtX2hlYXAtPnJlcG9ydEV4dHJhTWVtb3J5Q29zdCgKKyAgICAgICAgc2l6ZW9mKENv ZGVCbG9jaykgKyBtX2luc3RydWN0aW9ucy5zaXplKCkgKiBzaXplb2YoSW5zdHJ1Y3Rpb24pKTsK IH0KIAogQ29kZUJsb2NrOjp+Q29kZUJsb2NrKCkKQEAgLTE5MzEsNiArMTkzMiwxNCBAQCB2b2lk IENvZGVCbG9jazo6dmlzaXRBZ2dyZWdhdGUoU2xvdFZpc2l0CiAgICAgaWYgKCEhbV9hbHRlcm5h dGl2ZSkKICAgICAgICAgbV9hbHRlcm5hdGl2ZS0+dmlzaXRBZ2dyZWdhdGUodmlzaXRvcik7CiAK KyAgICB2aXNpdG9yLnJlcG9ydEV4dHJhTWVtb3J5VXNhZ2Uoc2l6ZW9mKENvZGVCbG9jaykpOwor ICAgIGlmIChtX2ppdENvZGUpCisgICAgICAgIHZpc2l0b3IucmVwb3J0RXh0cmFNZW1vcnlVc2Fn ZShtX2ppdENvZGUtPnNpemUoKSk7CisgICAgaWYgKG1faW5zdHJ1Y3Rpb25zLnNpemUoKSkgewor ICAgICAgICB2aXNpdG9yLnJlcG9ydEV4dHJhTWVtb3J5VXNhZ2UoCisgICAgICAgICAgICBtX2lu c3RydWN0aW9ucy5zaXplKCkgKiBzaXplb2YoSW5zdHJ1Y3Rpb24pIC8gbV9pbnN0cnVjdGlvbnMu cmVmQ291bnQoKSk7CisgICAgfQorCiAgICAgdmlzaXRvci5hcHBlbmQoJm1fdW5saW5rZWRDb2Rl KTsKIAogICAgIC8vIFRoZXJlIGFyZSB0aHJlZSB0aGluZ3MgdGhhdCBtYXkgdXNlIHVuY29uZGl0 aW9uYWwgZmluYWxpemVyczogbGF6eSBieXRlY29kZSBmcmVlaW5nLApJbmRleDogU291cmNlL1dU Ri9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dURi9DaGFuZ2VMb2cJKHJldmlzaW9u IDE1NTAwMykKKysrIFNvdXJjZS9XVEYvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMg KzEsMTMgQEAKKzIwMTMtMDktMDMgIEZpbGlwIFBpemxvICA8ZnBpemxvQGFwcGxlLmNvbT4KKwor ICAgICAgICBDb2RlQmxvY2sgbWVtb3J5IGNvc3QgcmVwb3J0aW5nIHNob3VsZCBiZSByYXRpb25h bGl6ZWQKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEy MDYxNQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICog d3RmL1JlZkNvdW50ZWRBcnJheS5oOgorICAgICAgICAoV1RGOjpSZWZDb3VudGVkQXJyYXk6OnJl ZkNvdW50KToKKwogMjAxMy0wOS0wMyAgQW5kcmVhcyBLbGluZyAgPGFrbGluZ0BhcHBsZS5jb20+ CiAKICAgICAgICAgU3VwcG9ydCBWZWN0b3I8UmVmPFQ+Pi4KSW5kZXg6IFNvdXJjZS9XVEYvd3Rm L1JlZkNvdW50ZWRBcnJheS5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XVEYvd3RmL1JlZkNvdW50 ZWRBcnJheS5oCShyZXZpc2lvbiAxNTUwMDMpCisrKyBTb3VyY2UvV1RGL3d0Zi9SZWZDb3VudGVk QXJyYXkuaAkod29ya2luZyBjb3B5KQpAQCAtMTEyLDYgKzExMiwxMyBAQCBwdWJsaWM6CiAgICAg ICAgIGZhc3RGcmVlKEhlYWRlcjo6ZnJvbVBheWxvYWQobV9kYXRhKSk7CiAgICAgfQogICAgIAor ICAgIHVuc2lnbmVkIHJlZkNvdW50KCkgY29uc3QKKyAgICB7CisgICAgICAgIGlmICghbV9kYXRh KQorICAgICAgICAgICAgcmV0dXJuIDA7CisgICAgICAgIHJldHVybiBIZWFkZXI6OmZyb21QYXls b2FkKG1fZGF0YSktPnJlZkNvdW50OworICAgIH0KKyAgICAKICAgICBzaXplX3Qgc2l6ZSgpIGNv bnN0CiAgICAgewogICAgICAgICBpZiAoIW1fZGF0YSkK