[JSC] Revert r226885 to make SlotVisitor creation lazy

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

[JSC] Revert r226885 to make SlotVisitor creation lazy

[Yusuke Suzuki]

Created attachment 362922 [details]
Patch

[Yusuke Suzuki]

Created attachment 362923 [details]
Patch

[Saam Barati]

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 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 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 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 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]

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

[Radar WebKit Bug Importer]

<rdar://problem/48390304>

[Yusuke Suzuki]

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