180907 2017-12-16 07:15:11 -0800 [JSC] Create parallel SlotVisitors apriori 2019-02-25 13:17:13 -0800 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 180906 181318 195013 1 ysuzuki ysuzuki commit-queue ews-watchlist fpizlo keith_miller mark.lam msaboff saam webkit-bug-importer oldest_to_newest 1382486 0 ysuzuki 2017-12-16 07:15:11 -0800 ... 1382537 1 fpizlo 2017-12-16 15:00:35 -0800 Good idea!! 1382590 2 329601 ysuzuki 2017-12-17 05:26:32 -0800 Created attachment 329601 Patch 1382591 3 329601 ysuzuki 2017-12-17 05:29:35 -0800 Comment on attachment 329601 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329601&action=review > Source/JavaScriptCore/heap/HeapInlines.h:-265 > - auto locker = holdLock(m_parallelSlotVisitorLock); I think this lock guards m_paralleSlotVisitors vector state. If it is correct, this lock is no longer necessary. This patch drops this. Looking through the code, I think this lock is only used for that purpose. But if this lock is used in the other purpose, we need to consider carefully about dropping this lock here. The side effect of holding this lock is, while holding this lock, no threads newly starts marking since it is blocked when getting SlotVisitor. But I don't think this lock is not used for this purpose. 1385637 4 329601 saam 2018-01-03 17:52:58 -0800 Comment on attachment 329601 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329601&action=review > Source/JavaScriptCore/ChangeLog:9 > + If we create these SlotVisitors apropri, we do not need to create SlotVisitors dynamically. apropri => apriori >> Source/JavaScriptCore/heap/HeapInlines.h:-265 >> - auto locker = holdLock(m_parallelSlotVisitorLock); > > I think this lock guards m_paralleSlotVisitors vector state. If it is correct, this lock is no longer necessary. This patch drops this. > Looking through the code, I think this lock is only used for that purpose. But if this lock is used in the other purpose, we need to consider carefully about dropping this lock here. > The side effect of holding this lock is, while holding this lock, no threads newly starts marking since it is blocked when getting SlotVisitor. But I don't think this lock is not used for this purpose. I agree by looking at the code. Fil would know for sure. I guess we need to consider calls to visitCount() and bytesVisited(). I don't actually see calls to that, but I didn't look very closely. 1385801 5 329601 ysuzuki 2018-01-04 07:40:41 -0800 Comment on attachment 329601 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329601&action=review >> Source/JavaScriptCore/ChangeLog:9 >> + If we create these SlotVisitors apropri, we do not need to create SlotVisitors dynamically. > > apropri => apriori Fixed. >>> Source/JavaScriptCore/heap/HeapInlines.h:-265 >>> - auto locker = holdLock(m_parallelSlotVisitorLock); >> >> I think this lock guards m_paralleSlotVisitors vector state. If it is correct, this lock is no longer necessary. This patch drops this. >> Looking through the code, I think this lock is only used for that purpose. But if this lock is used in the other purpose, we need to consider carefully about dropping this lock here. >> The side effect of holding this lock is, while holding this lock, no threads newly starts marking since it is blocked when getting SlotVisitor. But I don't think this lock is not used for this purpose. > > I agree by looking at the code. Fil would know for sure. I guess we need to consider calls to visitCount() and bytesVisited(). I don't actually see calls to that, but I didn't look very closely. I think the existing code does not consider about it. didVisitSomething checks visit counters without locking. I would like to ask Fil about visit counter's locking strategy, but anyway, this patch itself does not change the current strategy. 1385802 6 ysuzuki 2018-01-04 07:42:12 -0800 Committed r226405: <https://trac.webkit.org/changeset/226405> 1385804 7 webkit-bug-importer 2018-01-04 07:43:19 -0800 <rdar://problem/36297366> 1386064 8 commit-queue 2018-01-04 21:37:15 -0800 Re-opened since this is blocked by bug 181318 1388762 9 ysuzuki 2018-01-12 04:13:57 -0800 (In reply to WebKit Commit Bot from comment #8) > Re-opened since this is blocked by bug 181318 Before this patch, SlotVisitor is eventually created, and typically created in different threads. It places SlotVisitor in largely different memory region. But now, these visitors are allocated at first, it tends to place them very similar region and cause false sharing since typically these visitors are used in different threads later. I've added some padding `char padding[64];` to visitor and ensured performance degradation is fixed in my local copy. I'll land it with this change to check the bot agrees. 1388763 10 ysuzuki 2018-01-12 04:16:19 -0800 Committed r226885: <https://trac.webkit.org/changeset/226885> 329601 2017-12-17 05:26:32 -0800 2018-01-03 17:52:58 -0800 Patch bug-180907-20171217222632.patch text/plain 5820 ysuzuki U3VidmVyc2lvbiBSZXZpc2lvbjogMjI2MDEwCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCA4 M2EyYzA0ZTIyYjU3MjUyMTQzNWJjMGMwYjhiYzIxN2NhN2M0YjI0Li4xOWU4ZjQxYjg1NzJjMGZk ODEyNWU1OWE3M2Y3NzE0YjVmNDRlOGE3IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwyOCBAQAorMjAxNy0xMi0xNyAgWXVzdWtlIFN1enVraSAgPHV0YXRhbmUudGVhQGdtYWls LmNvbT4KKworICAgICAgICBbSlNDXSBDcmVhdGUgcGFyYWxsZWwgU2xvdFZpc2l0b3JzIGFwcmlv cmkKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE4MDkw NworCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRoZSBu dW1iZXIgb2YgU2xvdFZpc2l0b3JzIGFyZSBjYXBwZWQgd2l0aCB0aGUgbnVtYmVyIG9mIEhlYXBI ZWxwZXJQb29sJ3MgdGhyZWFkcyArIDIuCisgICAgICAgIElmIHdlIGNyZWF0ZSB0aGVzZSBTbG90 VmlzaXRvcnMgYXByb3ByaSwgd2UgZG8gbm90IG5lZWQgdG8gY3JlYXRlIFNsb3RWaXNpdG9ycyBk eW5hbWljYWxseS4KKyAgICAgICAgVGhlbiB3ZSBkbyBub3QgbmVlZCB0byBncmFiIGxvY2tzIHdo aWxlIGl0ZXJhdGluZyBhbGwgdGhlIFNsb3RWaXNpdG9ycy4KKworICAgICAgICBJbiBhZGRpdGlv biwgd2UgZG8gbm90IG5lZWQgdG8gY29uc2lkZXIgdGhlIGNhc2UgdGhhdCB0aGUgbnVtYmVyIG9m IFNsb3RWaXNpdG9ycyBpbmNyZWFzZXMKKyAgICAgICAgYWZ0ZXIgc2V0dGluZyB1cCBWaXNpdENv dW50ZXJzIGluIE1hcmtpbmdDb25zdHJhaW50U29sdmVyIHNpbmNlIHRoZSBudW1iZXIgb2YgU2xv dFZpc2l0b3JzCisgICAgICAgIGRvZXMgbm90IGluY3JlYXNlIGFueSBtb3JlLgorCisgICAgICAg ICogaGVhcC9IZWFwLmNwcDoKKyAgICAgICAgKEpTQzo6SGVhcDo6SGVhcCk6CisgICAgICAgIChK U0M6OkhlYXA6OnJ1bkJlZ2luUGhhc2UpOgorICAgICAgICAqIGhlYXAvSGVhcC5oOgorICAgICAg ICAqIGhlYXAvSGVhcElubGluZXMuaDoKKyAgICAgICAgKEpTQzo6SGVhcDo6Zm9yRWFjaFNsb3RW aXNpdG9yKToKKyAgICAgICAgKEpTQzo6SGVhcDo6bnVtYmVyT2ZTbG90VmlzaXRvcnMpOiBEZWxl dGVkLgorICAgICAgICAqIGhlYXAvTWFya2luZ0NvbnN0cmFpbnRTb2x2ZXIuY3BwOgorICAgICAg ICAoSlNDOjpNYXJraW5nQ29uc3RyYWludFNvbHZlcjo6ZGlkVmlzaXRTb21ldGhpbmcgY29uc3Qp OgorCiAyMDE3LTEyLTE2ICBZdXN1a2UgU3V6dWtpICA8dXRhdGFuZS50ZWFAZ21haWwuY29tPgog CiAgICAgICAgIFtKU0NdIE51bWJlciBvZiBTbG90VmlzaXRvcnMgY2FuIGluY3JlYXNlIGFmdGVy IHNldHRpbmcgdXAgbV92aXNpdENvdW50ZXJzCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvaGVhcC9IZWFwLmNwcCBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9oZWFwL0hlYXAuY3Bw CmluZGV4IDEyYzFiNmY4NTY2ZjllNWMyNGFjYzVjNjRhMGUxZTE1YWViZmQwNjguLjhlYmM3Y2Ri ZmUwYmE3NTM0ODZlZGRkNWU3YTNkZGY5OWE0NjJhN2IgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZh U2NyaXB0Q29yZS9oZWFwL0hlYXAuY3BwCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9oZWFw L0hlYXAuY3BwCkBAIC0zMTEsNiArMzExLDE0IEBAIEhlYXA6OkhlYXAoVk0qIHZtLCBIZWFwVHlw ZSBoZWFwVHlwZSkKICAgICAsIG1fdGhyZWFkQ29uZGl0aW9uKEF1dG9tYXRpY1RocmVhZENvbmRp dGlvbjo6Y3JlYXRlKCkpCiB7CiAgICAgbV93b3JsZFN0YXRlLnN0b3JlKDApOworCisgICAgZm9y ICh1bnNpZ25lZCBpID0gMCwgbnVtYmVyT2ZQYXJhbGxlbFRocmVhZHMgPSBoZWFwSGVscGVyUG9v bCgpLm51bWJlck9mVGhyZWFkcygpOyBpIDwgbnVtYmVyT2ZQYXJhbGxlbFRocmVhZHM7ICsraSkg eworICAgICAgICBzdGQ6OnVuaXF1ZV9wdHI8U2xvdFZpc2l0b3I+IHZpc2l0b3IgPSBzdGQ6Om1h a2VfdW5pcXVlPFNsb3RWaXNpdG9yPigqdGhpcywgdG9DU3RyaW5nKCJQIiwgaSArIDEpKTsKKyAg ICAgICAgaWYgKE9wdGlvbnM6Om9wdGltaXplUGFyYWxsZWxTbG90VmlzaXRvcnNGb3JTdG9wcGVk TXV0YXRvcigpKQorICAgICAgICAgICAgdmlzaXRvci0+b3B0aW1pemVGb3JTdG9wcGVkTXV0YXRv cigpOworICAgICAgICBtX2F2YWlsYWJsZVBhcmFsbGVsU2xvdFZpc2l0b3JzLmFwcGVuZCh2aXNp dG9yLmdldCgpKTsKKyAgICAgICAgbV9wYXJhbGxlbFNsb3RWaXNpdG9ycy5hcHBlbmQoV1RGTW92 ZSh2aXNpdG9yKSk7CisgICAgfQogICAgIAogICAgIGlmIChPcHRpb25zOjp1c2VDb25jdXJyZW50 R0MoKSkgewogICAgICAgICBpZiAoT3B0aW9uczo6dXNlU3RvY2hhc3RpY011dGF0b3JTY2hlZHVs ZXIoKSkKQEAgLTEyMzEsMTkgKzEyMzksOCBAQCBORVZFUl9JTkxJTkUgYm9vbCBIZWFwOjpydW5C ZWdpblBoYXNlKEdDQ29uZHVjdG9yIGNvbm4pCiAgICAgICAgICAgICBTbG90VmlzaXRvciogc2xv dFZpc2l0b3I7CiAgICAgICAgICAgICB7CiAgICAgICAgICAgICAgICAgTG9ja0hvbGRlciBsb2Nr ZXIobV9wYXJhbGxlbFNsb3RWaXNpdG9yTG9jayk7Ci0gICAgICAgICAgICAgICAgaWYgKG1fYXZh aWxhYmxlUGFyYWxsZWxTbG90VmlzaXRvcnMuaXNFbXB0eSgpKSB7Ci0gICAgICAgICAgICAgICAg ICAgIHN0ZDo6dW5pcXVlX3B0cjxTbG90VmlzaXRvcj4gbmV3VmlzaXRvciA9IHN0ZDo6bWFrZV91 bmlxdWU8U2xvdFZpc2l0b3I+KAotICAgICAgICAgICAgICAgICAgICAgICAgKnRoaXMsIHRvQ1N0 cmluZygiUCIsIG1fcGFyYWxsZWxTbG90VmlzaXRvcnMuc2l6ZSgpICsgMSkpOwotICAgICAgICAg ICAgICAgICAgICAKLSAgICAgICAgICAgICAgICAgICAgaWYgKE9wdGlvbnM6Om9wdGltaXplUGFy YWxsZWxTbG90VmlzaXRvcnNGb3JTdG9wcGVkTXV0YXRvcigpKQotICAgICAgICAgICAgICAgICAg ICAgICAgbmV3VmlzaXRvci0+b3B0aW1pemVGb3JTdG9wcGVkTXV0YXRvcigpOwotICAgICAgICAg ICAgICAgICAgICAKLSAgICAgICAgICAgICAgICAgICAgbmV3VmlzaXRvci0+ZGlkU3RhcnRNYXJr aW5nKCk7Ci0gICAgICAgICAgICAgICAgICAgIAotICAgICAgICAgICAgICAgICAgICBzbG90Vmlz aXRvciA9IG5ld1Zpc2l0b3IuZ2V0KCk7Ci0gICAgICAgICAgICAgICAgICAgIG1fcGFyYWxsZWxT bG90VmlzaXRvcnMuYXBwZW5kKFdURk1vdmUobmV3VmlzaXRvcikpOwotICAgICAgICAgICAgICAg IH0gZWxzZQotICAgICAgICAgICAgICAgICAgICBzbG90VmlzaXRvciA9IG1fYXZhaWxhYmxlUGFy YWxsZWxTbG90VmlzaXRvcnMudGFrZUxhc3QoKTsKKyAgICAgICAgICAgICAgICBSRUxFQVNFX0FT U0VSVF9XSVRIX01FU1NBR0UoIW1fYXZhaWxhYmxlUGFyYWxsZWxTbG90VmlzaXRvcnMuaXNFbXB0 eSgpLCAiUGFyYWxsZWwgU2xvdFZpc2l0b3JzIGFyZSBhbGxvY2F0ZWQgYXByaW9yaSIpOworICAg ICAgICAgICAgICAgIHNsb3RWaXNpdG9yID0gbV9hdmFpbGFibGVQYXJhbGxlbFNsb3RWaXNpdG9y cy50YWtlTGFzdCgpOwogICAgICAgICAgICAgfQogCiAgICAgICAgICAgICBXVEY6OnJlZ2lzdGVy R0NUaHJlYWQoR0NUaHJlYWRUeXBlOjpIZWxwZXIpOwpkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFT Y3JpcHRDb3JlL2hlYXAvSGVhcC5oIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL2hlYXAvSGVhcC5o CmluZGV4IDBmYzJiNzU0NGRiOGY1ODI4YmM4NjU3YjQ5N2JmYjNhYmVlYjQ4NDUuLjgzNWU0ZmZh YmY5MTZhMjYyYWUyMGRiY2E3OWJjZDFlNGYyYTc3ZWQgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZh U2NyaXB0Q29yZS9oZWFwL0hlYXAuaAorKysgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvaGVhcC9I ZWFwLmgKQEAgLTM3Miw3ICszNzIsNiBAQCBjbGFzcyBIZWFwIHsKIAogICAgIHRlbXBsYXRlPHR5 cGVuYW1lIEZ1bmM+CiAgICAgdm9pZCBmb3JFYWNoU2xvdFZpc2l0b3IoY29uc3QgRnVuYyYpOwot ICAgIHVuc2lnbmVkIG51bWJlck9mU2xvdFZpc2l0b3JzKCk7CiAKIHByaXZhdGU6CiAgICAgZnJp ZW5kIGNsYXNzIEFsbG9jYXRpbmdTY29wZTsKZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0 Q29yZS9oZWFwL0hlYXBJbmxpbmVzLmggYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvaGVhcC9IZWFw SW5saW5lcy5oCmluZGV4IDJhZGQwZWQ0NWZkOGE5NjUxYzNmMmNjZjQ5ZDg5OTZiMWJhMjBjODEu LjUxMjYwYzFhNjE1OWQzMTVmNjQyOTNlMTAxMGEwYWI5ZjA3YzUwZDMgMTAwNjQ0Ci0tLSBhL1Nv dXJjZS9KYXZhU2NyaXB0Q29yZS9oZWFwL0hlYXBJbmxpbmVzLmgKKysrIGIvU291cmNlL0phdmFT Y3JpcHRDb3JlL2hlYXAvSGVhcElubGluZXMuaApAQCAtMjYyLDE3ICsyNjIsMTAgQEAgaW5saW5l IHZvaWQgSGVhcDo6c3RvcElmTmVjZXNzYXJ5KCkKIHRlbXBsYXRlPHR5cGVuYW1lIEZ1bmM+CiB2 b2lkIEhlYXA6OmZvckVhY2hTbG90VmlzaXRvcihjb25zdCBGdW5jJiBmdW5jKQogewotICAgIGF1 dG8gbG9ja2VyID0gaG9sZExvY2sobV9wYXJhbGxlbFNsb3RWaXNpdG9yTG9jayk7CiAgICAgZnVu YygqbV9jb2xsZWN0b3JTbG90VmlzaXRvcik7CiAgICAgZnVuYygqbV9tdXRhdG9yU2xvdFZpc2l0 b3IpOwogICAgIGZvciAoYXV0byYgc2xvdFZpc2l0b3IgOiBtX3BhcmFsbGVsU2xvdFZpc2l0b3Jz KQogICAgICAgICBmdW5jKCpzbG90VmlzaXRvcik7CiB9CiAKLWlubGluZSB1bnNpZ25lZCBIZWFw OjpudW1iZXJPZlNsb3RWaXNpdG9ycygpCi17Ci0gICAgYXV0byBsb2NrZXIgPSBob2xkTG9jayht X3BhcmFsbGVsU2xvdFZpc2l0b3JMb2NrKTsKLSAgICByZXR1cm4gbV9wYXJhbGxlbFNsb3RWaXNp dG9ycy5zaXplKCkgKyAyOyAvLyBtX2NvbGxlY3RvclNsb3RWaXNpdG9yIGFuZCBtX211dGF0b3JT bG90VmlzaXRvcgotfQotCiB9IC8vIG5hbWVzcGFjZSBKU0MKZGlmZiAtLWdpdCBhL1NvdXJjZS9K YXZhU2NyaXB0Q29yZS9oZWFwL01hcmtpbmdDb25zdHJhaW50U29sdmVyLmNwcCBiL1NvdXJjZS9K YXZhU2NyaXB0Q29yZS9oZWFwL01hcmtpbmdDb25zdHJhaW50U29sdmVyLmNwcAppbmRleCA4OWM2 MDZhMmEzYzdhZGI1YmJmMDE1OGFkODlhNWEzZmM0MjkwMDExLi4yMzI5ZTZiNWJmOTlhM2FjZjMy OTE3MTBhNjlkMTNhYWZhYmRiYzJmIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUv aGVhcC9NYXJraW5nQ29uc3RyYWludFNvbHZlci5jcHAKKysrIGIvU291cmNlL0phdmFTY3JpcHRD b3JlL2hlYXAvTWFya2luZ0NvbnN0cmFpbnRTb2x2ZXIuY3BwCkBAIC01MSwxMCArNTEsNiBAQCBi b29sIE1hcmtpbmdDb25zdHJhaW50U29sdmVyOjpkaWRWaXNpdFNvbWV0aGluZygpIGNvbnN0CiAg ICAgICAgIGlmICh2aXNpdENvdW50ZXIudmlzaXRDb3VudCgpKQogICAgICAgICAgICAgcmV0dXJu IHRydWU7CiAgICAgfQotICAgIC8vIElmIHRoZSBudW1iZXIgb2YgU2xvdFZpc2l0b3JzIGluY3Jl YXNlcyBhZnRlciBjcmVhdGluZyBtX3Zpc2l0Q291bnRlcnMsCi0gICAgLy8gd2UgY29uc2VydmF0 aXZlbHkgc2F5IHRoZXJlIGNvdWxkIGJlIHNvbWV0aGluZyB2aXNpdGVkIGJ5IGFkZGVkIFNsb3RW aXNpdG9ycy4KLSAgICBpZiAobV9oZWFwLm51bWJlck9mU2xvdFZpc2l0b3JzKCkgPiBtX3Zpc2l0 Q291bnRlcnMuc2l6ZSgpKQotICAgICAgICByZXR1cm4gdHJ1ZTsKICAgICByZXR1cm4gZmFsc2U7 CiB9CiAK