Bug 180907

Summary: [JSC] Create parallel SlotVisitors apriori
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, fpizlo, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 180906, 181318    
Bug Blocks: 195013    
Attachments:
Description Flags
Patch saam: review+

Description Yusuke Suzuki 2017-12-16 07:15:11 PST
...
Comment 1 Filip Pizlo 2017-12-16 15:00:35 PST
Good idea!!
Comment 2 Yusuke Suzuki 2017-12-17 05:26:32 PST
Created attachment 329601 [details]
Patch
Comment 3 Yusuke Suzuki 2017-12-17 05:29:35 PST
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 4 Saam Barati 2018-01-03 17:52:58 PST
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 5 Yusuke Suzuki 2018-01-04 07:40:41 PST
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.
Comment 6 Yusuke Suzuki 2018-01-04 07:42:12 PST
Committed r226405: <https://trac.webkit.org/changeset/226405>
Comment 7 Radar WebKit Bug Importer 2018-01-04 07:43:19 PST
<rdar://problem/36297366>
Comment 8 WebKit Commit Bot 2018-01-04 21:37:15 PST
Re-opened since this is blocked by bug 181318
Comment 9 Yusuke Suzuki 2018-01-12 04:13:57 PST
(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.
Comment 10 Yusuke Suzuki 2018-01-12 04:16:19 PST
Committed r226885: <https://trac.webkit.org/changeset/226885>