Bug 180906 – [JSC] Number of SlotVisitors can increase after setting up m_visitCounters

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

[JSC] Number of SlotVisitors can increase after setting up m_visitCounters

[Yusuke Suzuki]

Created attachment 329573 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 329573 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=329573&action=review

> Source/JavaScriptCore/heap/HeapInlines.h:274
> +    auto locker = holdLock(m_parallelSlotVisitorLock);

In this patch, we keep this lock and the current mechanism.
But I think we should create all SlotVisitors in Heap's constructor based on HeapHelperPool's # of threads.
The size of m_parallelSlotVisitors is capped with this value, and it should not be so large.
At that time, we can remove this m_parallelSlotVisitorLock in this function and forEachSlotVisitor function.
I opened a bug for this, which is separated from this bug.
https://bugs.webkit.org/show_bug.cgi?id=180907

[Filip Pizlo]

Comment on attachment 329573 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=329573&action=review

Nice!

>> Source/JavaScriptCore/heap/HeapInlines.h:274
>> +    auto locker = holdLock(m_parallelSlotVisitorLock);
> 
> In this patch, we keep this lock and the current mechanism.
> But I think we should create all SlotVisitors in Heap's constructor based on HeapHelperPool's # of threads.
> The size of m_parallelSlotVisitors is capped with this value, and it should not be so large.
> At that time, we can remove this m_parallelSlotVisitorLock in this function and forEachSlotVisitor function.
> I opened a bug for this, which is separated from this bug.
> https://bugs.webkit.org/show_bug.cgi?id=180907

I think it's safe to assume that Vector::size() doesn't need locking.  On the other hand, it's also OK to be paranoid.  Up to you!

[Yusuke Suzuki]

Comment on attachment 329573 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=329573&action=review

Thank you for your review!

>>> Source/JavaScriptCore/heap/HeapInlines.h:274
>>> +    auto locker = holdLock(m_parallelSlotVisitorLock);
>> 
>> In this patch, we keep this lock and the current mechanism.
>> But I think we should create all SlotVisitors in Heap's constructor based on HeapHelperPool's # of threads.
>> The size of m_parallelSlotVisitors is capped with this value, and it should not be so large.
>> At that time, we can remove this m_parallelSlotVisitorLock in this function and forEachSlotVisitor function.
>> I opened a bug for this, which is separated from this bug.
>> https://bugs.webkit.org/show_bug.cgi?id=180907
> 
> I think it's safe to assume that Vector::size() doesn't need locking.  On the other hand, it's also OK to be paranoid.  Up to you!

I would like to keep this for a while. Once bug 180907 is fixed, we can remove this checking :)

[Yusuke Suzuki]

Committed r226010: <https://trac.webkit.org/changeset/226010>

[Radar WebKit Bug Importer]

<rdar://problem/36094575>