Bug 195013

Summary:

[JSC] Revert r226885 to make SlotVisitor creation lazy

Product:

WebKit

Reporter:

Yusuke Suzuki <ysuzuki>

Component:

New Bugs

Assignee:

Yusuke Suzuki <ysuzuki>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

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

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

180907

Bug Blocks:

195020

Attachments:

Description	Flags
Patch	none
Patch	saam: review+

Yusuke Suzuki

Reported 2019-02-25 13:16:38 PST

[JSC] Revert r226885 to make SlotVisitor creation lazy

Attachments
Patch (6.08 KB, patch) 2019-02-25 13:27 PST, Yusuke Suzuki	no flags	Details Formatted Diff Diff
Patch (6.31 KB, patch) 2019-02-25 13:33 PST, Yusuke Suzuki	saam: review+	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Yusuke Suzuki

Comment 1 2019-02-25 13:27:14 PST

Created attachment 362922 [details] Patch

Yusuke Suzuki

Comment 2 2019-02-25 13:33:06 PST

Created attachment 362923 [details] Patch

Saam Barati

Comment 3 2019-02-25 13:35:31 PST

Comment on attachment 362923 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362923&action=review > Source/JavaScriptCore/ChangeLog:8 > + We once changed SlotVisitor creation apriori to drop the lock. But it turns out that SlotVisitor is memory-consuming. I don't understand what these two sentences have to do with each other. What does "memory-consuming" here mean?

Yusuke Suzuki

Comment 4 2019-02-25 13:40:11 PST

Comment on attachment 362923 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362923&action=review >> Source/JavaScriptCore/ChangeLog:8 >> + We once changed SlotVisitor creation apriori to drop the lock. But it turns out that SlotVisitor is memory-consuming. > > I don't understand what these two sentences have to do with each other. What does "memory-consuming" here mean? In previous change, we created # of Heap collectors SlotVisitors apriori. But each SlotVisitor takes so much memory (each has two MarkedStack, and each MarkedStack at least has 4KB dirty memory). This patch reverts that apriori creation, and creates SlotVisitors dynamically.

Yusuke Suzuki

Comment 5 2019-02-25 14:18:05 PST

Comment on attachment 362923 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362923&action=review >>> Source/JavaScriptCore/ChangeLog:8 >>> + We once changed SlotVisitor creation apriori to drop the lock. But it turns out that SlotVisitor is memory-consuming. >> >> I don't understand what these two sentences have to do with each other. What does "memory-consuming" here mean? > > In previous change, we created # of Heap collectors SlotVisitors apriori. But each SlotVisitor takes so much memory (each has two MarkedStack, and each MarkedStack at least has 4KB dirty memory). > This patch reverts that apriori creation, and creates SlotVisitors dynamically. And the previous patch removed the lock because we no longer created SlotVisitors dynamically. But this patch attempts to revert that. Then, the lock comes back. I checked `forEachSlotVisitor()` calls in our JSC code base and the passed lambda does not cause dead-lock.

Saam Barati

Comment 6 2019-02-25 19:22:10 PST

Comment on attachment 362923 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362923&action=review >>>> Source/JavaScriptCore/ChangeLog:8 >>>> + We once changed SlotVisitor creation apriori to drop the lock. But it turns out that SlotVisitor is memory-consuming. >>> >>> I don't understand what these two sentences have to do with each other. What does "memory-consuming" here mean? >> >> In previous change, we created # of Heap collectors SlotVisitors apriori. But each SlotVisitor takes so much memory (each has two MarkedStack, and each MarkedStack at least has 4KB dirty memory). >> This patch reverts that apriori creation, and creates SlotVisitors dynamically. > > And the previous patch removed the lock because we no longer created SlotVisitors dynamically. But this patch attempts to revert that. Then, the lock comes back. > I checked `forEachSlotVisitor()` calls in our JSC code base and the passed lambda does not cause dead-lock. "But" => "Also, it turns out..."

Yusuke Suzuki

Comment 7 2019-02-25 21:01:56 PST

Comment on attachment 362923 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362923&action=review Thanks! >>>>> Source/JavaScriptCore/ChangeLog:8 >>>>> + We once changed SlotVisitor creation apriori to drop the lock. But it turns out that SlotVisitor is memory-consuming. >>>> >>>> I don't understand what these two sentences have to do with each other. What does "memory-consuming" here mean? >>> >>> In previous change, we created # of Heap collectors SlotVisitors apriori. But each SlotVisitor takes so much memory (each has two MarkedStack, and each MarkedStack at least has 4KB dirty memory). >>> This patch reverts that apriori creation, and creates SlotVisitors dynamically. >> >> And the previous patch removed the lock because we no longer created SlotVisitors dynamically. But this patch attempts to revert that. Then, the lock comes back. >> I checked `forEachSlotVisitor()` calls in our JSC code base and the passed lambda does not cause dead-lock. > > "But" => "Also, it turns out..." Fixed.

Yusuke Suzuki

Comment 8 2019-02-25 21:37:58 PST

Committed r242070: <https://trac.webkit.org/changeset/242070>

Radar WebKit Bug Importer

Comment 9 2019-02-25 21:41:15 PST

<rdar://problem/48390304>

Yusuke Suzuki

Comment 10 2019-05-28 01:45:08 PDT

Committed r245808: <https://trac.webkit.org/changeset/245808>

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