Bug 180907

Good idea!!

Created attachment 329601 [details] Patch

Comment on attachment 329601 [details] 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.

Comment on attachment 329601 [details] 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.

Comment on attachment 329601 [details] 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.

Committed r226405: <https://trac.webkit.org/changeset/226405>

<rdar://problem/36297366>

Re-opened since this is blocked by bug 181318

(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.

Committed r226885: <https://trac.webkit.org/changeset/226885>