Bug 195013 - [JSC] Revert r226885 to make SlotVisitor creation lazy
Summary: [JSC] Revert r226885 to make SlotVisitor creation lazy
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on: 180907
Blocks: 195020
  Show dependency treegraph
 
Reported: 2019-02-25 13:16 PST by Yusuke Suzuki
Modified: 2019-05-28 01:45 PDT (History)
6 users (show)

See Also:


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
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2019-02-25 13:16:38 PST
[JSC] Revert r226885 to make SlotVisitor creation lazy
Comment 1 Yusuke Suzuki 2019-02-25 13:27:14 PST
Created attachment 362922 [details]
Patch
Comment 2 Yusuke Suzuki 2019-02-25 13:33:06 PST
Created attachment 362923 [details]
Patch
Comment 3 Saam Barati 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?
Comment 4 Yusuke Suzuki 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.
Comment 5 Yusuke Suzuki 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.
Comment 6 Saam Barati 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..."
Comment 7 Yusuke Suzuki 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.
Comment 8 Yusuke Suzuki 2019-02-25 21:37:58 PST
Committed r242070: <https://trac.webkit.org/changeset/242070>
Comment 9 Radar WebKit Bug Importer 2019-02-25 21:41:15 PST
<rdar://problem/48390304>
Comment 10 Yusuke Suzuki 2019-05-28 01:45:08 PDT
Committed r245808: <https://trac.webkit.org/changeset/245808>