|
|
Created:
6 years, 2 months ago by jochen (gone - plz use gerrit) Modified:
6 years, 2 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionRemove foreground tab idle handler
For foreground tabs the blink scheduler handles idle time.
BUG=404850, 397026
R=hpayer@chromium.org,sullivan@chromium.org,reveman@chromium.org,ulan@chromium.org,dcheng@chromium.org
Committed: https://crrev.com/5a32aaf0592a4109f6d13f39a8aeaa55243fdd26
Cr-Commit-Position: refs/heads/master@{#297025}
Patch Set 1 #
Total comments: 7
Patch Set 2 : updates #
Messages
Total messages: 23 (2 generated)
jochen@chromium.org changed reviewers: + dcheng@chromium.org
Daniel, please review the removal of the IPC msg Annie, please review everything David, Hannes, Ulan, fyi
content/common/view_messages.h changes and IPC message handler deletions LGTM
+ross fyi
https://codereview.chromium.org/603083002/diff/1/content/renderer/render_thre... File content/renderer/render_thread_impl.cc (left): https://codereview.chromium.org/603083002/diff/1/content/renderer/render_thre... content/renderer/render_thread_impl.cc:1077: if (!base::DiscardableMemory::ReduceMemoryUsage()) where is this moved?
https://codereview.chromium.org/603083002/diff/1/content/renderer/render_thre... File content/renderer/render_thread_impl.cc (left): https://codereview.chromium.org/603083002/diff/1/content/renderer/render_thre... content/renderer/render_thread_impl.cc:1077: if (!base::DiscardableMemory::ReduceMemoryUsage()) On 2014/09/25 at 13:34:17, reveman wrote: > where is this moved? nowhere. In our experiments, the idle handler is almost never invoked, because the system's load is typically higher than 3% also, it's not really an idle handler, we just keep posting a delayed task, so we might end up discarding memory in the middle of an animation.
On 2014/09/25 at 14:18:29, jochen (slow for reviews) wrote: > https://codereview.chromium.org/603083002/diff/1/content/renderer/render_thre... > File content/renderer/render_thread_impl.cc (left): > > https://codereview.chromium.org/603083002/diff/1/content/renderer/render_thre... > content/renderer/render_thread_impl.cc:1077: if (!base::DiscardableMemory::ReduceMemoryUsage()) > On 2014/09/25 at 13:34:17, reveman wrote: > > where is this moved? > > nowhere. In our experiments, the idle handler is almost never invoked, because the system's load is typically higher than 3% > > also, it's not really an idle handler, we just keep posting a delayed task, so we might end up discarding memory in the middle of an animation. however, if you can specify what this call is supposed to do, I can try to find a spot where it achieves that
https://codereview.chromium.org/603083002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/render_message_filter.h (left): https://codereview.chromium.org/603083002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_message_filter.h:342: scoped_ptr<base::ProcessMetrics> process_metrics_; Does this affect the PerformanceMonitor.AverageCPU.RendererProcess UMA metric? We do use that to monitor CPU usage: https://uma.googleplex.com/timeline#metric=PerformanceMonitor.AverageCPU.Rend...
https://codereview.chromium.org/603083002/diff/1/content/renderer/render_thre... File content/renderer/render_thread_impl.cc (left): https://codereview.chromium.org/603083002/diff/1/content/renderer/render_thre... content/renderer/render_thread_impl.cc:1077: if (!base::DiscardableMemory::ReduceMemoryUsage()) On 2014/09/25 14:18:28, jochen (slow for reviews) wrote: > On 2014/09/25 at 13:34:17, reveman wrote: > > where is this moved? > > nowhere. In our experiments, the idle handler is almost never invoked, because > the system's load is typically higher than 3% > > also, it's not really an idle handler, we just keep posting a delayed task, so > we might end up discarding memory in the middle of an animation. This call will discard memory that has not been used for some specific amount of time. Calling this in the middle of an animation is not a problem as it won't discard memory that is still frequently used. Calling this when actually idle would be nice but just posting a delayed task is fine too in this case. However, we can't just remove it as that will affect the memory usage of significantly.
https://codereview.chromium.org/603083002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/render_message_filter.h (left): https://codereview.chromium.org/603083002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_message_filter.h:342: scoped_ptr<base::ProcessMetrics> process_metrics_; On 2014/09/25 at 14:34:47, sullivan wrote: > Does this affect the PerformanceMonitor.AverageCPU.RendererProcess UMA metric? We do use that to monitor CPU usage: > > https://uma.googleplex.com/timeline#metric=PerformanceMonitor.AverageCPU.Rend... that's collected here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/per... https://codereview.chromium.org/603083002/diff/1/content/renderer/render_thre... File content/renderer/render_thread_impl.cc (left): https://codereview.chromium.org/603083002/diff/1/content/renderer/render_thre... content/renderer/render_thread_impl.cc:1077: if (!base::DiscardableMemory::ReduceMemoryUsage()) On 2014/09/25 at 15:03:41, reveman wrote: > On 2014/09/25 14:18:28, jochen (slow for reviews) wrote: > > On 2014/09/25 at 13:34:17, reveman wrote: > > > where is this moved? > > > > nowhere. In our experiments, the idle handler is almost never invoked, because > > the system's load is typically higher than 3% > > > > also, it's not really an idle handler, we just keep posting a delayed task, so > > we might end up discarding memory in the middle of an animation. > > This call will discard memory that has not been used for some specific amount of time. Calling this in the middle of an animation is not a problem as it won't discard memory that is still frequently used. Calling this when actually idle would be nice but just posting a delayed task is fine too in this case. However, we can't just remove it as that will affect the memory usage of significantly. so just postTask this call every 30s should be fine (which is more or less what the handler did)? Is there a test that I can use to verify that nothing regresses?
On 2014/09/25 15:08:36, jochen (slow for reviews) wrote: > https://codereview.chromium.org/603083002/diff/1/content/browser/renderer_hos... > File content/browser/renderer_host/render_message_filter.h (left): > > https://codereview.chromium.org/603083002/diff/1/content/browser/renderer_hos... > content/browser/renderer_host/render_message_filter.h:342: > scoped_ptr<base::ProcessMetrics> process_metrics_; > On 2014/09/25 at 14:34:47, sullivan wrote: > > Does this affect the PerformanceMonitor.AverageCPU.RendererProcess UMA metric? > We do use that to monitor CPU usage: > > > > > https://uma.googleplex.com/timeline#metric=PerformanceMonitor.AverageCPU.Rend... > > that's collected here: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/per... > > https://codereview.chromium.org/603083002/diff/1/content/renderer/render_thre... > File content/renderer/render_thread_impl.cc (left): > > https://codereview.chromium.org/603083002/diff/1/content/renderer/render_thre... > content/renderer/render_thread_impl.cc:1077: if > (!base::DiscardableMemory::ReduceMemoryUsage()) > On 2014/09/25 at 15:03:41, reveman wrote: > > On 2014/09/25 14:18:28, jochen (slow for reviews) wrote: > > > On 2014/09/25 at 13:34:17, reveman wrote: > > > > where is this moved? > > > > > > nowhere. In our experiments, the idle handler is almost never invoked, > because > > > the system's load is typically higher than 3% > > > > > > also, it's not really an idle handler, we just keep posting a delayed task, > so > > > we might end up discarding memory in the middle of an animation. > > > > This call will discard memory that has not been used for some specific amount > of time. Calling this in the middle of an animation is not a problem as it won't > discard memory that is still frequently used. Calling this when actually idle > would be nice but just posting a delayed task is fine too in this case. > However, we can't just remove it as that will affect the memory usage of > significantly. > > so just postTask this call every 30s should be fine (which is more or less what > the handler did)? > > Is there a test that I can use to verify that nothing regresses? Any of the smoothness_benchmark tests should be good to check that framerate is unaffected.
lgtm
On 2014/09/25 at 15:20:56, sullivan wrote: > > > > Is there a test that I can use to verify that nothing regresses? > > Any of the smoothness_benchmark tests should be good to check that framerate is unaffected. that question was for David. I ran smoothness.top_25 locally and didn't see any regressions.
https://codereview.chromium.org/603083002/diff/1/content/renderer/render_thre... File content/renderer/render_thread_impl.cc (left): https://codereview.chromium.org/603083002/diff/1/content/renderer/render_thre... content/renderer/render_thread_impl.cc:1077: if (!base::DiscardableMemory::ReduceMemoryUsage()) On 2014/09/25 15:08:36, jochen (slow for reviews) wrote: > On 2014/09/25 at 15:03:41, reveman wrote: > > On 2014/09/25 14:18:28, jochen (slow for reviews) wrote: > > > On 2014/09/25 at 13:34:17, reveman wrote: > > > > where is this moved? > > > > > > nowhere. In our experiments, the idle handler is almost never invoked, > because > > > the system's load is typically higher than 3% > > > > > > also, it's not really an idle handler, we just keep posting a delayed task, > so > > > we might end up discarding memory in the middle of an animation. > > > > This call will discard memory that has not been used for some specific amount > of time. Calling this in the middle of an animation is not a problem as it won't > discard memory that is still frequently used. Calling this when actually idle > would be nice but just posting a delayed task is fine too in this case. > However, we can't just remove it as that will affect the memory usage of > significantly. > > so just postTask this call every 30s should be fine (which is more or less what > the handler did)? Maybe. Is there not a better idle signal we can hook this up to instead? What about the blink scheduler which is mentioned in the description of this cl? > > Is there a test that I can use to verify that nothing regresses? Sorry, nothing good at the moment. Feel free to add some implementation specific test to RenderThreadImplBrowserTest similar to EmulatedDiscardableMemoryDiscardedWhenWidgetsHidden if you like. You probably have to add some hooks to the discardable memory interface to avoid having the test take seconds to run though.
On 2014/09/25 16:44:49, reveman wrote: > https://codereview.chromium.org/603083002/diff/1/content/renderer/render_thre... > File content/renderer/render_thread_impl.cc (left): > > https://codereview.chromium.org/603083002/diff/1/content/renderer/render_thre... > content/renderer/render_thread_impl.cc:1077: if > (!base::DiscardableMemory::ReduceMemoryUsage()) > On 2014/09/25 15:08:36, jochen (slow for reviews) wrote: > > On 2014/09/25 at 15:03:41, reveman wrote: > > > On 2014/09/25 14:18:28, jochen (slow for reviews) wrote: > > > > On 2014/09/25 at 13:34:17, reveman wrote: > > > > > where is this moved? > > > > > > > > nowhere. In our experiments, the idle handler is almost never invoked, > > because > > > > the system's load is typically higher than 3% > > > > > > > > also, it's not really an idle handler, we just keep posting a delayed > task, > > so > > > > we might end up discarding memory in the middle of an animation. > > > > > > This call will discard memory that has not been used for some specific > amount > > of time. Calling this in the middle of an animation is not a problem as it > won't > > discard memory that is still frequently used. Calling this when actually idle > > would be nice but just posting a delayed task is fine too in this case. > > However, we can't just remove it as that will affect the memory usage of > > significantly. > > > > so just postTask this call every 30s should be fine (which is more or less > what > > the handler did)? > > Maybe. Is there not a better idle signal we can hook this up to instead? What > about the blink scheduler which is mentioned in the description of this cl? The idle task queue in the Blink scheduler isn't quite ready yet (I have an in-progress CL here - https://codereview.chromium.org/595023002/). The Blink scheduler attempts to run idle task in the the time available between frames committed by the compositor - i.e., fine granularity very frequently (potentially every frame). Therefore it's not quite the same concept as the IdleHandler here. You could probably emulate something similar by posting a delayed task every 30s, but in that task doing nothing other than posting an idle task to the Blink Scheduler to ensure that the main source of work happens when the compositor is idle. We might also consider adding additional policies to the blink scheduler to run certain tasks when the Tab is idle from a user-point of view (e.g., no user input for 30s) - would this be useful here? > > Is there a test that I can use to verify that nothing regresses? > > Sorry, nothing good at the moment. Feel free to add some implementation specific > test to RenderThreadImplBrowserTest similar to > EmulatedDiscardableMemoryDiscardedWhenWidgetsHidden if you like. You probably > have to add some hooks to the discardable memory interface to avoid having the > test take seconds to run though.
On 2014/09/25 17:45:47, rmcilroy wrote: > On 2014/09/25 16:44:49, reveman wrote: > > > https://codereview.chromium.org/603083002/diff/1/content/renderer/render_thre... > > File content/renderer/render_thread_impl.cc (left): > > > > > https://codereview.chromium.org/603083002/diff/1/content/renderer/render_thre... > > content/renderer/render_thread_impl.cc:1077: if > > (!base::DiscardableMemory::ReduceMemoryUsage()) > > On 2014/09/25 15:08:36, jochen (slow for reviews) wrote: > > > On 2014/09/25 at 15:03:41, reveman wrote: > > > > On 2014/09/25 14:18:28, jochen (slow for reviews) wrote: > > > > > On 2014/09/25 at 13:34:17, reveman wrote: > > > > > > where is this moved? > > > > > > > > > > nowhere. In our experiments, the idle handler is almost never invoked, > > > because > > > > > the system's load is typically higher than 3% > > > > > > > > > > also, it's not really an idle handler, we just keep posting a delayed > > task, > > > so > > > > > we might end up discarding memory in the middle of an animation. > > > > > > > > This call will discard memory that has not been used for some specific > > amount > > > of time. Calling this in the middle of an animation is not a problem as it > > won't > > > discard memory that is still frequently used. Calling this when actually > idle > > > would be nice but just posting a delayed task is fine too in this case. > > > However, we can't just remove it as that will affect the memory usage of > > > significantly. > > > > > > so just postTask this call every 30s should be fine (which is more or less > > what > > > the handler did)? > > > > Maybe. Is there not a better idle signal we can hook this up to instead? What > > about the blink scheduler which is mentioned in the description of this cl? > > The idle task queue in the Blink scheduler isn't quite ready yet (I have an > in-progress CL here - https://codereview.chromium.org/595023002/). The Blink > scheduler attempts to run idle task in the the time available between frames > committed by the compositor - i.e., fine granularity very frequently > (potentially every frame). Therefore it's not quite the same concept as the > IdleHandler here. That might be fine. We should be able to call this as frequently as we like. The only bad thing that will happen is that you waste CPU cycles and power. We don't want that of course but there are no other bad side-effects from calling it frequently. > You could probably emulate something similar by posting a > delayed task every 30s, but in that task doing nothing other than posting an > idle task to the Blink Scheduler to ensure that the main source of work happens > when the compositor is idle. Sgtm. > We might also consider adding additional policies > to the blink scheduler to run certain tasks when the Tab is idle from a > user-point of view (e.g., no user input for 30s) - would this be useful here? I don't think user input is relevant in this specific case. > > > > Is there a test that I can use to verify that nothing regresses? > > > > Sorry, nothing good at the moment. Feel free to add some implementation > specific > > test to RenderThreadImplBrowserTest similar to > > EmulatedDiscardableMemoryDiscardedWhenWidgetsHidden if you like. You probably > > have to add some hooks to the discardable memory interface to avoid having the > > test take seconds to run though.
here's a version that still calls the reduce memory calls ptal
lgtm
The CQ bit was checked by jochen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/603083002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as a531c805a32ca9ef1cc9679013e7df6e1e6dd90a
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/5a32aaf0592a4109f6d13f39a8aeaa55243fdd26 Cr-Commit-Position: refs/heads/master@{#297025} |