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: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | 179934 | ||||||
Bug Blocks: | 180907 | ||||||
Attachments: |
|
Description
Yusuke Suzuki
2017-12-16 06:10:31 PST
Created attachment 329573 [details]
Patch
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 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! 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 :) Committed r226010: <https://trac.webkit.org/changeset/226010> |