Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1117923004:
Run load state update timer only when needed (Closed)
DescriptionRun load state update timer only when needed
Run the load state update timer only when there is any tab that is loading.
Otherwise, the timer would run indefinitely on pages using a hanging GET
request.
BUG=483287
Committed: https://crrev.com/9d01994498c14fbb7a84ef7c423ea8be9501cd31
Cr-Commit-Position: refs/heads/master@{#331588}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add comments #
Total comments: 5
Patch Set 3 : Rebase off http://crrev.com/1130343006 #
Total comments: 12
Patch Set 4 : Fixes for mmenke #
Total comments: 2
Patch Set 5 : Fix for mmenke #Patch Set 6 : Rebase #
Messages
Total messages: 30 (6 generated)
andresantoso@chromium.org changed reviewers: + mmenke@chromium.org
mmenke, this is still rough and unfinished, but wanted to get your opinion first on whether this is the way to go or not. https://codereview.chromium.org/1117923004/diff/1/content/browser/loader/reso... File content/browser/loader/resource_scheduler.h (right): https://codereview.chromium.org/1117923004/diff/1/content/browser/loader/reso... content/browser/loader/resource_scheduler.h:153: bool HasLoadingClients() const; Is there a better way to find out if any tab is currently loading?
Unclear if WebContentsImpl::LoadStateChanged affects anything when not loading - we do still update load_state_, load_state_host_, and waiting_for_response_ even when not loading a page. They may well not be used for anything, though. We'll also need to update all relevant docs. We may want a scoped tracker to figure out if there's any significant change in how often the method is called before and after landing a CL to change this as well. I'm skeptical if we'll see a huge difference, but I could certainly be wrong, and it seems worth trying. https://codereview.chromium.org/1117923004/diff/1/content/browser/loader/reso... File content/browser/loader/resource_scheduler.h (right): https://codereview.chromium.org/1117923004/diff/1/content/browser/loader/reso... content/browser/loader/resource_scheduler.h:153: bool HasLoadingClients() const; On 2015/05/05 20:06:49, Andre wrote: > Is there a better way to find out if any tab is currently loading? Hrm...Don't see one, since we're on the IO thread.
On 2015/05/05 21:42:18, mmenke wrote: > Unclear if WebContentsImpl::LoadStateChanged affects anything when not loading - > we do still update load_state_, load_state_host_, and waiting_for_response_ even > when not loading a page. > > They may well not be used for anything, though. We'll also need to update all > relevant docs. > > We may want a scoped tracker to figure out if there's any significant change in > how often the method is called before and after landing a CL to change this as > well. I'm skeptical if we'll see a huge difference, but I could certainly be > wrong, and it seems worth trying. On second thought, may see more than I was thinking - for some reason, I was thinking most hanging gets would be POSTs, but that may well not be the case.
load_state_ and load_state_host_ seems to be used only for status text, while waiting_for_response_ is used to choose between the 2 different spinner types (waiting or loading). It seems safe to only keep them updated during loading. I'm not familiar with scoped trackers, do you want me to look into adding one? We should be able to see the results on the telemetry perf dashboards. I expect it should reduce idle_wakeups_browser significantly with the Docs page. https://chromeperf.appspot.com/report?masters=ChromiumPerf&bots=chromium-rel-... This will definitely help a lot in case of Google Docs, where the timer was running every 100ms indefinitely while idling on a document. I don't know what other sites use hanging gets. Should I continue and refine this CL?
Added comments in patchset 2. mmenke, PTAL.
On 2015/05/05 22:11:53, Andre wrote: > load_state_ and load_state_host_ seems to be used only for status text, while > waiting_for_response_ > is used to choose between the 2 different spinner types (waiting or loading). It > seems safe to only > keep them updated during loading. > > I'm not familiar with scoped trackers, do you want me to look into adding one? > We should be able to see the results on the telemetry perf dashboards. I expect > it should reduce > idle_wakeups_browser significantly with the Docs page. > https://chromeperf.appspot.com/report?masters=ChromiumPerf&bots=chromium-rel-... Scoped trackers are pretty trivial to use. They basically report the frequency a block of code is reached, and how long it takes in aggregate (As well as how often it blocks for a significant amount of time). They only log stuff for the first 30 seconds of startup, which misses out on a lot of what's interesting here, unfortunately, but since they're averaged over all users, they give us a baseline to compare against better than just getting call counts. So the way we'd gather stats is just add the tracker without anything else, get stats for a couple days or a week, and then land this CL and see how it changes. Nice that we have at least one relevant metrics, but docs isn't the world, and it would be great to know how much of a problem this timer is, to determine prioritization. > This will definitely help a lot in case of Google Docs, where the timer was > running every 100ms > indefinitely while idling on a document. I don't know what other sites use > hanging gets. > > Should I continue and refine this CL? I think so - this does seem worth pursuing, if you have the time to work on it. Branch point is next Friday, and I'm sufficiently paranoid about easy to miss but user-visible consequences to landing this change that I'd prefer to do it after branch point. Sound good to you?
Thanks! Waiting until after branch SGTM. Looks like there is already a ScopedTracker in ResourceDispatcherHostImpl::UpdateLoadInfoOnUIThread, is that what you are thinking? Is there anything else you want to see changed in this CL?
On 2015/05/06 19:51:31, Andre wrote: > Thanks! > Waiting until after branch SGTM. > Looks like there is already a ScopedTracker in > ResourceDispatcherHostImpl::UpdateLoadInfoOnUIThread, > is that what you are thinking? > Is there anything else you want to see changed in this CL? Ahh - you're right, there is. I was thinking of one in the method that calls UpdateLoadInfoOnUIThread (Which also used to have one, but I recently removed it because we have a Linux crasher there with a weird stack trace). Since we're really just concerned with how often it's called, rather than CPU use, the ScopedTracker in UpdateLoadInfoOnUIThread should be fine. I'll get back to you about anything to do this CL tomorrow or Friday. A lot of reviews this week, and since we've agreed to put this off until after branch, this has dropped to the bottom of my review queue (Not part of a nefarious plot, honest).
On 2015/05/06 19:55:38, mmenke wrote: > On 2015/05/06 19:51:31, Andre wrote: > > Thanks! > > Waiting until after branch SGTM. > > Looks like there is already a ScopedTracker in > > ResourceDispatcherHostImpl::UpdateLoadInfoOnUIThread, > > is that what you are thinking? > > Is there anything else you want to see changed in this CL? > > Ahh - you're right, there is. I was thinking of one in the method that calls > UpdateLoadInfoOnUIThread (Which also used to have one, but I recently removed it > because we have a Linux crasher there with a weird stack trace). Since we're > really just concerned with how often it's called, rather than CPU use, the > ScopedTracker in UpdateLoadInfoOnUIThread should be fine. > > I'll get back to you about anything to do this CL tomorrow or Friday. A lot of > reviews this week, and since we've agreed to put this off until after branch, > this has dropped to the bottom of my review queue (Not part of a nefarious plot, > honest). Another thing we can consider doing (Not in this CL): Make the ResourceLoader run its own timer to report upload status. It's kinda weird that there's only one timer for both. Why would having *two* timers running in some cases be better? Beyond separating out the ResourceLoader logic, it would let us experiment with a less frequent timer in RDH, and we can have the ResourceLoader stop its timer once the upload is complete (Though we'll have to restart it after redirects, if there's still an upload body). Unclear what that'll save us, though. I may make time to work on it myself, depending on how this works out and my review load. Can also take the opportunity to actually add tests. I think we should also have tests for this CL, if we can manage them. RDH is generally under tested, considering how much stuff it does. Would be great if we could test this runs only when it should.
I may need your help to figure out how to test this. I looked at ResourceDispatcherHostTest, but it's unclear to me how to deal with the timer and the runloop to test that UpdateLoadInfo is only called when it should. There is also a problem with the use of RenderViewHostImpl in UpdateLoadInfoOnUIThread, I think. We may have to remove that dependency and maybe create a ResourceDispatcherHostImpl::Client interface that RenderViewHostImpl implents? https://codereview.chromium.org/1117923004/diff/20001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1117923004/diff/20001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:117: const int kUpdateLoadStatesIntervalMsec = 100; Do you think this should be increased? The status test shouldn't need to update that fast right? We would want a smooth upload progress bar, but maybe 200ms is enough?
Happy to help with tests. I'll take a look tomorrow. I inherited basically all of this code, so will need to dig through things a bit to say anything useful on the topic. https://codereview.chromium.org/1117923004/diff/20001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1117923004/diff/20001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:117: const int kUpdateLoadStatesIntervalMsec = 100; On 2015/05/07 19:31:49, Andre wrote: > Do you think this should be increased? > The status test shouldn't need to update that fast right? > We would want a smooth upload progress bar, but maybe 200ms is enough? I think 200 ms would be fine, and we could experiment with higher (Particularly when there's no upload in progress)... But think we should stick with one thing at a time. I'd also like to have a better idea of what's being done with the upload progress we report to the renderer as well.
mmenke, M44 has branched. Time to try to land this?
On 2015/05/18 18:07:20, Andre wrote: > mmenke, M44 has branched. Time to try to land this? Indeed it is - I'll think about tests tomorrow, and should get you some concrete suggestions then.
On 2015/05/18 20:14:58, mmenke wrote: > On 2015/05/18 18:07:20, Andre wrote: > > mmenke, M44 has branched. Time to try to land this? > > Indeed it is - I'll think about tests tomorrow, and should get you some concrete > suggestions then. For an upload progress test, you can make a URLRequestJob suclass like URLRequestLoadInfoJob, that returns an upload progress value that increases each time it's called (Have to make the progress mutable), and always returns a load state of LOAD_STATE_SENDING_REQUEST. You can make MaybeCreateJobWithProtocolHandler create the new job class if some bool is set. Then make the test fixture's Send method automatically ACK the upload progress messages, spin the message loop until you've send a couple acks (3?), and then exit the message loop. Can then double check the list of IPCs in accum_, to make sure that the ACKs were actually received. Not the prettiest test, and it'll wait for the 100 ms timer a couple times, which isn't great, but I don't think it's too bad. Note that in the unit tests, there's no loading tab, so the scheduler check will always return false. Checking that the timer stops when the upload is canceled or complete is probably doable, but adds more ugliness. Checking that UpdateLoadInfo look more ugly to me, since only the RenderViewHostImpl, HostWebContentsDelegate/WebContentsImpl, and WebContentsDelegate get the notification. We could change the delegate for shell()'s WebContents, start a navigation with a hung request or two, and make sure we get notifications. And in another test, we could just do a normal page load, and make sure we get notifications, and then that they stop after commit - the "after they stop", we'd need to open a new window and wait for it to navigate or something...Which seems very weird. Another, much simpler solution, for both tests, is to add a method to RenderViewHostImpl to be called whenever UpdateLoadInfo is invoked - making sure it's not called when no longer needed, though, is still ugly. Not a huge fan of adding test code in production code, but this is ugly enough that it may be worth considering. On the down side, this doesn't test we pipe the messages all the way through to their final destination. I'm a bit concerned about the complexity of any of these approaches. While I spend the most test describing it, I think the upload test is simpler. I'm completely open to other ideas. We could just add a test for the upload case (Which is simpler than it sounds - I did less hand waving there), and leave checking other cases for another day, though the lack of tests in this area makes me nervous.
Uploaded patchset 3, now based off http://crrev.com/1130343006. https://codereview.chromium.org/1117923004/diff/20001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1117923004/diff/20001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:117: const int kUpdateLoadStatesIntervalMsec = 100; How about 250ms ;-D
Looks pretty good https://codereview.chromium.org/1117923004/diff/20001/content/browser/loader/... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/1117923004/diff/20001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:1009: for (auto& client : client_map_) { nit: const auto& client https://codereview.chromium.org/1117923004/diff/40001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1117923004/diff/40001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2162: load_info.second.upload_size); You should consider updating the GetLoadState() docs in content/public/browser/web_contents.h to mention this change. Admittedly, we weren't calling WebContentsDelegate::NavigationStateChanged when not loading, but now we're also not modifying the WebContent's publicly accessible load state. https://codereview.chromium.org/1117923004/diff/40001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2173: for (const auto& loader : pending_loaders_) { Should actually remove this call in the other CL. https://codereview.chromium.org/1117923004/diff/40001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2198: return; I think a repeating timer with an explicit Stop() here and a comment on it makes for clearer code. (And I think we're still fine with the one in RemovePendingLoader removed). So this would be: // Comment on why both of these are fine to stop on. if (info_map->empty() || !scheduler_->HasLoadingClients()) { // Stop(); return; } Note that we'd also not be calling UpdateLoadInfoOnUIThread when there are no loading clients, for potentially very modest savings. https://codereview.chromium.org/1117923004/diff/40001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2206: ScheduleUpdateLoadInfo(); Should use 2 space indent here, not 3 https://codereview.chromium.org/1117923004/diff/40001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2370: if (!update_load_states_timer_->IsRunning()) { Suggest early return instead - it's generally preferred. https://codereview.chromium.org/1117923004/diff/40001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/1117923004/diff/40001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.h:497: // get" request like when on a Google Docs document. nit: Don't use "we" in comments, due to ambiguity.
Patchset #4 (id:60001) has been deleted
PTAL https://codereview.chromium.org/1117923004/diff/20001/content/browser/loader/... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/1117923004/diff/20001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:1009: for (auto& client : client_map_) { On 2015/05/21 15:53:25, mmenke wrote: > nit: const auto& client Done. https://codereview.chromium.org/1117923004/diff/40001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1117923004/diff/40001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2162: load_info.second.upload_size); On 2015/05/21 15:53:25, mmenke wrote: > You should consider updating the GetLoadState() docs in > content/public/browser/web_contents.h to mention this change. > > Admittedly, we weren't calling WebContentsDelegate::NavigationStateChanged when > not loading, but now we're also not modifying the WebContent's publicly > accessible load state. Done. https://codereview.chromium.org/1117923004/diff/40001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2173: for (const auto& loader : pending_loaders_) { On 2015/05/21 15:53:25, mmenke wrote: > Should actually remove this call in the other CL. Done. https://codereview.chromium.org/1117923004/diff/40001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2198: return; You're right. Now that the upload progress code is gone, it's simpler to use a RepeatingTimer. Done. https://codereview.chromium.org/1117923004/diff/40001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2206: ScheduleUpdateLoadInfo(); On 2015/05/21 15:53:25, mmenke wrote: > Should use 2 space indent here, not 3 Done. https://codereview.chromium.org/1117923004/diff/40001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2370: if (!update_load_states_timer_->IsRunning()) { On 2015/05/21 15:53:25, mmenke wrote: > Suggest early return instead - it's generally preferred. Done. https://codereview.chromium.org/1117923004/diff/40001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/1117923004/diff/40001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.h:497: // get" request like when on a Google Docs document. On 2015/05/21 15:53:25, mmenke wrote: > nit: Don't use "we" in comments, due to ambiguity. Done.
LGTM. You'll need a content/public owner to sign off, too (And shouldn't CQ this on until the other one lands). https://codereview.chromium.org/1117923004/diff/80001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1117923004/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2210: // on a Google Docs document. nit: Shouldn't mention Google Docs, think it's fine just to end at request, or could say "like many chat widgits use."
https://codereview.chromium.org/1117923004/diff/80001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1117923004/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2210: // on a Google Docs document. On 2015/05/22 15:19:24, mmenke wrote: > nit: Shouldn't mention Google Docs, think it's fine just to end at request, or > could say "like many chat widgits use." Done.
andresantoso@chromium.org changed reviewers: + avi@chromium.org
+Avi for content/public.
content/public lgtm
Patchset #6 (id:120001) has been deleted
The CQ bit was checked by andresantoso@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org, avi@chromium.org Link to the patchset: https://codereview.chromium.org/1117923004/#ps140001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1117923004/140001
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/9d01994498c14fbb7a84ef7c423ea8be9501cd31 Cr-Commit-Position: refs/heads/master@{#331588} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
