New issue
Advanced search Search tips

Issue 616721 link

Starred by 1 user

Issue metadata

Status: Archived
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

Adjust delay for forcible worker thread shutdown tasks

Project Member Reported by nhiroki@chromium.org, Jun 2 2016

Issue description

This is a follow-up issue for  issue 487050 .

To determine appropriate delay for forcible worker thread shutdown tasks, we should add UMA to record how WorkerThread is terminated (i.e. WorkerThread::getExitCode()). If the number of |AsyncForciblyTerminated| is high, the delay may be too short. Also, UMA to record duration of worker script execution would be helpful to determine it.
 
Labels: M-53 OS-All
Status: Started (was: Assigned)
[Status Update]

According to the UMA, 99.7% of WorkerThread shutdown is done by graceful termination and 0.3% is done by asynchronous forcible termination.

I noticed this UMA is not sufficient for confirming effect of graceful shutdown because this is recorded in the dtor of WorkerThread. Renderer crashes by forcible termination may happen before the dtor. Maybe we should have another UMA like WorkerThread.TerminationAttempts. If there is a difference between the the number of WorkerThread.ExitCode and the number of WorkerThread.TerminationAttempts, probably some termination attempts crashed.

> Also, UMA to record duration of worker script execution would be helpful to determine it.

I have a WIP patch for this:
https://codereview.chromium.org/2077153002/

Comment 5 by kinuko@chromium.org, Jun 23 2016

Reg: the shutdown UMA-- for forcible termination case during process shutdown I started to worry that we might not be able to really send the UMA info to the browser process, which might skew the histogram too...?  (At least that used to be a problem when collecting less noisy UMA for loading. I vaguely remember we started to send UMA data more reliably than before but I'm not too sure about that esp. things that are collected during process shutdown)
Project Member

Comment 6 by bugdroid1@chromium.org, Jun 24 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cc65b94db2ebe5b99396f1cabb520f4dc1d8c259

commit cc65b94db2ebe5b99396f1cabb520f4dc1d8c259
Author: nhiroki <nhiroki@chromium.org>
Date: Fri Jun 24 00:45:42 2016

Worker: Record time taken for running a debugger/worker task on WorkerThread

UMA to record duration of a debugger/worker task would be helpful to determine
a delay for a task of forcible worker thread shutdown.

BUG= 616721 

Review-Url: https://codereview.chromium.org/2077153002
Cr-Commit-Position: refs/heads/master@{#401759}

[modify] https://crrev.com/cc65b94db2ebe5b99396f1cabb520f4dc1d8c259/third_party/WebKit/Source/core/workers/WorkerThread.cpp
[modify] https://crrev.com/cc65b94db2ebe5b99396f1cabb520f4dc1d8c259/tools/metrics/histograms/histograms.xml

Reg: recording ExitCode UMA on shutdown, separating the UMA into ExitCode_OnShutdown and ExitCode_OnRunning could be helpful to reduce a noise?
[Status Update]

According to WorkerThread.Task.Time UMA on Win Canary, 96% of regular tasks are complete in 2 sec, 98% in 5 sec and 99% in 14 sec. The current delay (2 sec) would be quite a reasonable value on desktop. On the other hand, I guess running a task on mobile may take more time. We might want to slightly increase the delay for mobile. I'm going to wait until the UMA gets in Dev etc on Android.
Not directly related, but do you know what percentage of the renderers is going through the shutdown sequence? I think that some (most?) renderers are forcibly killed by the browser process without being asked to shut down.


Reg: c#9-19, does "Fast Shutdown" mean forcible process termination? I don't know the percentage of renderers to be gracefully terminated. AFAICS, if a renderer process has SharedWorker or ServiceWorker, fast shutdown does not run:
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_process_host_impl.cc?sq=package:chromium&type=cs&rcl=1458862641&l=1670 (see |worker_ref_count_| handling)
[Status Update]

I checked UMAs on Android as of Jul 02 2016 (7 day aggregation):

> WorkerThread.ExitCode
GracefullyTerminated    99.86%
AsyncForciblyTerminated 00.14%
SyncForciblyTerminated  00.00%

> WorkerThread.Task.Time
79.9%tile    2 secs
82.9%tile  2.8 secs
85.6%tile  3.9 secs
87.3%tile    5 secs
89.3%tile   10 secs
91.2%tile 19.9 secs

The current delay (2 secs) seems to be short for Android, but 10 secs delay for 90%tile would be too long... 4 or 5 secs could be a point of compromise??

Note: Tweaking the delay does not affect synchronous forcible termination (i.e. terminateAndWait(), terminateAndWaitForAllWorkers()) requested by renderer shutdown etc. It affects only regular termination to schedule a delayed task.
Project Member

Comment 13 by sheriffbot@chromium.org, Jul 7 2016

Labels: -M-53 M-54 MovedFrom-53
Moving this nonessential bug to the next milestone.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Status: Archived (was: Started)
I think we can archive this, it's been 1+ years. I don't think there's plans to adjust the delay unless we get evidence that it's problematic. And the histogram for task times is getting overflowed because of too many samples.
Memo of the UMA before deletion (Feb 11 2018 - 7DA). The UMA of WorkerThread.Task.Time might be suspect because of  issue 809672 , also we've had bugs where the worker thread is in a tight loop of very small tasks due to misuse of the Mojo handle watcher API.

WorkerThread.Task.Time on Android Stable:
50.00%	52.78
75.00%	297.9
95.00%	6950
99.00%	399632
99.50%	1045913

WorkerThread.Task.Time on Win/Mac/Linux/CrOS Stable:
50.00%	3.021
75.00%	8.246
95.00%	135.0
99.00%	84247
99.50%	180172

WorkerThread.DebuggerTask.Time on Android Stable
50.00%	277.2
75.00%	624.5
95.00%	46261
99.00%	105016
99.50%	137481

WorkerThread.DebuggerTask.Time on Win/Mac/Linux/CrOS Stable
50.00%	67.53
75.00%	146.0
95.00%	17603
99.00%	56126
99.50%	80864
Project Member

Comment 16 by bugdroid1@chromium.org, Feb 13 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0335b2141d9938dfb8417f600165fcc9b2addf17

commit 0335b2141d9938dfb8417f600165fcc9b2addf17
Author: Matt Falkenhagen <falken@chromium.org>
Date: Tue Feb 13 06:09:00 2018

Remove WorkerThread.Task.Time histogram.

This is overflowing causing confusion. Since we don't use it we can
delete it. Also delete the related histogram
WorkerThread.DebuggerTask.Time.

Bug:  616721 , 809672 
Change-Id: I33895fdf033ab2c07ccecfb05c75085a45569011
Reviewed-on: https://chromium-review.googlesource.com/911229
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536264}
[modify] https://crrev.com/0335b2141d9938dfb8417f600165fcc9b2addf17/third_party/WebKit/Source/core/workers/WorkerThread.cpp
[modify] https://crrev.com/0335b2141d9938dfb8417f600165fcc9b2addf17/third_party/WebKit/Source/platform/scheduler/child/worker_scheduler_impl.cc
[modify] https://crrev.com/0335b2141d9938dfb8417f600165fcc9b2addf17/tools/metrics/histograms/histograms.xml

Sign in to add a comment