[webkit-reviews] review granted: [Bug 180906] [JSC] Number of SlotVisitors can increase after setting up m_visitCounters : [Attachment 329573] Patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 180906: [JSC] Number of SlotVisitors can increase after setting up
m_visitCounters
https://bugs.webkit.org/show_bug.cgi?id=180906

Attachment 329573: Patch

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




--- Comment #3 from Filip Pizlo <fpizlo at apple.com> ---
Comment on attachment 329573
  --> https://bugs.webkit.org/attachment.cgi?id=329573
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!