Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2124693002:
Worker: Fix broken GC logic on Dedicated Worker while DOMTimer is set (Closed)
|
Created:
4 years, 5 months ago by nhiroki Modified:
4 years, 4 months ago CC:
chromium-reviews, blink-reviews, kinuko+worker_chromium.org, falken, blink-worker-reviews_chromium.org, horo+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWorker: Fix broken GC logic on Dedicated Worker while DOMTimer is set
V8GC detemines whether a Dedicated Worker can be collectable by checking
pending activities on WorkerGlobalScope. This activity state is managed by
InProcessWorkerMessagingProxy on the main thread. In some situation, this
logic is broken and this CL fixes it.
<Before this CL>
InProcessWorkerObjectProxy on the worker thread reports pending activities
to the main thread in following cases:
(1) After DedicatedWorkerThread is initialized, reports that there are no
pending activities regardless of whether they exist.
(2) After MessageEvent is dispatched, checks a pending activity using
V8GCController and reports a result.
This logic does not work when a DOMTimer is set on initial script evaluation or
on MessageEvent. DOMTimer must be treated as a pending activity[*], but both
(1) and (2) do not take care of it (regarding 2, DOMTimer is not a
ScriptWrappable and V8GCController does not count such an object). As a result,
a worker can be GC'ed even if there is still an active timer.
<After this CL>
After initial script evaluation or MessageEvent, the object proxy starts an
exponential backoff timer to periodically check an activity state and reports
to the messaging proxy when all activities are done.
The messaging proxy reports to the GC that the worker object is collectable
after all posted messages and pending activities are completed.
[*] https://html.spec.whatwg.org/multipage/workers.html#the-worker's-lifetime
BUG=572226, 584851
Committed: https://crrev.com/9af6c91f5bc79d12247a5780071c44025e4c026d
Cr-Commit-Position: refs/heads/master@{#413052}
Patch Set 1 #
Total comments: 18
Patch Set 2 : rebase #Patch Set 3 : partially address review comments #Patch Set 4 : address review comments and add more tests #Patch Set 5 : stop checking pending activities on message event dispatching #
Total comments: 22
Patch Set 6 : rebase #Patch Set 7 : address review comments (postDelayedTask -> Timer, etc) #Patch Set 8 : address review comments more and minimize diff of this CL #Patch Set 9 : remove NOTREACHED from CompositorWorkerTaskRunnerWrapper::GetTimeDomain() #Patch Set 10 : fix win builds #Patch Set 11 : annotate WorkerGlobalScope interface with 'ActiveScriptWrappable' and make tests stricter #
Total comments: 17
Patch Set 12 : rebase #Patch Set 13 : address review comments #Patch Set 14 : add DependentLifetime annotation to WorkerGlobalScope.idl for fixing layout tests #Patch Set 15 : rebase #
Total comments: 4
Patch Set 16 : return if the timer is active #
Total comments: 5
Patch Set 17 : reset the next interval duration #Patch Set 18 : rebase #Messages
Total messages: 176 (123 generated)
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Description was changed from ========== WIP: Check pending activity BUG= ========== to ========== WIP: Worker: Fix flaky GC logic for Dedicated Worker BUG=572226 ==========
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Description was changed from
==========
WIP: Worker: Fix flaky GC logic for Dedicated Worker
BUG=572226
==========
to
==========
Worker: Fix broken GC logic on Dedicated Worker when DOMTimer is set
Before this CL, pending activities on WorkerGlobalScope are reported to the
document in the following cases:
(A1) After DedicatedWorkerThread is initialized, reports that there are no
pending activities regardless of whether they exist.
(A2) After MessageEvent is dispatched, checks a pending activity using
V8GCController and reports a result.
This logic does not work when a DOMTimer is set on initial script evaluation or
on MessageEvent. DOMTimer must be treated as a pending activity[*], but (A1) and
(A2) do not take care of it. DOMTimer is not a ScriptWrappable and
V8GCController does not count it. As a result, DedicatedWorker can be GC'ed even
if there is still an active DOM Timer.
After this CL,
(B1) After DedicatedWorkerThread is initialized or MessageEvent is dispatched,
checks whether there is a pending activity including DOMTimer and reports
a result. In addition, if there is a pending activity, schedules a delayed
task to check whether the activity finishes later.
The task is scheduled with some delay. This delay is exponentially increased up
to some extent in order not to frequently run the task.
[*] https://html.spec.whatwg.org/multipage/workers.html#the-worker's-lifetime
BUG=572226, 584851
==========
Description was changed from
==========
Worker: Fix broken GC logic on Dedicated Worker when DOMTimer is set
Before this CL, pending activities on WorkerGlobalScope are reported to the
document in the following cases:
(A1) After DedicatedWorkerThread is initialized, reports that there are no
pending activities regardless of whether they exist.
(A2) After MessageEvent is dispatched, checks a pending activity using
V8GCController and reports a result.
This logic does not work when a DOMTimer is set on initial script evaluation or
on MessageEvent. DOMTimer must be treated as a pending activity[*], but (A1) and
(A2) do not take care of it. DOMTimer is not a ScriptWrappable and
V8GCController does not count it. As a result, DedicatedWorker can be GC'ed even
if there is still an active DOM Timer.
After this CL,
(B1) After DedicatedWorkerThread is initialized or MessageEvent is dispatched,
checks whether there is a pending activity including DOMTimer and reports
a result. In addition, if there is a pending activity, schedules a delayed
task to check whether the activity finishes later.
The task is scheduled with some delay. This delay is exponentially increased up
to some extent in order not to frequently run the task.
[*] https://html.spec.whatwg.org/multipage/workers.html#the-worker's-lifetime
BUG=572226, 584851
==========
to
==========
Worker: Fix broken GC logic on Dedicated Worker when DOMTimer is set
Before this CL, pending activities on WorkerGlobalScope are reported to the
document in the following cases:
(A1) After DedicatedWorkerThread is initialized, reports that there are no
pending activities regardless of whether they exist.
(A2) After MessageEvent is dispatched, checks a pending activity using
V8GCController and reports a result.
This logic does not work when a DOMTimer is set on initial script evaluation or
on MessageEvent. DOMTimer must be treated as a pending activity[*], but (A1) and
(A2) do not take care of it. DOMTimer is not a ScriptWrappable and
V8GCController does not count it. As a result, a worker can be GC'ed even if
there is still an active DOM Timer.
After this CL,
(B1) After DedicatedWorkerThread is initialized or MessageEvent is dispatched,
checks whether there is a pending activity including DOMTimer and reports
a result. In addition, if there is a pending activity, schedules a delayed
task to check whether the activity finishes later.
The task is scheduled with some delay. This delay is exponentially increased up
to some extent in order not to frequently run the task.
[*] https://html.spec.whatwg.org/multipage/workers.html#the-worker's-lifetime
BUG=572226, 584851
==========
Description was changed from
==========
Worker: Fix broken GC logic on Dedicated Worker when DOMTimer is set
Before this CL, pending activities on WorkerGlobalScope are reported to the
document in the following cases:
(A1) After DedicatedWorkerThread is initialized, reports that there are no
pending activities regardless of whether they exist.
(A2) After MessageEvent is dispatched, checks a pending activity using
V8GCController and reports a result.
This logic does not work when a DOMTimer is set on initial script evaluation or
on MessageEvent. DOMTimer must be treated as a pending activity[*], but (A1) and
(A2) do not take care of it. DOMTimer is not a ScriptWrappable and
V8GCController does not count it. As a result, a worker can be GC'ed even if
there is still an active DOM Timer.
After this CL,
(B1) After DedicatedWorkerThread is initialized or MessageEvent is dispatched,
checks whether there is a pending activity including DOMTimer and reports
a result. In addition, if there is a pending activity, schedules a delayed
task to check whether the activity finishes later.
The task is scheduled with some delay. This delay is exponentially increased up
to some extent in order not to frequently run the task.
[*] https://html.spec.whatwg.org/multipage/workers.html#the-worker's-lifetime
BUG=572226, 584851
==========
to
==========
Worker: Fix broken GC logic on Dedicated Worker when DOMTimer is set
Before this CL, pending activities on WorkerGlobalScope are reported to the
document in the following cases:
(A1) After DedicatedWorkerThread is initialized, reports that there are no
pending activities regardless of whether they exist.
(A2) After MessageEvent is dispatched, checks a pending activity using
V8GCController and reports a result.
This logic does not work when a DOMTimer is set on initial script evaluation or
on MessageEvent. DOMTimer must be treated as a pending activity[*], but both
(A1) and (A2) do not take care of it (regarding (A2), DOMTimer is not a
ScriptWrappable and V8GCController does not count such an object). As a result,
a worker can be GC'ed even if there is still an active DOM Timer.
After this CL,
(B1) After DedicatedWorkerThread is initialized or MessageEvent is dispatched,
checks whether there is a pending activity including DOMTimer and reports
a result. In addition, if there is a pending activity, schedules a delayed
task to check whether the activity finishes later.
The task is scheduled with some delay. This delay is exponentially increased up
to some extent in order not to frequently run the task.
[*] https://html.spec.whatwg.org/multipage/workers.html#the-worker's-lifetime
BUG=572226, 584851
==========
Description was changed from
==========
Worker: Fix broken GC logic on Dedicated Worker when DOMTimer is set
Before this CL, pending activities on WorkerGlobalScope are reported to the
document in the following cases:
(A1) After DedicatedWorkerThread is initialized, reports that there are no
pending activities regardless of whether they exist.
(A2) After MessageEvent is dispatched, checks a pending activity using
V8GCController and reports a result.
This logic does not work when a DOMTimer is set on initial script evaluation or
on MessageEvent. DOMTimer must be treated as a pending activity[*], but both
(A1) and (A2) do not take care of it (regarding (A2), DOMTimer is not a
ScriptWrappable and V8GCController does not count such an object). As a result,
a worker can be GC'ed even if there is still an active DOM Timer.
After this CL,
(B1) After DedicatedWorkerThread is initialized or MessageEvent is dispatched,
checks whether there is a pending activity including DOMTimer and reports
a result. In addition, if there is a pending activity, schedules a delayed
task to check whether the activity finishes later.
The task is scheduled with some delay. This delay is exponentially increased up
to some extent in order not to frequently run the task.
[*] https://html.spec.whatwg.org/multipage/workers.html#the-worker's-lifetime
BUG=572226, 584851
==========
to
==========
Worker: Fix broken GC logic on Dedicated Worker when DOMTimer is set
V8GC detemines whether a Dedicated Worker can be collectable by checking
pending activities on WorkerGlobalScope. This activity state is managed by
InProcessWorkerMessagingProxy which is on the main thread.
<Before this CL>
Pending activities are reported to the main thread in following cases:
(A1) After DedicatedWorkerThread is initialized, reports that there are no
pending activities regardless of whether they exist.
(A2) After MessageEvent is dispatched, checks a pending activity using
V8GCController and reports a result.
This logic does not work when a DOMTimer is set on initial script evaluation or
on MessageEvent. DOMTimer must be treated as a pending activity[*], but both
(A1) and (A2) do not take care of it (regarding (A2), DOMTimer is not a
ScriptWrappable and V8GCController does not count such an object). As a result,
a worker can be GC'ed even if there is still an active DOM Timer.
<After this CL>
Pending activities are reported in following cases:
(B1) After DedicatedWorkerThread is initialized or MessageEvent is dispatched,
checks whether there is a pending activity including DOMTimer and reports
a result.
(B2) If there is a pending activity on (B1), schedules a delayed task to check
whether the activity finishes later.
The task is scheduled with some delay. This delay is exponentially increased up
to some extent in order not to frequently run the task.
[*] https://html.spec.whatwg.org/multipage/workers.html#the-worker's-lifetime
BUG=572226, 584851
==========
Patchset #1 (id:1) has been deleted
Description was changed from
==========
Worker: Fix broken GC logic on Dedicated Worker when DOMTimer is set
V8GC detemines whether a Dedicated Worker can be collectable by checking
pending activities on WorkerGlobalScope. This activity state is managed by
InProcessWorkerMessagingProxy which is on the main thread.
<Before this CL>
Pending activities are reported to the main thread in following cases:
(A1) After DedicatedWorkerThread is initialized, reports that there are no
pending activities regardless of whether they exist.
(A2) After MessageEvent is dispatched, checks a pending activity using
V8GCController and reports a result.
This logic does not work when a DOMTimer is set on initial script evaluation or
on MessageEvent. DOMTimer must be treated as a pending activity[*], but both
(A1) and (A2) do not take care of it (regarding (A2), DOMTimer is not a
ScriptWrappable and V8GCController does not count such an object). As a result,
a worker can be GC'ed even if there is still an active DOM Timer.
<After this CL>
Pending activities are reported in following cases:
(B1) After DedicatedWorkerThread is initialized or MessageEvent is dispatched,
checks whether there is a pending activity including DOMTimer and reports
a result.
(B2) If there is a pending activity on (B1), schedules a delayed task to check
whether the activity finishes later.
The task is scheduled with some delay. This delay is exponentially increased up
to some extent in order not to frequently run the task.
[*] https://html.spec.whatwg.org/multipage/workers.html#the-worker's-lifetime
BUG=572226, 584851
==========
to
==========
Worker: Fix broken GC logic on Dedicated Worker when DOMTimer is set
V8GC detemines whether a Dedicated Worker can be collectable by checking
pending activities on WorkerGlobalScope. This activity state is managed by
InProcessWorkerMessagingProxy which is on the main thread.
<Before this CL>
Pending activities are reported to the main thread in following cases:
(A1) After DedicatedWorkerThread is initialized, reports that there are no
pending activities regardless of whether they exist.
(A2) After MessageEvent is dispatched, checks a pending activity using
V8GCController and reports a result.
This logic does not work when a DOMTimer is set on initial script evaluation or
on MessageEvent. DOMTimer must be treated as a pending activity[*], but both
(A1) and (A2) do not take care of it (regarding (A2), DOMTimer is not a
ScriptWrappable and V8GCController does not count such an object). As a result,
a worker can be GC'ed even if there is still an active DOM Timer.
<After this CL>
Pending activities are reported in following cases:
(B1) After DedicatedWorkerThread is initialized or MessageEvent is dispatched,
checks whether there is a pending activity including DOMTimer and reports
a result.
(B2) If there is a pending activity on (B1), schedules a delayed task to check
whether the activity finishes later.
The task is scheduled with some delay. This delay is exponentially increased up
to some extent in order not to frequently run the task.
[*] https://html.spec.whatwg.org/multipage/workers.html#the-worker's-lifetime
BUG=572226, 584851
==========
Description was changed from
==========
Worker: Fix broken GC logic on Dedicated Worker when DOMTimer is set
V8GC detemines whether a Dedicated Worker can be collectable by checking
pending activities on WorkerGlobalScope. This activity state is managed by
InProcessWorkerMessagingProxy which is on the main thread.
<Before this CL>
Pending activities are reported to the main thread in following cases:
(A1) After DedicatedWorkerThread is initialized, reports that there are no
pending activities regardless of whether they exist.
(A2) After MessageEvent is dispatched, checks a pending activity using
V8GCController and reports a result.
This logic does not work when a DOMTimer is set on initial script evaluation or
on MessageEvent. DOMTimer must be treated as a pending activity[*], but both
(A1) and (A2) do not take care of it (regarding (A2), DOMTimer is not a
ScriptWrappable and V8GCController does not count such an object). As a result,
a worker can be GC'ed even if there is still an active DOM Timer.
<After this CL>
Pending activities are reported in following cases:
(B1) After DedicatedWorkerThread is initialized or MessageEvent is dispatched,
checks whether there is a pending activity including DOMTimer and reports
a result.
(B2) If there is a pending activity on (B1), schedules a delayed task to check
whether the activity finishes later.
The task is scheduled with some delay. This delay is exponentially increased up
to some extent in order not to frequently run the task.
[*] https://html.spec.whatwg.org/multipage/workers.html#the-worker's-lifetime
BUG=572226, 584851
==========
to
==========
Worker: Fix broken GC logic on Dedicated Worker while DOMTimer is set
V8GC detemines whether a Dedicated Worker can be collectable by checking
pending activities on WorkerGlobalScope. This activity state is managed by
InProcessWorkerMessagingProxy which is on the main thread.
<Before this CL>
Pending activities are reported to the main thread in following cases:
(A1) After DedicatedWorkerThread is initialized, reports that there are no
pending activities regardless of whether they exist.
(A2) After MessageEvent is dispatched, checks a pending activity using
V8GCController and reports a result.
This logic does not work when a DOMTimer is set on initial script evaluation or
on MessageEvent. DOMTimer must be treated as a pending activity[*], but both
(A1) and (A2) do not take care of it (regarding (A2), DOMTimer is not a
ScriptWrappable and V8GCController does not count such an object). As a result,
a worker can be GC'ed even if there is still an active DOM Timer.
<After this CL>
Pending activities are reported in following cases:
(B1) After DedicatedWorkerThread is initialized or MessageEvent is dispatched,
checks whether there is a pending activity including DOMTimer and reports
a result.
(B2) If there is a pending activity on (B1), schedules a delayed task to check
whether the activity finishes later.
The task is scheduled with some delay. This delay is exponentially increased up
to some extent in order not to frequently run the task.
[*] https://html.spec.whatwg.org/multipage/workers.html#the-worker's-lifetime
BUG=572226, 584851
==========
Description was changed from
==========
Worker: Fix broken GC logic on Dedicated Worker while DOMTimer is set
V8GC detemines whether a Dedicated Worker can be collectable by checking
pending activities on WorkerGlobalScope. This activity state is managed by
InProcessWorkerMessagingProxy which is on the main thread.
<Before this CL>
Pending activities are reported to the main thread in following cases:
(A1) After DedicatedWorkerThread is initialized, reports that there are no
pending activities regardless of whether they exist.
(A2) After MessageEvent is dispatched, checks a pending activity using
V8GCController and reports a result.
This logic does not work when a DOMTimer is set on initial script evaluation or
on MessageEvent. DOMTimer must be treated as a pending activity[*], but both
(A1) and (A2) do not take care of it (regarding (A2), DOMTimer is not a
ScriptWrappable and V8GCController does not count such an object). As a result,
a worker can be GC'ed even if there is still an active DOM Timer.
<After this CL>
Pending activities are reported in following cases:
(B1) After DedicatedWorkerThread is initialized or MessageEvent is dispatched,
checks whether there is a pending activity including DOMTimer and reports
a result.
(B2) If there is a pending activity on (B1), schedules a delayed task to check
whether the activity finishes later.
The task is scheduled with some delay. This delay is exponentially increased up
to some extent in order not to frequently run the task.
[*] https://html.spec.whatwg.org/multipage/workers.html#the-worker's-lifetime
BUG=572226, 584851
==========
to
==========
Worker: Fix broken GC logic on Dedicated Worker while DOMTimer is set
V8GC detemins whether a Dedicated Worker can be collectable by checking
pending activities on WorkerGlobalScope. This activity state is managed by
InProcessWorkerMessagingProxy which is on the main thread.
<Before this CL>
Pending activities are reported to the main thread in following cases:
(A1) After DedicatedWorkerThread is initialized, reports that there are no
pending activities regardless of whether they exist.
(A2) After MessageEvent is dispatched, checks a pending activity using
V8GCController and reports a result.
This logic does not work when a DOMTimer is set on initial script evaluation or
on MessageEvent. DOMTimer must be treated as a pending activity[*], but both
(A1) and (A2) do not take care of it (regarding (A2), DOMTimer is not a
ScriptWrappable and V8GCController does not count such an object). As a result,
a worker can be GC'ed even if there is still an active DOM Timer.
<After this CL>
Pending activities are reported in following cases:
(B1) After DedicatedWorkerThread is initialized or MessageEvent is dispatched,
checks whether there is a pending activity including DOMTimer and reports
a result.
(B2) If there is a pending activity on (B1), schedules a delayed task to check
whether the activity finishes later.
The task is scheduled with some delay. This delay is exponentially increased up
to some extent in order not to frequently run the task.
[*] https://html.spec.whatwg.org/multipage/workers.html#the-worker's-lifetime
BUG=572226, 584851
==========
Description was changed from
==========
Worker: Fix broken GC logic on Dedicated Worker while DOMTimer is set
V8GC detemins whether a Dedicated Worker can be collectable by checking
pending activities on WorkerGlobalScope. This activity state is managed by
InProcessWorkerMessagingProxy which is on the main thread.
<Before this CL>
Pending activities are reported to the main thread in following cases:
(A1) After DedicatedWorkerThread is initialized, reports that there are no
pending activities regardless of whether they exist.
(A2) After MessageEvent is dispatched, checks a pending activity using
V8GCController and reports a result.
This logic does not work when a DOMTimer is set on initial script evaluation or
on MessageEvent. DOMTimer must be treated as a pending activity[*], but both
(A1) and (A2) do not take care of it (regarding (A2), DOMTimer is not a
ScriptWrappable and V8GCController does not count such an object). As a result,
a worker can be GC'ed even if there is still an active DOM Timer.
<After this CL>
Pending activities are reported in following cases:
(B1) After DedicatedWorkerThread is initialized or MessageEvent is dispatched,
checks whether there is a pending activity including DOMTimer and reports
a result.
(B2) If there is a pending activity on (B1), schedules a delayed task to check
whether the activity finishes later.
The task is scheduled with some delay. This delay is exponentially increased up
to some extent in order not to frequently run the task.
[*] https://html.spec.whatwg.org/multipage/workers.html#the-worker's-lifetime
BUG=572226, 584851
==========
to
==========
Worker: Fix broken GC logic on Dedicated Worker while DOMTimer is set
V8GC detemines whether a Dedicated Worker can be collectable by checking
pending activities on WorkerGlobalScope. This activity state is managed by
InProcessWorkerMessagingProxy which is on the main thread.
<Before this CL>
Pending activities are reported to the main thread in following cases:
(A1) After DedicatedWorkerThread is initialized, reports that there are no
pending activities regardless of whether they exist.
(A2) After MessageEvent is dispatched, checks a pending activity using
V8GCController and reports a result.
This logic does not work when a DOMTimer is set on initial script evaluation or
on MessageEvent. DOMTimer must be treated as a pending activity[*], but both
(A1) and (A2) do not take care of it (regarding (A2), DOMTimer is not a
ScriptWrappable and V8GCController does not count such an object). As a result,
a worker can be GC'ed even if there is still an active DOM Timer.
<After this CL>
Pending activities are reported in following cases:
(B1) After DedicatedWorkerThread is initialized or MessageEvent is dispatched,
checks whether there is a pending activity including DOMTimer and reports
a result.
(B2) If there is a pending activity on (B1), schedules a delayed task to check
whether the activity finishes later.
The task is scheduled with some delay. This delay is exponentially increased up
to some extent in order not to frequently run the task.
[*] https://html.spec.whatwg.org/multipage/workers.html#the-worker's-lifetime
BUG=572226, 584851
==========
Description was changed from
==========
Worker: Fix broken GC logic on Dedicated Worker while DOMTimer is set
V8GC detemines whether a Dedicated Worker can be collectable by checking
pending activities on WorkerGlobalScope. This activity state is managed by
InProcessWorkerMessagingProxy which is on the main thread.
<Before this CL>
Pending activities are reported to the main thread in following cases:
(A1) After DedicatedWorkerThread is initialized, reports that there are no
pending activities regardless of whether they exist.
(A2) After MessageEvent is dispatched, checks a pending activity using
V8GCController and reports a result.
This logic does not work when a DOMTimer is set on initial script evaluation or
on MessageEvent. DOMTimer must be treated as a pending activity[*], but both
(A1) and (A2) do not take care of it (regarding (A2), DOMTimer is not a
ScriptWrappable and V8GCController does not count such an object). As a result,
a worker can be GC'ed even if there is still an active DOM Timer.
<After this CL>
Pending activities are reported in following cases:
(B1) After DedicatedWorkerThread is initialized or MessageEvent is dispatched,
checks whether there is a pending activity including DOMTimer and reports
a result.
(B2) If there is a pending activity on (B1), schedules a delayed task to check
whether the activity finishes later.
The task is scheduled with some delay. This delay is exponentially increased up
to some extent in order not to frequently run the task.
[*] https://html.spec.whatwg.org/multipage/workers.html#the-worker's-lifetime
BUG=572226, 584851
==========
to
==========
Worker: Fix broken GC logic on Dedicated Worker while DOMTimer is set
V8GC detemines whether a Dedicated Worker can be collectable by checking
pending activities on WorkerGlobalScope. This activity state is managed by
InProcessWorkerMessagingProxy which is on the main thread. This state
management is broken in some situation and this CL fixes it.
<Before this CL>
InProcessWorkerObjectProxy on the worker thread reports pending activities
to the main thread in following cases:
(A1) After DedicatedWorkerThread is initialized, reports that there are no
pending activities regardless of whether they exist.
(A2) After MessageEvent is dispatched, checks a pending activity using
V8GCController and reports a result.
This logic does not work when a DOMTimer is set on initial script evaluation or
on MessageEvent. DOMTimer must be treated as a pending activity[*], but both
(A1) and (A2) do not take care of it (regarding A2, DOMTimer is not a
ScriptWrappable and V8GCController does not count such an object). As a result,
a worker can be GC'ed even if there is still an active timer.
<After this CL>
Pending activities are reported in following cases:
(B1) After DedicatedWorkerThread is initialized or MessageEvent is dispatched,
checks whether there is a pending activity including DOMTimer and reports
a result.
(B2) If there is a pending activity on (B1), schedules a delayed task to check
whether the activity finishes later.
The task is scheduled with some delay. This delay is exponentially increased up
to some extent in order not to frequently run the task.
[*] https://html.spec.whatwg.org/multipage/workers.html#the-worker's-lifetime
BUG=572226, 584851
==========
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:160001) has been deleted
Patchset #1 (id:180001) has been deleted
Patchset #1 (id:200001) has been deleted
Patchset #1 (id:220001) has been deleted
Patchset #1 (id:240001) has been deleted
Patchset #1 (id:260001) has been deleted
Patchset #1 (id:280001) has been deleted
Patchset #1 (id:300001) has been deleted
Patchset #1 (id:320001) has been deleted
Patchset #1 (id:340001) has been deleted
Patchset #2 (id:380001) has been deleted
Patchset #1 (id:360001) has been deleted
Description was changed from
==========
Worker: Fix broken GC logic on Dedicated Worker while DOMTimer is set
V8GC detemines whether a Dedicated Worker can be collectable by checking
pending activities on WorkerGlobalScope. This activity state is managed by
InProcessWorkerMessagingProxy which is on the main thread. This state
management is broken in some situation and this CL fixes it.
<Before this CL>
InProcessWorkerObjectProxy on the worker thread reports pending activities
to the main thread in following cases:
(A1) After DedicatedWorkerThread is initialized, reports that there are no
pending activities regardless of whether they exist.
(A2) After MessageEvent is dispatched, checks a pending activity using
V8GCController and reports a result.
This logic does not work when a DOMTimer is set on initial script evaluation or
on MessageEvent. DOMTimer must be treated as a pending activity[*], but both
(A1) and (A2) do not take care of it (regarding A2, DOMTimer is not a
ScriptWrappable and V8GCController does not count such an object). As a result,
a worker can be GC'ed even if there is still an active timer.
<After this CL>
Pending activities are reported in following cases:
(B1) After DedicatedWorkerThread is initialized or MessageEvent is dispatched,
checks whether there is a pending activity including DOMTimer and reports
a result.
(B2) If there is a pending activity on (B1), schedules a delayed task to check
whether the activity finishes later.
The task is scheduled with some delay. This delay is exponentially increased up
to some extent in order not to frequently run the task.
[*] https://html.spec.whatwg.org/multipage/workers.html#the-worker's-lifetime
BUG=572226, 584851
==========
to
==========
Worker: Fix broken GC logic on Dedicated Worker while DOMTimer is set
V8GC detemines whether a Dedicated Worker can be collectable by checking
pending activities on WorkerGlobalScope. This activity state is managed by
InProcessWorkerMessagingProxy on the main thread. In some situation, this
logic is broken and this CL fixes it.
<Before this CL>
InProcessWorkerObjectProxy on the worker thread reports pending activities
to the main thread in following cases:
(A1) After DedicatedWorkerThread is initialized, reports that there are no
pending activities regardless of whether they exist.
(A2) After MessageEvent is dispatched, checks a pending activity using
V8GCController and reports a result.
This logic does not work when a DOMTimer is set on initial script evaluation or
on MessageEvent. DOMTimer must be treated as a pending activity[*], but both
(A1) and (A2) do not take care of it (regarding A2, DOMTimer is not a
ScriptWrappable and V8GCController does not count such an object). As a result,
a worker can be GC'ed even if there is still an active timer.
<After this CL>
Pending activities are reported in following cases:
(B1) After DedicatedWorkerThread is initialized or MessageEvent is dispatched,
checks whether there is a pending activity including DOMTimer and reports
a result.
(B2) If there is a pending activity on (B1), schedules a delayed task to check
whether the activity finishes later.
The task is scheduled with some delay. This delay is exponentially increased up
to some extent in order not to frequently run the task.
[*] https://html.spec.whatwg.org/multipage/workers.html#the-worker's-lifetime
BUG=572226, 584851
==========
nhiroki@chromium.org changed reviewers: + haraken@chromium.org, kinuko@chromium.org, tzik@chromium.org
Hi, can you review this? +haraken@ (and kinuko@) for the entire CL. +tzik@ for usage of makeCancellable(). Thanks!
lgtm https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp (right): https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:129: m_taskCanceller.cancel(); nit: cancel() is noop if isActive() returns false. You can call it unconditionally.
Sorry about the delayed review. https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp (right): https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:51: const long long kMaxDelayToCheckPendingActivityInMs = 30; // 30 secs If we have the exponential-backoff timer, maybe can we entirely remove the hasPendingActivity check from the postMessage? It looks wasteful to check hasPendingActivity() at every postMessage -- the timer mechanism looks much simpler and more lightweight. https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerMessagingProxy.cpp (right): https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerMessagingProxy.cpp:15: : InProcessWorkerMessagingProxy(worker->getExecutionContext(), worker, workerClients) Maybe is it redundant to pass both |worker| and |worker->getExecutionContext()|?
https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/DedicatedWorkerThread.cpp (right): https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/DedicatedWorkerThread.cpp:69: void DedicatedWorkerThread::postInitialize() nit: can we rename this to... say, didInitializeOnWorkerThread ? (postXxx is confusing, would be nice to have threading suffix) https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.h (right): https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.h:84: InProcessWorkerMessagingProxy(ExecutionContext*, InProcessWorkerBase*, WorkerClients*); Does this start to take ExecutionContext* separately only for testing? Alternatively we could add a test-only private constructor or override getExecutionContext() instead? https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.h:96: bool m_workerThreadHadPendingActivity = false; nit: not a huge fan of exposing field members to subclasses, esp. if it's only for testing class https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp (right): https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:76: void InProcessWorkerObjectProxy::reportPendingActivity(WorkerGlobalScope* workerGlobalScope) This should be probably renamed to checkPendingActivity https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:95: m_currentDelayToCheckPendingActivityInMs = std::min(newDelayInMs, m_maxDelayToCheckPendingActivityInMs); Hmm.. it feels what the class-level comment in the header says about this class's role and what we do here are a bit different.
Patchset #4 (id:460001) has been deleted
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:480001) has been deleted
Thank you for reviewing. Updated. PTAL. https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/DedicatedWorkerThread.cpp (right): https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/DedicatedWorkerThread.cpp:69: void DedicatedWorkerThread::postInitialize() On 2016/08/05 14:55:28, kinuko wrote: > nit: can we rename this to... say, didInitializeOnWorkerThread ? (postXxx is > confusing, would be nice to have threading suffix) Agreed. Renamed. https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.h (right): https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.h:84: InProcessWorkerMessagingProxy(ExecutionContext*, InProcessWorkerBase*, WorkerClients*); On 2016/08/05 14:55:29, kinuko wrote: > Does this start to take ExecutionContext* separately only for testing? > Alternatively we could add a test-only private constructor or override > getExecutionContext() instead? Yes, this change is only for testing. Some tests create a messaging proxy with null InProcessWorkerBase* and that results in a crash on the ctor (|workerObject->getExecutionContext()|), so I made this change. Instead of the approach, I added a private ctor that can accept a null WorkerObjectBase, and then made the proper ctor and tests call it. https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.h:96: bool m_workerThreadHadPendingActivity = false; On 2016/08/05 14:55:29, kinuko wrote: > nit: not a huge fan of exposing field members to subclasses, esp. if it's only > for testing class Moved these fields back to the private section. https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp (right): https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:51: const long long kMaxDelayToCheckPendingActivityInMs = 30; // 30 secs On 2016/08/05 08:20:58, haraken wrote: > > If we have the exponential-backoff timer, maybe can we entirely remove the > hasPendingActivity check from the postMessage? It looks wasteful to check > hasPendingActivity() at every postMessage -- the timer mechanism looks much > simpler and more lightweight. I think it couldn't work in some cases. For example, when a message event initiates some activity, the activity may not be reported to InProcessWorkerMessagingProxy until the next timer is fired. As a result, the worker object can be collected, the pending activity still exists though. Instead, we could reduce the number of the check at postMessage. If the exponential-backoff timer is running when a message arrives, we can skip the check and defer to the timer. The patchset 4 adopts this way (see InprocessWorkerMessagingProxy::isTaskToCheckPendingActivityScheduled()). WDYT? https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:76: void InProcessWorkerObjectProxy::reportPendingActivity(WorkerGlobalScope* workerGlobalScope) On 2016/08/05 14:55:29, kinuko wrote: > This should be probably renamed to checkPendingActivity Done. https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:95: m_currentDelayToCheckPendingActivityInMs = std::min(newDelayInMs, m_maxDelayToCheckPendingActivityInMs); On 2016/08/05 14:55:29, kinuko wrote: > Hmm.. it feels what the class-level comment in the header says about this > class's role and what we do here are a bit different. Slightly updated the header comment. Checking pending activities seems to deviate from responsibilities of InProcessWorkerObject**Proxy**. We could split this feature into a separate class or rename InProcessWorkerObjectProxy to something later. https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:129: m_taskCanceller.cancel(); On 2016/08/05 05:16:06, tzik wrote: > nit: cancel() is noop if isActive() returns false. You can call it > unconditionally. Done. https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerMessagingProxy.cpp (right): https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerMessagingProxy.cpp:15: : InProcessWorkerMessagingProxy(worker->getExecutionContext(), worker, workerClients) On 2016/08/05 08:20:58, haraken wrote: > > Maybe is it redundant to pass both |worker| and |worker->getExecutionContext()|? Reverted this change (see my reply to kinuko's comment in InProcessWorkerMessagingProxy.h)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp (right): https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:51: const long long kMaxDelayToCheckPendingActivityInMs = 30; // 30 secs On 2016/08/08 09:19:14, nhiroki (slow) wrote: > On 2016/08/05 08:20:58, haraken wrote: > > > > If we have the exponential-backoff timer, maybe can we entirely remove the > > hasPendingActivity check from the postMessage? It looks wasteful to check > > hasPendingActivity() at every postMessage -- the timer mechanism looks much > > simpler and more lightweight. > > I think it couldn't work in some cases. For example, when a message event > initiates some activity, the activity may not be reported to > InProcessWorkerMessagingProxy until the next timer is fired. As a result, the > worker object can be collected, the pending activity still exists though. Are you worrying about the following case? 1) The timer reports that # of pending activities is 0. 2) A message event is dispatched. It initiates some pending activity. 3) The main thread runs a GC and collects the worker object. But what happens if the main-thread GC runs between 1) and 2)? Won't the worker object get collected with your CL as well? > Instead, we could reduce the number of the check at postMessage. If the > exponential-backoff timer is running when a message arrives, we can skip the > check and defer to the timer. The patchset 4 adopts this way (see > InprocessWorkerMessagingProxy::isTaskToCheckPendingActivityScheduled()). > > WDYT?
https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp (right): https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:51: const long long kMaxDelayToCheckPendingActivityInMs = 30; // 30 secs On 2016/08/08 11:00:55, haraken wrote: > On 2016/08/08 09:19:14, nhiroki (slow) wrote: > > On 2016/08/05 08:20:58, haraken wrote: > > > > > > If we have the exponential-backoff timer, maybe can we entirely remove the > > > hasPendingActivity check from the postMessage? It looks wasteful to check > > > hasPendingActivity() at every postMessage -- the timer mechanism looks much > > > simpler and more lightweight. > > > > I think it couldn't work in some cases. For example, when a message event > > initiates some activity, the activity may not be reported to > > InProcessWorkerMessagingProxy until the next timer is fired. As a result, the > > worker object can be collected, the pending activity still exists though. > > Are you worrying about the following case? > > 1) The timer reports that # of pending activities is 0. > 2) A message event is dispatched. It initiates some pending activity. > 3) The main thread runs a GC and collects the worker object. Yes, that's right. > But what happens if the main-thread GC runs between 1) and 2)? Won't the worker > object get collected with your CL as well? Regardless of this CL, the main-thread GC does not collect the worker between 1) and 2) because InProcessWorkerMessagingProxy counts inflight messages and prevents GC until the count reaches zero (see m_unconfirmedMessageCount on InProcessWorkerMessagingProxy). > > Instead, we could reduce the number of the check at postMessage. If the > > exponential-backoff timer is running when a message arrives, we can skip the > > check and defer to the timer. The patchset 4 adopts this way (see > > InprocessWorkerMessagingProxy::isTaskToCheckPendingActivityScheduled()). > > > > WDYT?
On 2016/08/08 11:35:34, nhiroki (slow) wrote: > https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp > (right): > > https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:51: const > long long kMaxDelayToCheckPendingActivityInMs = 30; // 30 secs > On 2016/08/08 11:00:55, haraken wrote: > > On 2016/08/08 09:19:14, nhiroki (slow) wrote: > > > On 2016/08/05 08:20:58, haraken wrote: > > > > > > > > If we have the exponential-backoff timer, maybe can we entirely remove the > > > > hasPendingActivity check from the postMessage? It looks wasteful to check > > > > hasPendingActivity() at every postMessage -- the timer mechanism looks > much > > > > simpler and more lightweight. > > > > > > I think it couldn't work in some cases. For example, when a message event > > > initiates some activity, the activity may not be reported to > > > InProcessWorkerMessagingProxy until the next timer is fired. As a result, > the > > > worker object can be collected, the pending activity still exists though. > > > > Are you worrying about the following case? > > > > 1) The timer reports that # of pending activities is 0. > > 2) A message event is dispatched. It initiates some pending activity. > > 3) The main thread runs a GC and collects the worker object. > > Yes, that's right. > > > But what happens if the main-thread GC runs between 1) and 2)? Won't the > worker > > object get collected with your CL as well? > > Regardless of this CL, the main-thread GC does not collect the worker between 1) > and 2) because InProcessWorkerMessagingProxy counts inflight messages and > prevents GC until the count reaches zero (see m_unconfirmedMessageCount on > InProcessWorkerMessagingProxy). ah, thanks. Then would it be an option to just ignore the result of the timer while there's any inflight message? i.e., make the main-thread GC collect the worker object if the timer reports 0 pending activity while there's no inflight message. > > > > Instead, we could reduce the number of the check at postMessage. If the > > > exponential-backoff timer is running when a message arrives, we can skip the > > > check and defer to the timer. The patchset 4 adopts this way (see > > > InprocessWorkerMessagingProxy::isTaskToCheckPendingActivityScheduled()). > > > > > > WDYT?
On 2016/08/08 11:48:32, haraken wrote: > On 2016/08/08 11:35:34, nhiroki (slow) wrote: > > > https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp > > (right): > > > > > https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:51: > const > > long long kMaxDelayToCheckPendingActivityInMs = 30; // 30 secs > > On 2016/08/08 11:00:55, haraken wrote: > > > On 2016/08/08 09:19:14, nhiroki (slow) wrote: > > > > On 2016/08/05 08:20:58, haraken wrote: > > > > > > > > > > If we have the exponential-backoff timer, maybe can we entirely remove > the > > > > > hasPendingActivity check from the postMessage? It looks wasteful to > check > > > > > hasPendingActivity() at every postMessage -- the timer mechanism looks > > much > > > > > simpler and more lightweight. > > > > > > > > I think it couldn't work in some cases. For example, when a message event > > > > initiates some activity, the activity may not be reported to > > > > InProcessWorkerMessagingProxy until the next timer is fired. As a result, > > the > > > > worker object can be collected, the pending activity still exists though. > > > > > > Are you worrying about the following case? > > > > > > 1) The timer reports that # of pending activities is 0. > > > 2) A message event is dispatched. It initiates some pending activity. > > > 3) The main thread runs a GC and collects the worker object. > > > > Yes, that's right. > > > > > But what happens if the main-thread GC runs between 1) and 2)? Won't the > > worker > > > object get collected with your CL as well? > > > > Regardless of this CL, the main-thread GC does not collect the worker between > 1) > > and 2) because InProcessWorkerMessagingProxy counts inflight messages and > > prevents GC until the count reaches zero (see m_unconfirmedMessageCount on > > InProcessWorkerMessagingProxy). > > ah, thanks. Then would it be an option to just ignore the result of the timer > while there's any inflight message? Are you assuming that the exponential-backoff timer is running on the main thread? The timer is running on the worker thread, and inflight messages are counted on the main thread, so the timer cannot confirm whether there is an inflight message before checkPendingActivity() (of course, the worker thread can check the count w/ a lock, but I'd like to avoid it as much as possible in terms of performance and a risk of deadlock). > i.e., make the main-thread GC collect the worker object if the timer reports 0 > pending activity while there's no inflight message. I think this could be the same with the current logic in InProcessWorkerMessagingProxy::hasPendingActivity()? (I might be missing something...?) By the way, what if we move the exponential-backoff timer to the main thread (InProcessWorkerMessagingProxy)? We could more efficiently coordinate the timer and the count of inflight messages to reduce unnecessary checkPendingActivity() calls as following: // InProcessWorkerMessagingProxy will have following fields. bool m_mayHavePendingActivity = true; int m_countOfInflightMessages = 0; bool m_checkingPendingActivity = false; // Algorithm 1) Start the timer when a worker global scope is created on main thread. 2) GC can collect the worker if (!m_mayHavePendingActivity && m_countOfInflightMessages == 0 && !m_checkingPendingActivity) 3) When postMessage is called, InProcessWorkerMessagingProxy... 3-1) sets |m_mayHavePendingActivity| and increments |m_countOfInflightMessages|. 3-2) posts a message to worker thread. 3-3) receives an ack of the message from worker thread. 3-4) decrements |m_countOfInflightMessage| (Important: |m_mayHavePendingActivity| is *NOT* unset at this step so that we can prevent GC until the next timer) 4) When the timer is fired on main thread, InProcessWorkerMessagingProxy... 4-1) aborts following steps if |m_mayHavePendingActivity| is false. 4-2) aborts following steps if |m_countOfInflightMessages| > 0. 4-3) aborts following steps if |m_checkingPendingActivity| is true. 4-4) posts a task to worker thread to check activities and sets |m_checkingPendingActivity|. 4-5) receives a result of the check. 4-5) unsets |m_checkingPendingActivity|. 4-6) unsets |m_mayHavePendingActivity| if the result is 0 and |m_countOfInflightMessages| is 0. If a design doc is helpful to understand/discuss this algorithm, I'll write it up. > > > > > > Instead, we could reduce the number of the check at postMessage. If the > > > > exponential-backoff timer is running when a message arrives, we can skip > the > > > > check and defer to the timer. The patchset 4 adopts this way (see > > > > InprocessWorkerMessagingProxy::isTaskToCheckPendingActivityScheduled()). > > > > > > > > WDYT?
On 2016/08/09 02:26:56, nhiroki wrote: > On 2016/08/08 11:48:32, haraken wrote: > > On 2016/08/08 11:35:34, nhiroki (slow) wrote: > > > > > > https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp > > > (right): > > > > > > > > > https://codereview.chromium.org/2124693002/diff/400001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:51: > > const > > > long long kMaxDelayToCheckPendingActivityInMs = 30; // 30 secs > > > On 2016/08/08 11:00:55, haraken wrote: > > > > On 2016/08/08 09:19:14, nhiroki (slow) wrote: > > > > > On 2016/08/05 08:20:58, haraken wrote: > > > > > > > > > > > > If we have the exponential-backoff timer, maybe can we entirely remove > > the > > > > > > hasPendingActivity check from the postMessage? It looks wasteful to > > check > > > > > > hasPendingActivity() at every postMessage -- the timer mechanism looks > > > much > > > > > > simpler and more lightweight. > > > > > > > > > > I think it couldn't work in some cases. For example, when a message > event > > > > > initiates some activity, the activity may not be reported to > > > > > InProcessWorkerMessagingProxy until the next timer is fired. As a > result, > > > the > > > > > worker object can be collected, the pending activity still exists > though. > > > > > > > > Are you worrying about the following case? > > > > > > > > 1) The timer reports that # of pending activities is 0. > > > > 2) A message event is dispatched. It initiates some pending activity. > > > > 3) The main thread runs a GC and collects the worker object. > > > > > > Yes, that's right. > > > > > > > But what happens if the main-thread GC runs between 1) and 2)? Won't the > > > worker > > > > object get collected with your CL as well? > > > > > > Regardless of this CL, the main-thread GC does not collect the worker > between > > 1) > > > and 2) because InProcessWorkerMessagingProxy counts inflight messages and > > > prevents GC until the count reaches zero (see m_unconfirmedMessageCount on > > > InProcessWorkerMessagingProxy). > > > > ah, thanks. Then would it be an option to just ignore the result of the timer > > while there's any inflight message? > > Are you assuming that the exponential-backoff timer is running on the main > thread? The timer is running on the worker thread, and inflight messages are > counted on the main thread, so the timer cannot confirm whether there is an > inflight message before checkPendingActivity() (of course, the worker thread can > check the count w/ a lock, but I'd like to avoid it as much as possible in terms > of performance and a risk of deadlock). > > > i.e., make the main-thread GC collect the worker object if the timer reports 0 > > pending activity while there's no inflight message. > > I think this could be the same with the current logic in > InProcessWorkerMessagingProxy::hasPendingActivity()? (I might be missing > something...?) > > > By the way, what if we move the exponential-backoff timer to the main thread > (InProcessWorkerMessagingProxy)? We could more efficiently coordinate the timer > and the count of inflight messages to reduce unnecessary checkPendingActivity() > calls as following: > > // InProcessWorkerMessagingProxy will have following fields. > > bool m_mayHavePendingActivity = true; > int m_countOfInflightMessages = 0; > bool m_checkingPendingActivity = false; > > // Algorithm > > 1) Start the timer when a worker global scope is created on main thread. > 2) GC can collect the worker if (!m_mayHavePendingActivity && > m_countOfInflightMessages == 0 && !m_checkingPendingActivity) > 3) When postMessage is called, InProcessWorkerMessagingProxy... > 3-1) sets |m_mayHavePendingActivity| and increments > |m_countOfInflightMessages|. > 3-2) posts a message to worker thread. > 3-3) receives an ack of the message from worker thread. > 3-4) decrements |m_countOfInflightMessage| (Important: > |m_mayHavePendingActivity| is *NOT* unset at this step so that we can prevent GC > until the next timer) > 4) When the timer is fired on main thread, InProcessWorkerMessagingProxy... > 4-1) aborts following steps if |m_mayHavePendingActivity| is false. > 4-2) aborts following steps if |m_countOfInflightMessages| > 0. > 4-3) aborts following steps if |m_checkingPendingActivity| is true. > 4-4) posts a task to worker thread to check activities and sets > |m_checkingPendingActivity|. > 4-5) receives a result of the check. > 4-5) unsets |m_checkingPendingActivity|. > 4-6) unsets |m_mayHavePendingActivity| if the result is 0 and > |m_countOfInflightMessages| is 0. > > If a design doc is helpful to understand/discuss this algorithm, I'll write it > up. As offline chat w/ haraken@, we could remove checkPendingActivity() from message events w/o moving the timer logic to the main thread. I'll try it. (Moving the timer to the main thread may more simplify the logic, but it needs more work. I'll consider it in a separate CL.) > > > > > > > > Instead, we could reduce the number of the check at postMessage. If the > > > > > exponential-backoff timer is running when a message arrives, we can skip > > the > > > > > check and defer to the timer. The patchset 4 adopts this way (see > > > > > InprocessWorkerMessagingProxy::isTaskToCheckPendingActivityScheduled()). > > > > > > > > > > WDYT?
Patchset #5 (id:520001) has been deleted
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Patchset #5 (id:540001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Updated. Can you take another look? The patchset 5 stops costly checkPendingActivity() call on message event dispatching, and the exponential-backoff timer periodically check it instead. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp (right): https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp:141: // A message event is an activity and may initiates another activity. nit: initiates -> initiate https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp:289: void InProcessWorkerMessagingProxy::reportPendingActivity() Looks like this method should be renamed now as it seems to report that we have no pending activities / inflight pending activities are finished ? https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp:304: return (m_unconfirmedMessageCount || m_workerGlobalScopeMayHavePendingActivity) && !m_askedToTerminate; nit: let's write this as following: if (m_askedToTerminate) return false; return m_unconfirmedMessageCount || m_workerGlobalScopeMayHavePendingActivity; https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.h (right): https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.h:74: virtual void reportPendingActivity(); nit: comment that these can be overridden only for testing https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp (right): https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:78: if (m_taskCanceller.isActive()) I might expect that calling this would reset the timer if we have active timer. E.g. if we have multiple post messages in a relatively short period I probably expect that the reportPendingActivity is called in 1 sec (or some default sec) after the last one. If the delay has already become so long it feels it's also good/reasonable to start from shorter delay?
The overall approach looks good. https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.h (right): https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.h:74: virtual void reportPendingActivity(); On 2016/08/09 16:59:09, kinuko wrote: > nit: comment that these can be overridden only for testing Or rename it to reportPendingActivityForTeseting https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp (right): https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:50: const long long kDefaultDelayToCheckPendingActivityInMs = 1; // 1 sec Ms or sec? https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:83: auto result = makeCancellable(std::move(task)); I'm wondering why you cannot simply use a Timer. (I'd prefer limiting the usage of makeCancellable to only low-level classes.) I guess you can implement what you want to implement using startOneShot/cancel/isActive. https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:142: bool hasPendingActivity = workerGlobalScope->hasPendingActivity() || V8GCController::hasPendingActivity(workerGlobalScope->thread()->isolate(), workerGlobalScope); Maybe can we remove the 'workerGlobalScope->hasPendingActivity() ||' check? I think V8GCController::hasPendingActivity returns true if workerGlobalScope->hasPendingActivity() returns true.
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you for reviewing! Updated. PTAL. (I'll revise the CL description later) https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp (right): https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp:141: // A message event is an activity and may initiates another activity. On 2016/08/09 16:59:09, kinuko wrote: > nit: initiates -> initiate Done. https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp:289: void InProcessWorkerMessagingProxy::reportPendingActivity() On 2016/08/09 16:59:09, kinuko wrote: > Looks like this method should be renamed now as it seems to report that we have > no pending activities / inflight pending activities are finished ? Renamed to pendingActivityFinished. https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp:304: return (m_unconfirmedMessageCount || m_workerGlobalScopeMayHavePendingActivity) && !m_askedToTerminate; On 2016/08/09 16:59:09, kinuko wrote: > nit: let's write this as following: > > if (m_askedToTerminate) > return false; > > return m_unconfirmedMessageCount || m_workerGlobalScopeMayHavePendingActivity; Done. https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.h (right): https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.h:74: virtual void reportPendingActivity(); On 2016/08/10 02:33:14, haraken wrote: > On 2016/08/09 16:59:09, kinuko wrote: > > nit: comment that these can be overridden only for testing > > Or rename it to reportPendingActivityForTeseting Added a comment. (Renaming couldn't work well for workerThreadTerminated because it needs to make the caller of this function (ie., WorkerThread::performShutdownOnWorkerThread) 'virtual' to call the test-only function) https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp (right): https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:50: const long long kDefaultDelayToCheckPendingActivityInMs = 1; // 1 sec On 2016/08/10 02:33:14, haraken wrote: > > Ms or sec? Good catch! These should be Ms for postDelayedTask. I shortened delays for local test and forgot to restore them... Btw, based on your other comment, I changed postDelayedTask to a Timer whose intervals are represented in seconds, so I kept these values and renamed them instead. https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:78: if (m_taskCanceller.isActive()) On 2016/08/09 16:59:09, kinuko wrote: > I might expect that calling this would reset the timer if we have active timer. > E.g. if we have multiple post messages in a relatively short period I probably > expect that the reportPendingActivity is called in 1 sec (or some default sec) > after the last one. If the delay has already become so long it feels it's also > good/reasonable to start from shorter delay? Sounds reasonable. I made this reset the timer every time. https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:83: auto result = makeCancellable(std::move(task)); On 2016/08/10 02:33:14, haraken wrote: > > I'm wondering why you cannot simply use a Timer. (I'd prefer limiting the usage > of makeCancellable to only low-level classes.) > > I guess you can implement what you want to implement using > startOneShot/cancel/isActive. It's definitely simpler. Changed postDelayedTask to Timer. https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:142: bool hasPendingActivity = workerGlobalScope->hasPendingActivity() || V8GCController::hasPendingActivity(workerGlobalScope->thread()->isolate(), workerGlobalScope); On 2016/08/10 02:33:14, haraken wrote: > > Maybe can we remove the 'workerGlobalScope->hasPendingActivity() ||' check? I > think V8GCController::hasPendingActivity returns true if > workerGlobalScope->hasPendingActivity() returns true. Hmmm..., I tried removing the call, but V8GCController::hasPendingActivity() didn't call WorkerGlobalScope::hasPendingActivity(). Am I missing something...?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp (right): https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:142: bool hasPendingActivity = workerGlobalScope->hasPendingActivity() || V8GCController::hasPendingActivity(workerGlobalScope->thread()->isolate(), workerGlobalScope); On 2016/08/15 05:52:11, nhiroki wrote: > On 2016/08/10 02:33:14, haraken wrote: > > > > Maybe can we remove the 'workerGlobalScope->hasPendingActivity() ||' check? I > > think V8GCController::hasPendingActivity returns true if > > workerGlobalScope->hasPendingActivity() returns true. > > Hmmm..., I tried removing the call, but V8GCController::hasPendingActivity() > didn't call WorkerGlobalScope::hasPendingActivity(). Am I missing something...? Just to confirm: WorkerGlobalScope has a wrapper, right? Conceptually it looks strange to call hasPendingActivity outside V8GCController, because hasPendingActivity is a mechanism to keep wrappers alive.
https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp (right): https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:142: bool hasPendingActivity = workerGlobalScope->hasPendingActivity() || V8GCController::hasPendingActivity(workerGlobalScope->thread()->isolate(), workerGlobalScope); On 2016/08/15 06:50:39, haraken wrote: > On 2016/08/15 05:52:11, nhiroki wrote: > > On 2016/08/10 02:33:14, haraken wrote: > > > > > > Maybe can we remove the 'workerGlobalScope->hasPendingActivity() ||' check? > I > > > think V8GCController::hasPendingActivity returns true if > > > workerGlobalScope->hasPendingActivity() returns true. > > > > Hmmm..., I tried removing the call, but V8GCController::hasPendingActivity() > > didn't call WorkerGlobalScope::hasPendingActivity(). Am I missing > something...? > > Just to confirm: WorkerGlobalScope has a wrapper, right? > > Conceptually it looks strange to call hasPendingActivity outside V8GCController, > because hasPendingActivity is a mechanism to keep wrappers alive. > Hmmm..., apparently I don't really understand the wrapper mechanism. Can you help me understand it? AFAICS, WorkerGlobalScope has DEFINE_WRAPPERTYPEINFO(), but its wrap() and assiciateWithWrapper() are guarded w/ RELEASE_NOTREACHED. A comment on wrap() says, "WorkerGlobalScope must never be wrapped with wrap method. The global object of ECMAScript environment is used as the wrapper." Does this indicate that the wrapper of WorkerGlobalScope is specially handled in V8GCController? Or, should it be handled as the same as others?
Layout tests for compositor workers fail on NOTREACHED at CompositorWorkerTaskRunnerWrapper::GetTimeDomain() called from TimerBase::startOneShot(). crash log for renderer (pid <unknown>): STDOUT: <empty> STDERR: [1:3:0815/172622:939793055959:FATAL:compositor_worker_scheduler.cc(96)] Check failed: false. STDERR: #0 0x7f6e1d5a61ce base::debug::StackTrace::StackTrace() STDERR: #1 0x7f6e1d60c6cf logging::LogMessage::~LogMessage() STDERR: #2 0x7f6e186a39e2 blink::scheduler::(anonymous namespace)::CompositorWorkerTaskRunnerWrapper::GetTimeDomain() STDERR: #3 0x7f6e186af50d blink::scheduler::WebTaskRunnerImpl::Now() STDERR: #4 0x7f6e186af565 blink::scheduler::WebTaskRunnerImpl::monotonicallyIncreasingVirtualTimeSeconds() STDERR: #5 0x7f6e183765a7 blink::TimerBase::timerMonotonicallyIncreasingTime() STDERR: #6 0x7f6e18376417 blink::TimerBase::start() STDERR: #7 0x7f6e13fd77da blink::TimerBase::startOneShot() STDERR: #8 0x7f6e15321f31 blink::InProcessWorkerObjectProxy::startPendingActivityTimer() STDERR: #9 0x7f6e1532239d blink::InProcessWorkerObjectProxy::didEvaluateWorkerScript() STDERR: #10 0x7f6e15342717 blink::WorkerThread::initializeOnWorkerThread() CompositorWorker does not support Timer...?
haraken@chromium.org changed reviewers: + yukishiino@chromium.org
https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp (right): https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:142: bool hasPendingActivity = workerGlobalScope->hasPendingActivity() || V8GCController::hasPendingActivity(workerGlobalScope->thread()->isolate(), workerGlobalScope); On 2016/08/15 08:01:33, nhiroki wrote: > On 2016/08/15 06:50:39, haraken wrote: > > On 2016/08/15 05:52:11, nhiroki wrote: > > > On 2016/08/10 02:33:14, haraken wrote: > > > > > > > > Maybe can we remove the 'workerGlobalScope->hasPendingActivity() ||' > check? > > I > > > > think V8GCController::hasPendingActivity returns true if > > > > workerGlobalScope->hasPendingActivity() returns true. > > > > > > Hmmm..., I tried removing the call, but V8GCController::hasPendingActivity() > > > didn't call WorkerGlobalScope::hasPendingActivity(). Am I missing > > something...? > > > > Just to confirm: WorkerGlobalScope has a wrapper, right? > > > > Conceptually it looks strange to call hasPendingActivity outside > V8GCController, > > because hasPendingActivity is a mechanism to keep wrappers alive. > > > > Hmmm..., apparently I don't really understand the wrapper mechanism. Can you > help me understand it? > > AFAICS, WorkerGlobalScope has DEFINE_WRAPPERTYPEINFO(), but its wrap() and > assiciateWithWrapper() are guarded w/ RELEASE_NOTREACHED. A comment on wrap() > says, > > "WorkerGlobalScope must never be wrapped with wrap method. The global object > of ECMAScript environment is used as the wrapper." > > Does this indicate that the wrapper of WorkerGlobalScope is specially handled in > V8GCController? Or, should it be handled as the same as others? yukishiino@: Would you help us understand this? Our question is how WorkerGlobalScope's wrapper is handled in V8GCController. This CL is adding WorkerGlobalScope::hasPendingActivity, but it looks like it's not called in V8GCController::hasPendingActivity. Does it mean that WorkerGlobalScope's wrapper is not iterated in the PersistentVisitor?
haraken@chromium.org changed reviewers: + alexclarke@chromium.org
On 2016/08/15 08:29:20, nhiroki wrote: > Layout tests for compositor workers fail on NOTREACHED at > CompositorWorkerTaskRunnerWrapper::GetTimeDomain() called from > TimerBase::startOneShot(). > > crash log for renderer (pid <unknown>): > STDOUT: <empty> > STDERR: [1:3:0815/172622:939793055959:FATAL:compositor_worker_scheduler.cc(96)] > Check failed: false. > STDERR: #0 0x7f6e1d5a61ce base::debug::StackTrace::StackTrace() > STDERR: #1 0x7f6e1d60c6cf logging::LogMessage::~LogMessage() > STDERR: #2 0x7f6e186a39e2 blink::scheduler::(anonymous > namespace)::CompositorWorkerTaskRunnerWrapper::GetTimeDomain() > STDERR: #3 0x7f6e186af50d blink::scheduler::WebTaskRunnerImpl::Now() > STDERR: #4 0x7f6e186af565 > blink::scheduler::WebTaskRunnerImpl::monotonicallyIncreasingVirtualTimeSeconds() > STDERR: #5 0x7f6e183765a7 blink::TimerBase::timerMonotonicallyIncreasingTime() > STDERR: #6 0x7f6e18376417 blink::TimerBase::start() > STDERR: #7 0x7f6e13fd77da blink::TimerBase::startOneShot() > STDERR: #8 0x7f6e15321f31 > blink::InProcessWorkerObjectProxy::startPendingActivityTimer() > STDERR: #9 0x7f6e1532239d > blink::InProcessWorkerObjectProxy::didEvaluateWorkerScript() > STDERR: #10 0x7f6e15342717 blink::WorkerThread::initializeOnWorkerThread() > > CompositorWorker does not support Timer...? Alex: Do you have any idea on this?
Description was changed from
==========
Worker: Fix broken GC logic on Dedicated Worker while DOMTimer is set
V8GC detemines whether a Dedicated Worker can be collectable by checking
pending activities on WorkerGlobalScope. This activity state is managed by
InProcessWorkerMessagingProxy on the main thread. In some situation, this
logic is broken and this CL fixes it.
<Before this CL>
InProcessWorkerObjectProxy on the worker thread reports pending activities
to the main thread in following cases:
(A1) After DedicatedWorkerThread is initialized, reports that there are no
pending activities regardless of whether they exist.
(A2) After MessageEvent is dispatched, checks a pending activity using
V8GCController and reports a result.
This logic does not work when a DOMTimer is set on initial script evaluation or
on MessageEvent. DOMTimer must be treated as a pending activity[*], but both
(A1) and (A2) do not take care of it (regarding A2, DOMTimer is not a
ScriptWrappable and V8GCController does not count such an object). As a result,
a worker can be GC'ed even if there is still an active timer.
<After this CL>
Pending activities are reported in following cases:
(B1) After DedicatedWorkerThread is initialized or MessageEvent is dispatched,
checks whether there is a pending activity including DOMTimer and reports
a result.
(B2) If there is a pending activity on (B1), schedules a delayed task to check
whether the activity finishes later.
The task is scheduled with some delay. This delay is exponentially increased up
to some extent in order not to frequently run the task.
[*] https://html.spec.whatwg.org/multipage/workers.html#the-worker's-lifetime
BUG=572226, 584851
==========
to
==========
Worker: Fix broken GC logic on Dedicated Worker while DOMTimer is set
V8GC detemines whether a Dedicated Worker can be collectable by checking
pending activities on WorkerGlobalScope. This activity state is managed by
InProcessWorkerMessagingProxy on the main thread. In some situation, this
logic is broken and this CL fixes it.
<Before this CL>
InProcessWorkerObjectProxy on the worker thread reports pending activities
to the main thread in following cases:
(A1) After DedicatedWorkerThread is initialized, reports that there are no
pending activities regardless of whether they exist.
(A2) After MessageEvent is dispatched, checks a pending activity using
V8GCController and reports a result.
This logic does not work when a DOMTimer is set on initial script evaluation or
on MessageEvent. DOMTimer must be treated as a pending activity[*], but both
(A1) and (A2) do not take care of it (regarding A2, DOMTimer is not a
ScriptWrappable and V8GCController does not count such an object). As a result,
a worker can be GC'ed even if there is still an active timer.
<After this CL>
Pending activities are reported in following cases:
(B1) After DedicatedWorkerThread is initialized or MessageEvent is dispatched,
checks whether there is a pending activity including DOMTimer and reports
a result.
(B2) If there is a pending activity on (B1), schedules a delayed task to check
whether the activity finishes later.
The task is scheduled with some delay. This delay is exponentially increased up
to some extent in order not to frequently run the task.
[*] https://html.spec.whatwg.org/multipage/workers.html#the-worker's-lifetime
BUG=572226, 584851
==========
alexclarke@google.com changed reviewers: + skyostil@chromium.org
On 2016/08/15 14:37:14, haraken wrote: > On 2016/08/15 08:29:20, nhiroki wrote: > > Layout tests for compositor workers fail on NOTREACHED at > > CompositorWorkerTaskRunnerWrapper::GetTimeDomain() called from > > TimerBase::startOneShot(). > > > > crash log for renderer (pid <unknown>): > > STDOUT: <empty> > > STDERR: > [1:3:0815/172622:939793055959:FATAL:compositor_worker_scheduler.cc(96)] > > Check failed: false. > > STDERR: #0 0x7f6e1d5a61ce base::debug::StackTrace::StackTrace() > > STDERR: #1 0x7f6e1d60c6cf logging::LogMessage::~LogMessage() > > STDERR: #2 0x7f6e186a39e2 blink::scheduler::(anonymous > > namespace)::CompositorWorkerTaskRunnerWrapper::GetTimeDomain() > > STDERR: #3 0x7f6e186af50d blink::scheduler::WebTaskRunnerImpl::Now() > > STDERR: #4 0x7f6e186af565 > > > blink::scheduler::WebTaskRunnerImpl::monotonicallyIncreasingVirtualTimeSeconds() > > STDERR: #5 0x7f6e183765a7 blink::TimerBase::timerMonotonicallyIncreasingTime() > > STDERR: #6 0x7f6e18376417 blink::TimerBase::start() > > STDERR: #7 0x7f6e13fd77da blink::TimerBase::startOneShot() > > STDERR: #8 0x7f6e15321f31 > > blink::InProcessWorkerObjectProxy::startPendingActivityTimer() > > STDERR: #9 0x7f6e1532239d > > blink::InProcessWorkerObjectProxy::didEvaluateWorkerScript() > > STDERR: #10 0x7f6e15342717 blink::WorkerThread::initializeOnWorkerThread() > > > > CompositorWorker does not support Timer...? > > Alex: Do you have any idea on this? Sami it looks like the CompositorWorkerScheduler is biting us again, can we get rid of it?
skyostil@chromium.org changed reviewers: + vollick@chromium.org
On 2016/08/15 15:21:11, alex clarke wrote: > Sami it looks like the CompositorWorkerScheduler is biting us again, can we get > rid of it? We should but IIRC that's blocked on investigating performance regressions that cropped up last time we tried to bring the scheduler to the compositor thread. +vollick@, any chance one of y'all could try that again? In any case, I think for now we can replace the NOTREACHED with a null return.
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
Worker: Fix broken GC logic on Dedicated Worker while DOMTimer is set
V8GC detemines whether a Dedicated Worker can be collectable by checking
pending activities on WorkerGlobalScope. This activity state is managed by
InProcessWorkerMessagingProxy on the main thread. In some situation, this
logic is broken and this CL fixes it.
<Before this CL>
InProcessWorkerObjectProxy on the worker thread reports pending activities
to the main thread in following cases:
(A1) After DedicatedWorkerThread is initialized, reports that there are no
pending activities regardless of whether they exist.
(A2) After MessageEvent is dispatched, checks a pending activity using
V8GCController and reports a result.
This logic does not work when a DOMTimer is set on initial script evaluation or
on MessageEvent. DOMTimer must be treated as a pending activity[*], but both
(A1) and (A2) do not take care of it (regarding A2, DOMTimer is not a
ScriptWrappable and V8GCController does not count such an object). As a result,
a worker can be GC'ed even if there is still an active timer.
<After this CL>
Pending activities are reported in following cases:
(B1) After DedicatedWorkerThread is initialized or MessageEvent is dispatched,
checks whether there is a pending activity including DOMTimer and reports
a result.
(B2) If there is a pending activity on (B1), schedules a delayed task to check
whether the activity finishes later.
The task is scheduled with some delay. This delay is exponentially increased up
to some extent in order not to frequently run the task.
[*] https://html.spec.whatwg.org/multipage/workers.html#the-worker's-lifetime
BUG=572226, 584851
==========
to
==========
Worker: Fix broken GC logic on Dedicated Worker while DOMTimer is set
V8GC detemines whether a Dedicated Worker can be collectable by checking
pending activities on WorkerGlobalScope. This activity state is managed by
InProcessWorkerMessagingProxy on the main thread. In some situation, this
logic is broken and this CL fixes it.
<Before this CL>
InProcessWorkerObjectProxy on the worker thread reports pending activities
to the main thread in following cases:
(1) After DedicatedWorkerThread is initialized, reports that there are no
pending activities regardless of whether they exist.
(2) After MessageEvent is dispatched, checks a pending activity using
V8GCController and reports a result.
This logic does not work when a DOMTimer is set on initial script evaluation or
on MessageEvent. DOMTimer must be treated as a pending activity[*], but both
(1) and (2) do not take care of it (regarding 2, DOMTimer is not a
ScriptWrappable and V8GCController does not count such an object). As a result,
a worker can be GC'ed even if there is still an active timer.
<After this CL>
After initial script evaluation or MessageEvent, the object proxy starts an
exponential backoff timer to periodically check an activity state and reports
to the messaging proxy when all activities are done.
The messaging proxy reports to the GC controller that the worker object is
collectable when all posted messages and pending activities are completed.
[*] https://html.spec.whatwg.org/multipage/workers.html#the-worker's-lifetime
BUG=572226, 584851
==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
haraken@, thank you for routing :) On 2016/08/15 15:42:47, Sami wrote: > On 2016/08/15 15:21:11, alex clarke wrote: > > Sami it looks like the CompositorWorkerScheduler is biting us again, can we > get > > rid of it? > > We should but IIRC that's blocked on investigating performance regressions that > cropped up last time we tried to bring the scheduler to the compositor thread. > +vollick@, any chance one of y'all could try that again? > > In any case, I think for now we can replace the NOTREACHED with a null return. Thank you. I removed the check and confirmed it can work for the tests. Could you take a look at compositor_worker_scheduler.cc as an owner?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp (right): https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:142: bool hasPendingActivity = workerGlobalScope->hasPendingActivity() || V8GCController::hasPendingActivity(workerGlobalScope->thread()->isolate(), workerGlobalScope); On 2016/08/15 14:36:35, haraken wrote: > On 2016/08/15 08:01:33, nhiroki wrote: > > On 2016/08/15 06:50:39, haraken wrote: > > > On 2016/08/15 05:52:11, nhiroki wrote: > > > > On 2016/08/10 02:33:14, haraken wrote: > > > > > > > > > > Maybe can we remove the 'workerGlobalScope->hasPendingActivity() ||' > > check? > > > I > > > > > think V8GCController::hasPendingActivity returns true if > > > > > workerGlobalScope->hasPendingActivity() returns true. > > > > > > > > Hmmm..., I tried removing the call, but > V8GCController::hasPendingActivity() > > > > didn't call WorkerGlobalScope::hasPendingActivity(). Am I missing > > > something...? > > > > > > Just to confirm: WorkerGlobalScope has a wrapper, right? > > > > > > Conceptually it looks strange to call hasPendingActivity outside > > V8GCController, > > > because hasPendingActivity is a mechanism to keep wrappers alive. > > > > > > > Hmmm..., apparently I don't really understand the wrapper mechanism. Can you > > help me understand it? > > > > AFAICS, WorkerGlobalScope has DEFINE_WRAPPERTYPEINFO(), but its wrap() and > > assiciateWithWrapper() are guarded w/ RELEASE_NOTREACHED. A comment on wrap() > > says, > > > > "WorkerGlobalScope must never be wrapped with wrap method. The global object > > of ECMAScript environment is used as the wrapper." > > > > Does this indicate that the wrapper of WorkerGlobalScope is specially handled > in > > V8GCController? Or, should it be handled as the same as others? > > yukishiino@: Would you help us understand this? Our question is how > WorkerGlobalScope's wrapper is handled in V8GCController. This CL is adding > WorkerGlobalScope::hasPendingActivity, but it looks like it's not called in > V8GCController::hasPendingActivity. Does it mean that WorkerGlobalScope's > wrapper is not iterated in the PersistentVisitor? [ActiveScritpWrappable] is missing in WorkerGlobalScope.idl. Already talked with nhiroki@ offline.
On 2016/08/16 06:08:58, Yuki wrote: > https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp > (right): > > https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:142: bool > hasPendingActivity = workerGlobalScope->hasPendingActivity() || > V8GCController::hasPendingActivity(workerGlobalScope->thread()->isolate(), > workerGlobalScope); > On 2016/08/15 14:36:35, haraken wrote: > > On 2016/08/15 08:01:33, nhiroki wrote: > > > On 2016/08/15 06:50:39, haraken wrote: > > > > On 2016/08/15 05:52:11, nhiroki wrote: > > > > > On 2016/08/10 02:33:14, haraken wrote: > > > > > > > > > > > > Maybe can we remove the 'workerGlobalScope->hasPendingActivity() ||' > > > check? > > > > I > > > > > > think V8GCController::hasPendingActivity returns true if > > > > > > workerGlobalScope->hasPendingActivity() returns true. > > > > > > > > > > Hmmm..., I tried removing the call, but > > V8GCController::hasPendingActivity() > > > > > didn't call WorkerGlobalScope::hasPendingActivity(). Am I missing > > > > something...? > > > > > > > > Just to confirm: WorkerGlobalScope has a wrapper, right? > > > > > > > > Conceptually it looks strange to call hasPendingActivity outside > > > V8GCController, > > > > because hasPendingActivity is a mechanism to keep wrappers alive. > > > > > > > > > > Hmmm..., apparently I don't really understand the wrapper mechanism. Can you > > > help me understand it? > > > > > > AFAICS, WorkerGlobalScope has DEFINE_WRAPPERTYPEINFO(), but its wrap() and > > > assiciateWithWrapper() are guarded w/ RELEASE_NOTREACHED. A comment on > wrap() > > > says, > > > > > > "WorkerGlobalScope must never be wrapped with wrap method. The global > object > > > of ECMAScript environment is used as the wrapper." > > > > > > Does this indicate that the wrapper of WorkerGlobalScope is specially > handled > > in > > > V8GCController? Or, should it be handled as the same as others? > > > > yukishiino@: Would you help us understand this? Our question is how > > WorkerGlobalScope's wrapper is handled in V8GCController. This CL is adding > > WorkerGlobalScope::hasPendingActivity, but it looks like it's not called in > > V8GCController::hasPendingActivity. Does it mean that WorkerGlobalScope's > > wrapper is not iterated in the PersistentVisitor? > > [ActiveScritpWrappable] is missing in WorkerGlobalScope.idl. > Already talked with nhiroki@ offline. Oh... nice catch. Is there any way to catch the error at runtime? (e.g., add some DCHECK)
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you! Updated. PTAL. https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp (right): https://codereview.chromium.org/2124693002/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:142: bool hasPendingActivity = workerGlobalScope->hasPendingActivity() || V8GCController::hasPendingActivity(workerGlobalScope->thread()->isolate(), workerGlobalScope); On 2016/08/16 06:08:58, Yuki wrote: > On 2016/08/15 14:36:35, haraken wrote: > > On 2016/08/15 08:01:33, nhiroki wrote: > > > On 2016/08/15 06:50:39, haraken wrote: > > > > On 2016/08/15 05:52:11, nhiroki wrote: > > > > > On 2016/08/10 02:33:14, haraken wrote: > > > > > > > > > > > > Maybe can we remove the 'workerGlobalScope->hasPendingActivity() ||' > > > check? > > > > I > > > > > > think V8GCController::hasPendingActivity returns true if > > > > > > workerGlobalScope->hasPendingActivity() returns true. > > > > > > > > > > Hmmm..., I tried removing the call, but > > V8GCController::hasPendingActivity() > > > > > didn't call WorkerGlobalScope::hasPendingActivity(). Am I missing > > > > something...? > > > > > > > > Just to confirm: WorkerGlobalScope has a wrapper, right? > > > > > > > > Conceptually it looks strange to call hasPendingActivity outside > > > V8GCController, > > > > because hasPendingActivity is a mechanism to keep wrappers alive. > > > > > > > > > > Hmmm..., apparently I don't really understand the wrapper mechanism. Can you > > > help me understand it? > > > > > > AFAICS, WorkerGlobalScope has DEFINE_WRAPPERTYPEINFO(), but its wrap() and > > > assiciateWithWrapper() are guarded w/ RELEASE_NOTREACHED. A comment on > wrap() > > > says, > > > > > > "WorkerGlobalScope must never be wrapped with wrap method. The global > object > > > of ECMAScript environment is used as the wrapper." > > > > > > Does this indicate that the wrapper of WorkerGlobalScope is specially > handled > > in > > > V8GCController? Or, should it be handled as the same as others? > > > > yukishiino@: Would you help us understand this? Our question is how > > WorkerGlobalScope's wrapper is handled in V8GCController. This CL is adding > > WorkerGlobalScope::hasPendingActivity, but it looks like it's not called in > > V8GCController::hasPendingActivity. Does it mean that WorkerGlobalScope's > > wrapper is not iterated in the PersistentVisitor? > > [ActiveScritpWrappable] is missing in WorkerGlobalScope.idl. > Already talked with nhiroki@ offline. Thank you yukishiino@! I confirmed WorkerGlobalScope::hasPendingActivity() is appropriately called from V8GCController after adding the annotation :)
On 2016/08/16 06:10:57, haraken wrote: > Oh... nice catch. Is there any way to catch the error at runtime? (e.g., add > some DCHECK) Here you are. https://codereview.chromium.org/2250063002/ Could you take a look? (Three IDLs are missing [ActiveScriptWrappable]!!!)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
third_party/WebKit/Source/platform/scheduler/child/compositor_worker_scheduler.cc lgtm
Mostly looks good. https://codereview.chromium.org/2124693002/diff/680001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp (right): https://codereview.chromium.org/2124693002/diff/680001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp:298: m_workerGlobalScopeMayHavePendingActivity = false; I'm wondering why you need both m_workerGlobalScopeMayHavePendingActivity and m_unconfirmedMessageCount. I guess m_unconfirmedMessageCount would be enough. Either way, you can work on it in a follow up. https://codereview.chromium.org/2124693002/diff/680001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp (right): https://codereview.chromium.org/2124693002/diff/680001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:81: m_timer->startOneShot(m_currentIntervalInSec, BLINK_FROM_HERE); Given that startPendingActivityTimer is called at every message event, won't this cause a timer flood? I'd prefer not scheduling the next timer if m_timer->isActive() returns true. (i.e., return immediately after line 79) https://codereview.chromium.org/2124693002/diff/680001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:145: { Shall we add DCHECK(m_workerGlobalScope) (if you want to keep using WeakPersistent)? https://codereview.chromium.org/2124693002/diff/680001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.h (right): https://codereview.chromium.org/2124693002/diff/680001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.h:103: WeakPersistent<WorkerGlobalScope> m_workerGlobalScope; I'm wondering why you can just use Persistent<WorkerGlobalScope>. I don't think the strong Persistent will cause memory leak because InProcessWorkerObjectProxy::willDestroyWorkerGlobalScope explicitly clears out m_workerGlobalScope.
https://codereview.chromium.org/2124693002/diff/680001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/DedicatedWorkerTest.cpp (right): https://codereview.chromium.org/2124693002/diff/680001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/DedicatedWorkerTest.cpp:35: }; nit: No need of trailing ; https://codereview.chromium.org/2124693002/diff/680001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp (right): https://codereview.chromium.org/2124693002/diff/680001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:81: m_timer->startOneShot(m_currentIntervalInSec, BLINK_FROM_HERE); I think we could possibly restart the timer only if the current interval length is longer than kDefault (or some threshold). In that way we won't be flooding & we could check the pending activity relatively timely (I also think it might be worth restarting the timer when the worker posts a message to the document as it often means worker has done some pending activity... but that can be explored separately / unrelated to existing bugs) https://codereview.chromium.org/2124693002/diff/680001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.h (right): https://codereview.chromium.org/2124693002/diff/680001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.h:95: // global scope. The interval of the timer is initially set to nit: the current interval duration is m_currentIntervalInSec and it's initially set to kDefaultIntervalInSec ? https://codereview.chromium.org/2124693002/diff/680001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.h:98: // when the worker global scoped is destroyed. maybe comment that m_maxIntervalInSec is usually kMaxIntervalInSec but made as a member variable for testing
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Thank you for your comments! Updated. https://codereview.chromium.org/2124693002/diff/680001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/DedicatedWorkerTest.cpp (right): https://codereview.chromium.org/2124693002/diff/680001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/DedicatedWorkerTest.cpp:35: }; On 2016/08/16 21:37:38, kinuko wrote: > nit: No need of trailing ; Removed. Good eye :) https://codereview.chromium.org/2124693002/diff/680001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp (right): https://codereview.chromium.org/2124693002/diff/680001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp:298: m_workerGlobalScopeMayHavePendingActivity = false; On 2016/08/16 12:17:40, haraken wrote: > > I'm wondering why you need both m_workerGlobalScopeMayHavePendingActivity and > m_unconfirmedMessageCount. I guess m_unconfirmedMessageCount would be enough. > > Either way, you can work on it in a follow up. I'm not sure how m_unconfirmedMessageCount works without m_workerGlobalScopeMayHavePendingActivity. In that way, when does ObjectProxy send an ack for a posted message to MessagingProxy? If we naively remove m_workerGlobalScopeMayHavePendingActivity from the current CL, it would fail as follow: 1) MessageProxy posts a message and increments the count (0->1). 2) ObjectProxy dispatches a message event, which schedules a DOMTimer, and sends an ack. 3) MessageProxy receives the ack and decrements the count (1->0). 4) The worker object is GC'ed. 5) The DOMTimer is never fired. If ObjectProxy sends an ack when the exponential-backoff timer is fired instead of when the message event is dispatched, memory leak could happen: 1) MessageProxy posts a message and increments the count (0->1). 2) ObjectProxy dispatches a message event, which schedules a DOMTimer. 3) MessageProxy posts another message and increments the count (1->2). 4) ObjectProxy dispatches another message event. 5) DOMTimer is fired. All activities are done at this point. 6) The exponential-backoff timer is fired. 7) ObjectProxy makes sure there are no pending activities and sends an ack. 8) The exponential-backoff timer stops. 9) MessageProxy receives the ack and decrements the count (2->1). 10) MessageProxy never receives the second ack and the worker is leaked. https://codereview.chromium.org/2124693002/diff/680001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp (right): https://codereview.chromium.org/2124693002/diff/680001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:81: m_timer->startOneShot(m_currentIntervalInSec, BLINK_FROM_HERE); On 2016/08/16 21:37:38, kinuko wrote: > I think we could possibly restart the timer only if the current interval length > is longer than kDefault (or some threshold). In that way we won't be flooding & > we could check the pending activity relatively timely On second thought, it'd be preferable just to return if the timer is active regardless of the current interval duration. I'm considering following cases... Case 1) If there is a long-running activity (eg. repeated timer) and a message event is newly dispatched, the timer needs to run until completion of the long running activity after all, so the current interval wouldn't have to be reset on the message event. Case 2) If a few of short-running message events are dispatched, we could expect that all activities are done before the current interval is too lengthened and activities would timely be checked without the reset. Case 3) If a bunch of short-running message events are dispatched, we could consider them as one long-running activity and handle this case like case 1. What do you think? > (I also think it might be worth restarting the timer when the worker posts a > message to the document as it often means worker has done some pending > activity... but that can be explored separately / unrelated to existing bugs) Acknowledged. I'll consider it in a separate CL/issue. https://codereview.chromium.org/2124693002/diff/680001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:145: { On 2016/08/16 12:17:40, haraken wrote: > > Shall we add DCHECK(m_workerGlobalScope) (if you want to keep using > WeakPersistent)? WONTFIX because I changed WeakPersistent to Persistent. https://codereview.chromium.org/2124693002/diff/680001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.h (right): https://codereview.chromium.org/2124693002/diff/680001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.h:95: // global scope. The interval of the timer is initially set to On 2016/08/16 21:37:38, kinuko wrote: > nit: the current interval duration is m_currentIntervalInSec and it's initially > set to kDefaultIntervalInSec ? Done. https://codereview.chromium.org/2124693002/diff/680001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.h:98: // when the worker global scoped is destroyed. On 2016/08/16 21:37:38, kinuko wrote: > maybe comment that m_maxIntervalInSec is usually kMaxIntervalInSec but made as a > member variable for testing Done. https://codereview.chromium.org/2124693002/diff/680001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.h:103: WeakPersistent<WorkerGlobalScope> m_workerGlobalScope; On 2016/08/16 12:17:40, haraken wrote: > > I'm wondering why you can just use Persistent<WorkerGlobalScope>. I don't think > the strong Persistent will cause memory leak because > InProcessWorkerObjectProxy::willDestroyWorkerGlobalScope explicitly clears out > m_workerGlobalScope. <TL;DR> Changed WeakPersistent to Persistent. <Long braindump-ish reply> I'm a bit unsure the usage policy of strong/weak Persistent. As you commented, there is no risk of memory leak by strong Persistent, but, on the other hand, using WeakPersistent seems to more clarify the ownership/lifecycle of the object. Is there any guide about it? ... ah, I just noticed that BlinkGCAPIReference.md mentions it: "The use of `WeakMember<T>` incurs some overhead in garbage collector's performance. Use it sparingly. Usually, weak members are not necessary at all, because reference cycles with members are allowed. More specifically, `WeakMember<T>` should be used only if the owner of a weak member can outlive the pointed object. Otherwise, `Member<T>` should be used." OK, using strong Persistent for avoiding overhead sounds reasonable. I changed WeakPersistent to Persistent.
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Patchset #16 (id:780001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2124693002/diff/760001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp (right): https://codereview.chromium.org/2124693002/diff/760001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:85: if (m_currentIntervalInSec == kDefaultIntervalInSec) Does this branch really work? You're updating m_currentIntervalInSec to m_currentIntervalInSec*1.5 at line 94. Hence, this branch wouldn't be helpful to avoid the timer flood. Given the overhead of creating cancellable tasks (which we're discussing in another thread), I want to take a conservative approach here; i.e., if it's not easy to avoid the timer flood, just return immediately when m_timer->isActive returns true. It may cause a 30 sec of delay in the worst case, but it would be better than causing the timer flood. Maybe we could introduce another timer and do something like this: void startPendingActivityTimer() { if (!m_anotherTimer.isActive()) m_anotherTimer.startOneShot(1000); } void anotherTimerFired() { m_currentIntervalInSec = kDefaultIntervalInSec; m_timer->startOneShot(m_currentIntervalInSec); m_currentIntervalInSec = std::min(...); }
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2124693002/diff/680001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp (right): https://codereview.chromium.org/2124693002/diff/680001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:81: m_timer->startOneShot(m_currentIntervalInSec, BLINK_FROM_HERE); On 2016/08/17 04:45:32, nhiroki wrote: > On second thought, it'd be preferable just to return if the timer is active > regardless of the current interval duration. I'm considering following cases... One possible scenario I considered was a message from document could be for canceling or changing some ongoing activity, that feels plausible. If we consider keeping workers possibly longer is just fine I'm ok with returning immediately when isActive is true. After all 30 sec is not that long.
https://codereview.chromium.org/2124693002/diff/760001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp (right): https://codereview.chromium.org/2124693002/diff/760001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:85: if (m_currentIntervalInSec == kDefaultIntervalInSec) m_currentIntervalInSec is actually a next interval sec so it won't work. We could expose something similar to base::Timer::GetCurrentDelay() in Timer.h (having that tends to be useful anyways) and check it to make this pattern saner, but as I wrote I'm fine just returning immediately too. (Separately-- reseting an ongoing timer feels relatively a common pattern, if it's costly it's probably worth commenting that in Timer.h until we fix it)
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you for reviewing. Updated. https://codereview.chromium.org/2124693002/diff/680001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp (right): https://codereview.chromium.org/2124693002/diff/680001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:81: m_timer->startOneShot(m_currentIntervalInSec, BLINK_FROM_HERE); On 2016/08/17 22:15:08, kinuko wrote: > On 2016/08/17 04:45:32, nhiroki wrote: > > On second thought, it'd be preferable just to return if the timer is active > > regardless of the current interval duration. I'm considering following > cases... > > One possible scenario I considered was a message from document could be for > canceling or changing some ongoing activity, that feels plausible. If we > consider keeping workers possibly longer is just fine I'm ok with returning > immediately when isActive is true. After all 30 sec is not that long. Acknowledged. https://codereview.chromium.org/2124693002/diff/760001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp (right): https://codereview.chromium.org/2124693002/diff/760001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:85: if (m_currentIntervalInSec == kDefaultIntervalInSec) On 2016/08/17 06:30:47, haraken wrote: > > Does this branch really work? You're updating m_currentIntervalInSec to > m_currentIntervalInSec*1.5 at line 94. Hence, this branch wouldn't be helpful to > avoid the timer flood. Ooops... yes, this doesn't work. > Given the overhead of creating cancellable tasks (which we're discussing in > another thread), I want to take a conservative approach here; i.e., if it's not > easy to avoid the timer flood, just return immediately when m_timer->isActive > returns true. It may cause a 30 sec of delay in the worst case, but it would be > better than causing the timer flood. Agreed. Changed this function to immediately return if isActive() returns true. > Maybe we could introduce another timer and do something like this: > > void startPendingActivityTimer() { > if (!m_anotherTimer.isActive()) > m_anotherTimer.startOneShot(1000); > } > > void anotherTimerFired() { > m_currentIntervalInSec = kDefaultIntervalInSec; > m_timer->startOneShot(m_currentIntervalInSec); > m_currentIntervalInSec = std::min(...); > } https://codereview.chromium.org/2124693002/diff/760001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:85: if (m_currentIntervalInSec == kDefaultIntervalInSec) On 2016/08/17 22:50:10, kinuko wrote: > m_currentIntervalInSec is actually a next interval sec so it won't work. We > could expose something similar to base::Timer::GetCurrentDelay() in Timer.h > (having that tends to be useful anyways) and check it to make this pattern > saner, but as I wrote I'm fine just returning immediately too. > > (Separately-- reseting an ongoing timer feels relatively a common pattern, if > it's costly it's probably worth commenting that in Timer.h until we fix it) Acknowledged.
LGTM on my side.
https://codereview.chromium.org/2124693002/diff/800001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp (right): https://codereview.chromium.org/2124693002/diff/800001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:82: if (m_timer->isActive()) We don't re-adjust the next interval either? As I wrote I think there could be a scenario where posting a message could change the pending activity behavior. https://codereview.chromium.org/2124693002/diff/800001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.h (right): https://codereview.chromium.org/2124693002/diff/800001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.h:102: // The current interval duration of the timer. This is initially set to current -> next ...?
Let me note again that if restarting a timer should be discouraged it'd be better to have a comment about that in Timer.h. Resetting/restarting a timer is relatively a common pattern with base::Timer, I can imagine people could easily fall back to the pattern otherwise
https://codereview.chromium.org/2124693002/diff/800001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp (right): https://codereview.chromium.org/2124693002/diff/800001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:82: if (m_timer->isActive()) On 2016/08/18 01:26:50, kinuko wrote: > We don't re-adjust the next interval either? As I wrote I think there could be > a scenario where posting a message could change the pending activity behavior. Good point. We need to update m_currentIntervalInSec to the default value. Just to clarify: Do you prefer resetting the existing timer and scheduler another timer with the new interval (i.e., default value) after updating m_currentIntervalInSec? What I wrote in the previous review is not to schedule another timer for simplicity at the moment.
On 2016/08/18 01:31:34, kinuko wrote: > Let me note again that if restarting a timer should be discouraged it'd be > better to have a comment about that in Timer.h. Resetting/restarting a timer is > relatively a common pattern with base::Timer, I can imagine people could easily > fall back to the pattern otherwise I'll add a comment.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2124693002/diff/800001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp (right): https://codereview.chromium.org/2124693002/diff/800001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:82: if (m_timer->isActive()) On 2016/08/18 01:31:44, haraken wrote: > On 2016/08/18 01:26:50, kinuko wrote: > > We don't re-adjust the next interval either? As I wrote I think there could > be > > a scenario where posting a message could change the pending activity behavior. > > Good point. We need to update m_currentIntervalInSec to the default value. > > Just to clarify: Do you prefer resetting the existing timer and scheduler > another timer with the new interval (i.e., default value) after updating > m_currentIntervalInSec? What I wrote in the previous review is not to schedule > another timer for simplicity at the moment. Thanks for confirming. I think I'm fine with the behavior too for this case.
On 2016/08/18 01:32:42, haraken wrote: > On 2016/08/18 01:31:34, kinuko wrote: > > Let me note again that if restarting a timer should be discouraged it'd be > > better to have a comment about that in Timer.h. Resetting/restarting a timer > is > > relatively a common pattern with base::Timer, I can imagine people could > easily > > fall back to the pattern otherwise > > I'll add a comment. Thanks!
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you for the clarification! Updated the patch to reset the interval duration if isActive() returns true. https://codereview.chromium.org/2124693002/diff/800001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.h (right): https://codereview.chromium.org/2124693002/diff/800001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.h:102: // The current interval duration of the timer. This is initially set to On 2016/08/18 01:26:50, kinuko wrote: > current -> next ...? Done.
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
(After alexclarke@ lands his cancellable timer patch, restarting timers should be considered fairly cheap. Of course it's still best to avoid it if possible :)
lgtm On 2016/08/18 15:09:13, Sami wrote: > (After alexclarke@ lands his cancellable timer patch, restarting timers should > be considered fairly cheap. Of course it's still best to avoid it if possible :) Awesome :D
Thank you for carefully/patiently reviewing this :)
Description was changed from
==========
Worker: Fix broken GC logic on Dedicated Worker while DOMTimer is set
V8GC detemines whether a Dedicated Worker can be collectable by checking
pending activities on WorkerGlobalScope. This activity state is managed by
InProcessWorkerMessagingProxy on the main thread. In some situation, this
logic is broken and this CL fixes it.
<Before this CL>
InProcessWorkerObjectProxy on the worker thread reports pending activities
to the main thread in following cases:
(1) After DedicatedWorkerThread is initialized, reports that there are no
pending activities regardless of whether they exist.
(2) After MessageEvent is dispatched, checks a pending activity using
V8GCController and reports a result.
This logic does not work when a DOMTimer is set on initial script evaluation or
on MessageEvent. DOMTimer must be treated as a pending activity[*], but both
(1) and (2) do not take care of it (regarding 2, DOMTimer is not a
ScriptWrappable and V8GCController does not count such an object). As a result,
a worker can be GC'ed even if there is still an active timer.
<After this CL>
After initial script evaluation or MessageEvent, the object proxy starts an
exponential backoff timer to periodically check an activity state and reports
to the messaging proxy when all activities are done.
The messaging proxy reports to the GC controller that the worker object is
collectable when all posted messages and pending activities are completed.
[*] https://html.spec.whatwg.org/multipage/workers.html#the-worker's-lifetime
BUG=572226, 584851
==========
to
==========
Worker: Fix broken GC logic on Dedicated Worker while DOMTimer is set
V8GC detemines whether a Dedicated Worker can be collectable by checking
pending activities on WorkerGlobalScope. This activity state is managed by
InProcessWorkerMessagingProxy on the main thread. In some situation, this
logic is broken and this CL fixes it.
<Before this CL>
InProcessWorkerObjectProxy on the worker thread reports pending activities
to the main thread in following cases:
(1) After DedicatedWorkerThread is initialized, reports that there are no
pending activities regardless of whether they exist.
(2) After MessageEvent is dispatched, checks a pending activity using
V8GCController and reports a result.
This logic does not work when a DOMTimer is set on initial script evaluation or
on MessageEvent. DOMTimer must be treated as a pending activity[*], but both
(1) and (2) do not take care of it (regarding 2, DOMTimer is not a
ScriptWrappable and V8GCController does not count such an object). As a result,
a worker can be GC'ed even if there is still an active timer.
<After this CL>
After initial script evaluation or MessageEvent, the object proxy starts an
exponential backoff timer to periodically check an activity state and reports
to the messaging proxy when all activities are done.
The messaging proxy reports to the GC that the worker object is collectable
after all posted messages and pending activities are completed.
[*] https://html.spec.whatwg.org/multipage/workers.html#the-worker's-lifetime
BUG=572226, 584851
==========
The CQ bit was checked by nhiroki@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tzik@chromium.org, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/2124693002/#ps820001 (title: "reset the next interval duration")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by nhiroki@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org, tzik@chromium.org, kinuko@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2124693002/#ps840001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by nhiroki@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from
==========
Worker: Fix broken GC logic on Dedicated Worker while DOMTimer is set
V8GC detemines whether a Dedicated Worker can be collectable by checking
pending activities on WorkerGlobalScope. This activity state is managed by
InProcessWorkerMessagingProxy on the main thread. In some situation, this
logic is broken and this CL fixes it.
<Before this CL>
InProcessWorkerObjectProxy on the worker thread reports pending activities
to the main thread in following cases:
(1) After DedicatedWorkerThread is initialized, reports that there are no
pending activities regardless of whether they exist.
(2) After MessageEvent is dispatched, checks a pending activity using
V8GCController and reports a result.
This logic does not work when a DOMTimer is set on initial script evaluation or
on MessageEvent. DOMTimer must be treated as a pending activity[*], but both
(1) and (2) do not take care of it (regarding 2, DOMTimer is not a
ScriptWrappable and V8GCController does not count such an object). As a result,
a worker can be GC'ed even if there is still an active timer.
<After this CL>
After initial script evaluation or MessageEvent, the object proxy starts an
exponential backoff timer to periodically check an activity state and reports
to the messaging proxy when all activities are done.
The messaging proxy reports to the GC that the worker object is collectable
after all posted messages and pending activities are completed.
[*] https://html.spec.whatwg.org/multipage/workers.html#the-worker's-lifetime
BUG=572226, 584851
==========
to
==========
Worker: Fix broken GC logic on Dedicated Worker while DOMTimer is set
V8GC detemines whether a Dedicated Worker can be collectable by checking
pending activities on WorkerGlobalScope. This activity state is managed by
InProcessWorkerMessagingProxy on the main thread. In some situation, this
logic is broken and this CL fixes it.
<Before this CL>
InProcessWorkerObjectProxy on the worker thread reports pending activities
to the main thread in following cases:
(1) After DedicatedWorkerThread is initialized, reports that there are no
pending activities regardless of whether they exist.
(2) After MessageEvent is dispatched, checks a pending activity using
V8GCController and reports a result.
This logic does not work when a DOMTimer is set on initial script evaluation or
on MessageEvent. DOMTimer must be treated as a pending activity[*], but both
(1) and (2) do not take care of it (regarding 2, DOMTimer is not a
ScriptWrappable and V8GCController does not count such an object). As a result,
a worker can be GC'ed even if there is still an active timer.
<After this CL>
After initial script evaluation or MessageEvent, the object proxy starts an
exponential backoff timer to periodically check an activity state and reports
to the messaging proxy when all activities are done.
The messaging proxy reports to the GC that the worker object is collectable
after all posted messages and pending activities are completed.
[*] https://html.spec.whatwg.org/multipage/workers.html#the-worker's-lifetime
BUG=572226, 584851
==========
Message was sent while issue was closed.
Committed patchset #18 (id:840001)
Message was sent while issue was closed.
Description was changed from
==========
Worker: Fix broken GC logic on Dedicated Worker while DOMTimer is set
V8GC detemines whether a Dedicated Worker can be collectable by checking
pending activities on WorkerGlobalScope. This activity state is managed by
InProcessWorkerMessagingProxy on the main thread. In some situation, this
logic is broken and this CL fixes it.
<Before this CL>
InProcessWorkerObjectProxy on the worker thread reports pending activities
to the main thread in following cases:
(1) After DedicatedWorkerThread is initialized, reports that there are no
pending activities regardless of whether they exist.
(2) After MessageEvent is dispatched, checks a pending activity using
V8GCController and reports a result.
This logic does not work when a DOMTimer is set on initial script evaluation or
on MessageEvent. DOMTimer must be treated as a pending activity[*], but both
(1) and (2) do not take care of it (regarding 2, DOMTimer is not a
ScriptWrappable and V8GCController does not count such an object). As a result,
a worker can be GC'ed even if there is still an active timer.
<After this CL>
After initial script evaluation or MessageEvent, the object proxy starts an
exponential backoff timer to periodically check an activity state and reports
to the messaging proxy when all activities are done.
The messaging proxy reports to the GC that the worker object is collectable
after all posted messages and pending activities are completed.
[*] https://html.spec.whatwg.org/multipage/workers.html#the-worker's-lifetime
BUG=572226, 584851
==========
to
==========
Worker: Fix broken GC logic on Dedicated Worker while DOMTimer is set
V8GC detemines whether a Dedicated Worker can be collectable by checking
pending activities on WorkerGlobalScope. This activity state is managed by
InProcessWorkerMessagingProxy on the main thread. In some situation, this
logic is broken and this CL fixes it.
<Before this CL>
InProcessWorkerObjectProxy on the worker thread reports pending activities
to the main thread in following cases:
(1) After DedicatedWorkerThread is initialized, reports that there are no
pending activities regardless of whether they exist.
(2) After MessageEvent is dispatched, checks a pending activity using
V8GCController and reports a result.
This logic does not work when a DOMTimer is set on initial script evaluation or
on MessageEvent. DOMTimer must be treated as a pending activity[*], but both
(1) and (2) do not take care of it (regarding 2, DOMTimer is not a
ScriptWrappable and V8GCController does not count such an object). As a result,
a worker can be GC'ed even if there is still an active timer.
<After this CL>
After initial script evaluation or MessageEvent, the object proxy starts an
exponential backoff timer to periodically check an activity state and reports
to the messaging proxy when all activities are done.
The messaging proxy reports to the GC that the worker object is collectable
after all posted messages and pending activities are completed.
[*] https://html.spec.whatwg.org/multipage/workers.html#the-worker's-lifetime
BUG=572226, 584851
Committed: https://crrev.com/9af6c91f5bc79d12247a5780071c44025e4c026d
Cr-Commit-Position: refs/heads/master@{#413052}
==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/9af6c91f5bc79d12247a5780071c44025e4c026d Cr-Commit-Position: refs/heads/master@{#413052} |
