Bug 180906

Summary:

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

Product:

WebKit

Reporter:

Yusuke Suzuki <ysuzuki>

Component:

New Bugs

Assignee:

Yusuke Suzuki <ysuzuki>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, ews-watchlist, fpizlo, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

179934

Bug Blocks:

180907

Attachments:

Description	Flags
Patch	fpizlo: review+

Yusuke Suzuki

Reported 2017-12-16 06:10:31 PST

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

Attachments
Patch (3.78 KB, patch) 2017-12-16 06:15 PST, Yusuke Suzuki	fpizlo: review+	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Yusuke Suzuki

Comment 1 2017-12-16 06:15:22 PST

Created attachment 329573 [details] Patch

Yusuke Suzuki

Comment 2 2017-12-16 07:15:37 PST

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 3 2017-12-16 15:02:56 PST

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 4 2017-12-17 02:08:35 PST

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

Comment 5 2017-12-17 02:53:56 PST

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

Radar WebKit Bug Importer

Comment 6 2017-12-17 02:54:17 PST

<rdar://problem/36094575>

Note You need to log in before you can comment on or make changes to this bug.