Bug 180907 - [JSC] Create parallel SlotVisitors apriori
Summary: [JSC] Create parallel SlotVisitors apriori
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on: 180906 181318
Blocks: 195013
  Show dependency treegraph
 
Reported: 2017-12-16 07:15 PST by Yusuke Suzuki
Modified: 2019-02-25 13:17 PST (History)
8 users (show)

See Also:


Attachments
Patch (5.68 KB, patch)
2017-12-17 05:26 PST, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>