WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
195013
[JSC] Revert
r226885
to make SlotVisitor creation lazy
https://bugs.webkit.org/show_bug.cgi?id=195013
Summary
[JSC] Revert r226885 to make SlotVisitor creation lazy
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.
Top of Page
Format For Printing
XML
Clone This Bug