Premature killing of util process for data decoder service |
||
Issue descriptionIn https://codereview.chromium.org/2538833004, a 5-second delay was added to the tearing down process, to better amortize the cost of launching a sandboxed utility process. However, it only covers 5 more seconds of request, but not extending it further even if there are requests coming in. For example, at t = 0.5, the util process became idle, and tearing down was scheduled on t = 5.5. Assume the requests keep coming every second, so at t = 1, 2, 3, 4, 5, the requests can reuse the same process. However, at t = 6, that process has been killed, and we need to launch a new sandboxed process for this request. In the steady state, we kill and re-spawn it around every 5 seconds. This issue was discovered by analyzing the image decoding in NTP feeds, where a suatained period of time of spotty image decoding happens. To better debounce process launching and killing. It might make more sense to keep it alive as long as there haven't been a 5-second period of idling. My main concern is that any possible memory leaks within data decoder would not be automatically fixed by periodical killing, but I guess over-killing might also hinder the opportunity to discover such memory leaks. Thoughts?
,
May 21 2018
More context: given the small image sizes on NTP feeds, the decoding latency is usually <10ms. Respawning the util process would add ~300ms to it. This is measured on Pixel 2 XL release build.
,
Jun 1 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/673672f2638bdfcfe7f811b1b679f15429be1441 commit 673672f2638bdfcfe7f811b1b679f15429be1441 Author: Ken Rockot <rockot@chromium.org> Date: Fri Jun 01 06:06:40 2018 Add ServiceKeepalive helper for idle service shutdown Adds ServiceKeepalive to the service manager client library. This can be constructed with a fixed time delay and used by services to automatically shut themselves down after some idle period. "Idle" state is equivalent to the number of living ServiceContextRefs being zero. This is hooked up in the data_decoder service to correct a bug in its current implementation of time-delayed self-termination. Bug: 845247 Change-Id: I7431658f1c1f407e55384abd2a48e77a5b324062 Reviewed-on: https://chromium-review.googlesource.com/1068078 Commit-Queue: Ken Rockot <rockot@chromium.org> Reviewed-by: Wei-Yin Chen (陳威尹) <wychen@chromium.org> Reviewed-by: Jay Civelli <jcivelli@chromium.org> Cr-Commit-Position: refs/heads/master@{#563547} [modify] https://crrev.com/673672f2638bdfcfe7f811b1b679f15429be1441/services/data_decoder/data_decoder_service.cc [modify] https://crrev.com/673672f2638bdfcfe7f811b1b679f15429be1441/services/data_decoder/data_decoder_service.h [modify] https://crrev.com/673672f2638bdfcfe7f811b1b679f15429be1441/services/data_decoder/public/cpp/test_data_decoder_service.cc [modify] https://crrev.com/673672f2638bdfcfe7f811b1b679f15429be1441/services/data_decoder/public/cpp/test_data_decoder_service.h [modify] https://crrev.com/673672f2638bdfcfe7f811b1b679f15429be1441/services/service_manager/public/cpp/BUILD.gn [modify] https://crrev.com/673672f2638bdfcfe7f811b1b679f15429be1441/services/service_manager/public/cpp/service_context_ref.cc [modify] https://crrev.com/673672f2638bdfcfe7f811b1b679f15429be1441/services/service_manager/public/cpp/service_context_ref.h [add] https://crrev.com/673672f2638bdfcfe7f811b1b679f15429be1441/services/service_manager/public/cpp/service_keepalive.cc [add] https://crrev.com/673672f2638bdfcfe7f811b1b679f15429be1441/services/service_manager/public/cpp/service_keepalive.h
,
Jun 28 2018
|
||
►
Sign in to add a comment |
||
Comment 1 by wychen@chromium.org
, May 21 2018