180906 2017-12-16 06:10:31 -0800 [JSC] Number of SlotVisitors can increase after setting up m_visitCounters 2017-12-17 02:57:56 -0800 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 179934 180907 1 ysuzuki ysuzuki commit-queue ews-watchlist fpizlo keith_miller mark.lam msaboff saam webkit-bug-importer oldest_to_newest 1382481 0 ysuzuki 2017-12-16 06:10:31 -0800 [JSC] Number of SlotVisitors can increase after setting up m_visitCounters 1382482 1 329573 ysuzuki 2017-12-16 06:15:22 -0800 Created attachment 329573 Patch 1382487 2 329573 ysuzuki 2017-12-16 07:15:37 -0800 Comment on attachment 329573 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329573&action=review > Source/JavaScriptCore/heap/HeapInlines.h:274 > + auto locker = holdLock(m_parallelSlotVisitorLock); In this patch, we keep this lock and the current mechanism. But I think we should create all SlotVisitors in Heap's constructor based on HeapHelperPool's # of threads. The size of m_parallelSlotVisitors is capped with this value, and it should not be so large. At that time, we can remove this m_parallelSlotVisitorLock in this function and forEachSlotVisitor function. I opened a bug for this, which is separated from this bug. https://bugs.webkit.org/show_bug.cgi?id=180907 1382538 3 329573 fpizlo 2017-12-16 15:02:56 -0800 Comment on attachment 329573 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329573&action=review Nice! >> Source/JavaScriptCore/heap/HeapInlines.h:274 >> + auto locker = holdLock(m_parallelSlotVisitorLock); > > In this patch, we keep this lock and the current mechanism. > But I think we should create all SlotVisitors in Heap's constructor based on HeapHelperPool's # of threads. > The size of m_parallelSlotVisitors is capped with this value, and it should not be so large. > At that time, we can remove this m_parallelSlotVisitorLock in this function and forEachSlotVisitor function. > I opened a bug for this, which is separated from this bug. > https://bugs.webkit.org/show_bug.cgi?id=180907 I think it's safe to assume that Vector::size() doesn't need locking. On the other hand, it's also OK to be paranoid. Up to you! 1382587 4 329573 ysuzuki 2017-12-17 02:08:35 -0800 Comment on attachment 329573 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329573&action=review Thank you for your review! >>> Source/JavaScriptCore/heap/HeapInlines.h:274 >>> + auto locker = holdLock(m_parallelSlotVisitorLock); >> >> In this patch, we keep this lock and the current mechanism. >> But I think we should create all SlotVisitors in Heap's constructor based on HeapHelperPool's # of threads. >> The size of m_parallelSlotVisitors is capped with this value, and it should not be so large. >> At that time, we can remove this m_parallelSlotVisitorLock in this function and forEachSlotVisitor function. >> I opened a bug for this, which is separated from this bug. >> https://bugs.webkit.org/show_bug.cgi?id=180907 > > I think it's safe to assume that Vector::size() doesn't need locking. On the other hand, it's also OK to be paranoid. Up to you! I would like to keep this for a while. Once bug 180907 is fixed, we can remove this checking :) 1382588 5 ysuzuki 2017-12-17 02:53:56 -0800 Committed r226010: <https://trac.webkit.org/changeset/226010> 1382589 6 webkit-bug-importer 2017-12-17 02:54:17 -0800 <rdar://problem/36094575> 329573 2017-12-16 06:15:22 -0800 2017-12-17 02:54:31 -0800 Patch bug-180906-20171216231521.patch text/plain 3869 ysuzuki U3VidmVyc2lvbiBSZXZpc2lvbjogMjI1OTcxCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBh ZmIzOTY1ODIzMmVhZjAxNjI5ZmYwOTY5M2UzODE2MjJkOTA0YjU5Li41OGM5ZjBlMGYwMDI1MDQy MmUzM2Q2YWMzODMzOTBhNmVmZDA1YWM3IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwyNiBAQAorMjAxNy0xMi0xNiAgWXVzdWtlIFN1enVraSAgPHV0YXRhbmUudGVhQGdtYWls LmNvbT4KKworICAgICAgICBbSlNDXSBOdW1iZXIgb2YgU2xvdFZpc2l0b3JzIGNhbiBpbmNyZWFz ZSBhZnRlciBzZXR0aW5nIHVwIG1fdmlzaXRDb3VudGVycworICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTgwOTA2CisKKyAgICAgICAgUmV2aWV3ZWQgYnkg Tk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgVGhlIG51bWJlciBvZiBTbG90VmlzaXRvcnMgY2Fu IGluY3JlYXNlIGFmdGVyIHNldHRpbmcgdXAgbV92aXNpdENvdW50ZXJzLgorICAgICAgICBJZiBp dCBoYXBwZW5zLCBvdXIgbV92aXNpdENvdW50ZXJzIG1pc3NlcyB0aGUgdmlzaXQgY291bnQgb2Yg bmV3bHkgYWRkZWQKKyAgICAgICAgU2xvdFZpc2l0b3JzLiBJdCBhY2NpZGVudGFsbHkgZGVjaWRl cyB0aGF0IGNvbnN0cmFpbnRzIGFyZSBjb252ZXJnZWQuCisgICAgICAgIFRoaXMgbGVhZHMgdG8g cmFuZG9tIGFzc2VydGlvbiBoaXRzIGluIExpbnV4IGVudmlyb25tZW50LgorCisgICAgICAgIElu IHRoaXMgcGF0Y2gsIHdlIGNvbXBhcmUgdGhlIG51bWJlciBvZiBTbG90VmlzaXRvcnMgaW4gZGlk VmlzaXRTb21ldGhpbmcoKS4KKyAgICAgICAgSWYgdGhlIG51bWJlciBvZiBTbG90VmlzaXRvcnMg aXMgY2hhbmdlZCwgd2UgY29uc2VydmF0aXZlbHkgc2F5IHdlIGRpZAorICAgICAgICB2aXNpdCBz b21ldGhpbmcuCisKKyAgICAgICAgKiBoZWFwL0hlYXAuaDoKKyAgICAgICAgKiBoZWFwL0hlYXBJ bmxpbmVzLmg6CisgICAgICAgIChKU0M6OkhlYXA6Om51bWJlck9mU2xvdFZpc2l0b3JzKToKKyAg ICAgICAgKiBoZWFwL01hcmtpbmdDb25zdHJhaW50U2V0Lmg6CisgICAgICAgICogaGVhcC9NYXJr aW5nQ29uc3RyYWludFNvbHZlci5jcHA6CisgICAgICAgIChKU0M6Ok1hcmtpbmdDb25zdHJhaW50 U29sdmVyOjpkaWRWaXNpdFNvbWV0aGluZyBjb25zdCk6CisKIDIwMTctMTItMTUgIFl1c3VrZSBT dXp1a2kgIDx1dGF0YW5lLnRlYUBnbWFpbC5jb20+CiAKICAgICAgICAgVW5yZXZpZXdlZCwgMzJi aXQgSlNFbXB0eSBpcyBub3QgbnVsbHB0ciArIENlbGxUYWcKZGlmZiAtLWdpdCBhL1NvdXJjZS9K YXZhU2NyaXB0Q29yZS9oZWFwL0hlYXAuaCBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9oZWFwL0hl YXAuaAppbmRleCA4MzVlNGZmYWJmOTE2YTI2MmFlMjBkYmNhNzliY2QxZTRmMmE3N2VkLi4wZmMy Yjc1NDRkYjhmNTgyOGJjODY1N2I0OTdiZmIzYWJlZWI0ODQ1IDEwMDY0NAotLS0gYS9Tb3VyY2Uv SmF2YVNjcmlwdENvcmUvaGVhcC9IZWFwLmgKKysrIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL2hl YXAvSGVhcC5oCkBAIC0zNzIsNiArMzcyLDcgQEAgY2xhc3MgSGVhcCB7CiAKICAgICB0ZW1wbGF0 ZTx0eXBlbmFtZSBGdW5jPgogICAgIHZvaWQgZm9yRWFjaFNsb3RWaXNpdG9yKGNvbnN0IEZ1bmMm KTsKKyAgICB1bnNpZ25lZCBudW1iZXJPZlNsb3RWaXNpdG9ycygpOwogCiBwcml2YXRlOgogICAg IGZyaWVuZCBjbGFzcyBBbGxvY2F0aW5nU2NvcGU7CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNj cmlwdENvcmUvaGVhcC9IZWFwSW5saW5lcy5oIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL2hlYXAv SGVhcElubGluZXMuaAppbmRleCBlZDBiMjk2ZDRhMTIxMGZkZGU2OGRkZDYyZjdmMjg4YzkxYzU1 NDJiLi4yYWRkMGVkNDVmZDhhOTY1MWMzZjJjY2Y0OWQ4OTk2YjFiYTIwYzgxIDEwMDY0NAotLS0g YS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvaGVhcC9IZWFwSW5saW5lcy5oCisrKyBiL1NvdXJjZS9K YXZhU2NyaXB0Q29yZS9oZWFwL0hlYXBJbmxpbmVzLmgKQEAgLTI2OSw0ICsyNjksMTAgQEAgdm9p ZCBIZWFwOjpmb3JFYWNoU2xvdFZpc2l0b3IoY29uc3QgRnVuYyYgZnVuYykKICAgICAgICAgZnVu Yygqc2xvdFZpc2l0b3IpOwogfQogCitpbmxpbmUgdW5zaWduZWQgSGVhcDo6bnVtYmVyT2ZTbG90 VmlzaXRvcnMoKQoreworICAgIGF1dG8gbG9ja2VyID0gaG9sZExvY2sobV9wYXJhbGxlbFNsb3RW aXNpdG9yTG9jayk7CisgICAgcmV0dXJuIG1fcGFyYWxsZWxTbG90VmlzaXRvcnMuc2l6ZSgpICsg MjsgLy8gbV9jb2xsZWN0b3JTbG90VmlzaXRvciBhbmQgbV9tdXRhdG9yU2xvdFZpc2l0b3IKK30K KwogfSAvLyBuYW1lc3BhY2UgSlNDCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUv aGVhcC9NYXJraW5nQ29uc3RyYWludFNldC5oIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL2hlYXAv TWFya2luZ0NvbnN0cmFpbnRTZXQuaAppbmRleCBhN2UxODQ5ZDMxODE4N2JlZGFkYWY3NTFlYzhi YWM3ZWFiNzlkZTM5Li4wNzllZWU1ODZmODQzMmM5ZTRiZDUwZDU1ZDVkZTIzODE0ZjgzNGQ3IDEw MDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvaGVhcC9NYXJraW5nQ29uc3RyYWludFNl dC5oCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9oZWFwL01hcmtpbmdDb25zdHJhaW50U2V0 LmgKQEAgLTY3LDggKzY3LDYgQEAgY2xhc3MgTWFya2luZ0NvbnN0cmFpbnRTZXQgewogICAgIAog ICAgIGJvb2wgZXhlY3V0ZUNvbnZlcmdlbmNlSW1wbChTbG90VmlzaXRvciYpOwogICAgIAotICAg IGJvb2wgZHJhaW4oU2xvdFZpc2l0b3ImLCBCaXRWZWN0b3ImIHVuZXhlY3V0ZWQsIEJpdFZlY3Rv ciYgZXhlY3V0ZWQsIGJvb2wmIGRpZFZpc2l0U29tZXRoaW5nKTsKLSAgICAKICAgICBIZWFwJiBt X2hlYXA7CiAgICAgQml0VmVjdG9yIG1fdW5leGVjdXRlZFJvb3RzOwogICAgIEJpdFZlY3RvciBt X3VuZXhlY3V0ZWRPdXRncm93dGhzOwpkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFTY3JpcHRDb3Jl L2hlYXAvTWFya2luZ0NvbnN0cmFpbnRTb2x2ZXIuY3BwIGIvU291cmNlL0phdmFTY3JpcHRDb3Jl L2hlYXAvTWFya2luZ0NvbnN0cmFpbnRTb2x2ZXIuY3BwCmluZGV4IDIzMjllNmI1YmY5OWEzYWNm MzI5MTcxMGE2OWQxM2FhZmFiZGJjMmYuLjg5YzYwNmEyYTNjN2FkYjViYmYwMTU4YWQ4OWE1YTNm YzQyOTAwMTEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9oZWFwL01hcmtpbmdD b25zdHJhaW50U29sdmVyLmNwcAorKysgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvaGVhcC9NYXJr aW5nQ29uc3RyYWludFNvbHZlci5jcHAKQEAgLTUxLDYgKzUxLDEwIEBAIGJvb2wgTWFya2luZ0Nv bnN0cmFpbnRTb2x2ZXI6OmRpZFZpc2l0U29tZXRoaW5nKCkgY29uc3QKICAgICAgICAgaWYgKHZp c2l0Q291bnRlci52aXNpdENvdW50KCkpCiAgICAgICAgICAgICByZXR1cm4gdHJ1ZTsKICAgICB9 CisgICAgLy8gSWYgdGhlIG51bWJlciBvZiBTbG90VmlzaXRvcnMgaW5jcmVhc2VzIGFmdGVyIGNy ZWF0aW5nIG1fdmlzaXRDb3VudGVycywKKyAgICAvLyB3ZSBjb25zZXJ2YXRpdmVseSBzYXkgdGhl cmUgY291bGQgYmUgc29tZXRoaW5nIHZpc2l0ZWQgYnkgYWRkZWQgU2xvdFZpc2l0b3JzLgorICAg IGlmIChtX2hlYXAubnVtYmVyT2ZTbG90VmlzaXRvcnMoKSA+IG1fdmlzaXRDb3VudGVycy5zaXpl KCkpCisgICAgICAgIHJldHVybiB0cnVlOwogICAgIHJldHVybiBmYWxzZTsKIH0KIAo=