Bug 180907 – [JSC] Create parallel SlotVisitors apriori

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

...

[Filip Pizlo]

Good idea!!

[Yusuke Suzuki]

Created attachment 329601 [details]
Patch

[Yusuke Suzuki]

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.

[Saam Barati]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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

[Radar WebKit Bug Importer]

<rdar://problem/36297366>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 181318

[Yusuke Suzuki]

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

[Yusuke Suzuki]

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