Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1372263002:
Add a field trial to delay all resource requests by thread hops. (Closed)
DescriptionAdd a field trial to delay all resource requests by thread hops.
This is an experiment to determine if we can afford making some resource
request decisions on the UI thread.
BUG=524228
Committed: https://crrev.com/3cc6d6a77c53a12e37d82128620c2b5609e8f247
Cr-Commit-Position: refs/heads/master@{#356142}
Patch Set 1 #Patch Set 2 : flag #Patch Set 3 : put in content for ContentShell.apk #Patch Set 4 : #
Total comments: 14
Patch Set 5 : mmenke comments #
Total comments: 6
Patch Set 6 : asvitkine #
Total comments: 2
Messages
Total messages: 37 (6 generated)
davidben@chromium.org changed reviewers: + mmenke@chromium.org
Think this looks reasonable? I'm thinking I'll just look at the thread hop time and compare it to Net.RequestTime2.Success (which isn't quiiiite the right denominator; could add another one of those perhaps?) to see roughly what the cost would be.
This LGTM https://codereview.chromium.org/1372263002/diff/60001/chrome/browser/renderer... File chrome/browser/renderer_host/thread_hop_resource_throttle.cc (right): https://codereview.chromium.org/1372263002/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/thread_hop_resource_throttle.cc:21: // static nit: New style guide doesn't require comments like this. https://codereview.chromium.org/1372263002/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/thread_hop_resource_throttle.cc:59: base::TimeDelta::FromSeconds(0), base::TimeDelta()? Or base::TimeDelta::FromMilliseconds(0) - seems weird to mix seconds and milliseconds. https://codereview.chromium.org/1372263002/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/thread_hop_resource_throttle.cc:60: base::TimeDelta::FromMilliseconds(100), 100); I don't think we need 100 buckets? I think even 20 would be more than enough.
https://codereview.chromium.org/1372263002/diff/60001/chrome/browser/renderer... File chrome/browser/renderer_host/thread_hop_resource_throttle.cc (right): https://codereview.chromium.org/1372263002/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/thread_hop_resource_throttle.cc:15: #include "content/public/browser/resource_controller.h" include header for RedirectInfo? https://codereview.chromium.org/1372263002/diff/60001/chrome/browser/renderer... File chrome/browser/renderer_host/thread_hop_resource_throttle.h (right): https://codereview.chromium.org/1372263002/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/thread_hop_resource_throttle.h:9: nit: Remove blank line. https://codereview.chromium.org/1372263002/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/thread_hop_resource_throttle.h:40: DISALLOW_COPY_AND_ASSIGN(ThreadHopResourceThrottle); include macros.h?
davidben@chromium.org changed reviewers: + asvitkine@chromium.org
+asvitkine for histograms review and also field trial advice. So, this is a very uninteresting perf-only field trial. I also was dumb and procrastinated way too much in getting to this w.r.t. branch point timing. I'm not quite sure what order everything needs to land in and what process things need to be mucked with how. Is it feasible to land this stuff now and do the rest of the process when turning the experiment on? I gather that if it's just Dev and such the process is lighter weight. Since this is just a tiny experiment to advise how to refactor the ResourceLoader logic, I'd like to get enough data to make a decision earlier if possible, but "no, davidben, you should have done this earlier, wait for M47" is also a perfectly reasonable answer. :-) https://codereview.chromium.org/1372263002/diff/60001/chrome/browser/renderer... File chrome/browser/renderer_host/thread_hop_resource_throttle.cc (right): https://codereview.chromium.org/1372263002/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/thread_hop_resource_throttle.cc:15: #include "content/public/browser/resource_controller.h" On 2015/10/01 21:37:33, mmenke wrote: > include header for RedirectInfo? The forward-decl from resource_throttle.h should be sufficient I think since we're not actually doing anything with that type. Or do we want to include it there anyway? https://codereview.chromium.org/1372263002/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/thread_hop_resource_throttle.cc:21: // static On 2015/10/01 21:36:01, mmenke wrote: > nit: New style guide doesn't require comments like this. Really? I rather liked those. :-( Removed. https://codereview.chromium.org/1372263002/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/thread_hop_resource_throttle.cc:59: base::TimeDelta::FromSeconds(0), On 2015/10/01 21:36:01, mmenke wrote: > base::TimeDelta()? Or base::TimeDelta::FromMilliseconds(0) - seems weird to mix > seconds and milliseconds. Done. https://codereview.chromium.org/1372263002/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/thread_hop_resource_throttle.cc:60: base::TimeDelta::FromMilliseconds(100), 100); On 2015/10/01 21:36:01, mmenke wrote: > I don't think we need 100 buckets? I think even 20 would be more than enough. I mostly copied from elsewhere. You sure 20 is enough though? So empirically the numbers seem to all be smushed around 0-16ms on my machine. https://codereview.chromium.org/1372263002/diff/60001/chrome/browser/renderer... File chrome/browser/renderer_host/thread_hop_resource_throttle.h (right): https://codereview.chromium.org/1372263002/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/thread_hop_resource_throttle.h:9: On 2015/10/01 21:37:33, mmenke wrote: > nit: Remove blank line. Derp. Done. https://codereview.chromium.org/1372263002/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/thread_hop_resource_throttle.h:40: DISALLOW_COPY_AND_ASSIGN(ThreadHopResourceThrottle); On 2015/10/01 21:37:33, mmenke wrote: > include macros.h? Done.
https://codereview.chromium.org/1372263002/diff/60001/chrome/browser/renderer... File chrome/browser/renderer_host/thread_hop_resource_throttle.cc (right): https://codereview.chromium.org/1372263002/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/thread_hop_resource_throttle.cc:15: #include "content/public/browser/resource_controller.h" On 2015/10/01 22:06:25, David Benjamin wrote: > On 2015/10/01 21:37:33, mmenke wrote: > > include header for RedirectInfo? > > The forward-decl from resource_throttle.h should be sufficient I think since > we're not actually doing anything with that type. Or do we want to include it > there anyway? Ah, right. https://codereview.chromium.org/1372263002/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/thread_hop_resource_throttle.cc:60: base::TimeDelta::FromMilliseconds(100), 100); On 2015/10/01 22:06:25, David Benjamin wrote: > On 2015/10/01 21:36:01, mmenke wrote: > > I don't think we need 100 buckets? I think even 20 would be more than enough. > > I mostly copied from elsewhere. You sure 20 is enough though? So empirically the > numbers seem to all be smushed around 0-16ms on my machine. I'm honestly not sure...I think 100 is way more than we need. Think you may want to think about what we care about. If they're all 0-4, do we care if they're all 0 vs 4? Do we care about 50 vs 100 ms?
https://codereview.chromium.org/1372263002/diff/80001/chrome/browser/renderer... File chrome/browser/renderer_host/thread_hop_resource_throttle.cc (right): https://codereview.chromium.org/1372263002/diff/80001/chrome/browser/renderer... chrome/browser/renderer_host/thread_hop_resource_throttle.cc:58: base::TimeDelta::FromMilliseconds(100), 100); Is there a reason you picked these parameters? Having 100ms as the max for a timing macro seems like it would result a lot of samples in the overflow bucket. Also, why 100 buckets and not e.g. 50 that is more typical? For example, why not just use UMA_HISTOGRAM_TIMES()? https://codereview.chromium.org/1372263002/diff/80001/chrome/browser/renderer... File chrome/browser/renderer_host/thread_hop_resource_throttle.h (right): https://codereview.chromium.org/1372263002/diff/80001/chrome/browser/renderer... chrome/browser/renderer_host/thread_hop_resource_throttle.h:27: // content::ResourceThrottle implementation: Nit: I think the new suggested content for this type of comment is: // content::ResourceThrottle:
https://codereview.chromium.org/1372263002/diff/80001/chrome/browser/renderer... File chrome/browser/renderer_host/thread_hop_resource_throttle.cc (right): https://codereview.chromium.org/1372263002/diff/80001/chrome/browser/renderer... chrome/browser/renderer_host/thread_hop_resource_throttle.cc:58: base::TimeDelta::FromMilliseconds(100), 100); On 2015/10/02 16:42:54, Alexei Svitkine wrote: > Is there a reason you picked these parameters? > > Having 100ms as the max for a timing macro seems like it would result a lot of > samples in the overflow bucket. Also, why 100 buckets and not e.g. 50 that is > more typical? > > For example, why not just use UMA_HISTOGRAM_TIMES()? I picked 100 because it looked like others did this too, though it seems I looked at exactly the wrong ones and 50's more common. :-) I'll switch to that. UMA_HISTOGRAM_TIMES goes up to 10 seconds and I would be surprised if it goes up anywhere near that high. It's just measuring a pair of thread hops. 10 / 50 is 0.2 seconds which is probably already high enough that I'd want to avoid the thread hop. Or am I completely misunderstanding what the number of buckets means and their distribution? Is UMA_HISTOGRAMS_TIMES still what I want for this kind of metric?
https://codereview.chromium.org/1372263002/diff/80001/chrome/browser/renderer... File chrome/browser/renderer_host/thread_hop_resource_throttle.cc (right): https://codereview.chromium.org/1372263002/diff/80001/chrome/browser/renderer... chrome/browser/renderer_host/thread_hop_resource_throttle.cc:58: base::TimeDelta::FromMilliseconds(100), 100); On 2015/10/02 17:19:42, David Benjamin wrote: > On 2015/10/02 16:42:54, Alexei Svitkine wrote: > > Is there a reason you picked these parameters? > > > > Having 100ms as the max for a timing macro seems like it would result a lot of > > samples in the overflow bucket. Also, why 100 buckets and not e.g. 50 that is > > more typical? > > > > For example, why not just use UMA_HISTOGRAM_TIMES()? > > I picked 100 because it looked like others did this too, though it seems I > looked at exactly the wrong ones and 50's more common. :-) I'll switch to that. > > UMA_HISTOGRAM_TIMES goes up to 10 seconds and I would be surprised if it goes up > anywhere near that high. It's just measuring a pair of thread hops. 10 / 50 is > 0.2 seconds which is probably already high enough that I'd want to avoid the > thread hop. Or am I completely misunderstanding what the number of buckets means > and their distribution? > > Is UMA_HISTOGRAMS_TIMES still what I want for this kind of metric? These macros provide a exponential distribution. So while you do have a max of 10s, the bucket sizes near there will be much higher and you'll get more granularity at the lower values. For example, here's the distribution from that macro for another histogram: https://uma.googleplex.com/histograms/?dayCount=1&endDate=10-01-2015&version=... Is the granularity sufficient? If so, then I think it's fine to use it. Otherwise, up to you if you want something shorter. I'm actually ok if we want to add an UMA_HISTOGRAM_SHORT_TIMES().
https://codereview.chromium.org/1372263002/diff/80001/chrome/browser/renderer... File chrome/browser/renderer_host/thread_hop_resource_throttle.cc (right): https://codereview.chromium.org/1372263002/diff/80001/chrome/browser/renderer... chrome/browser/renderer_host/thread_hop_resource_throttle.cc:58: base::TimeDelta::FromMilliseconds(100), 100); On 2015/10/02 17:30:43, Alexei Svitkine wrote: > On 2015/10/02 17:19:42, David Benjamin wrote: > > On 2015/10/02 16:42:54, Alexei Svitkine wrote: > > > Is there a reason you picked these parameters? > > > > > > Having 100ms as the max for a timing macro seems like it would result a lot > of > > > samples in the overflow bucket. Also, why 100 buckets and not e.g. 50 that > is > > > more typical? > > > > > > For example, why not just use UMA_HISTOGRAM_TIMES()? > > > > I picked 100 because it looked like others did this too, though it seems I > > looked at exactly the wrong ones and 50's more common. :-) I'll switch to > that. > > > > UMA_HISTOGRAM_TIMES goes up to 10 seconds and I would be surprised if it goes > up > > anywhere near that high. It's just measuring a pair of thread hops. 10 / 50 is > > 0.2 seconds which is probably already high enough that I'd want to avoid the > > thread hop. Or am I completely misunderstanding what the number of buckets > means > > and their distribution? > > > > Is UMA_HISTOGRAMS_TIMES still what I want for this kind of metric? > > These macros provide a exponential distribution. So while you do have a max of > 10s, the bucket sizes near there will be much higher and you'll get more > granularity at the lower values. For example, here's the distribution from that > macro for another histogram: > > https://uma.googleplex.com/histograms/?dayCount=1&endDate=10-01-2015&version=... > > Is the granularity sufficient? If so, then I think it's fine to use it. > Otherwise, up to you if you want something shorter. I'm actually ok if we want > to add an UMA_HISTOGRAM_SHORT_TIMES(). Gotcha. UMA_HISTOGRAM_TIMES it is then! https://codereview.chromium.org/1372263002/diff/80001/chrome/browser/renderer... File chrome/browser/renderer_host/thread_hop_resource_throttle.h (right): https://codereview.chromium.org/1372263002/diff/80001/chrome/browser/renderer... chrome/browser/renderer_host/thread_hop_resource_throttle.h:27: // content::ResourceThrottle implementation: On 2015/10/02 16:42:54, Alexei Svitkine wrote: > Nit: I think the new suggested content for this type of comment is: > > // content::ResourceThrottle: Done.
lgtm
davidben@chromium.org changed reviewers: + sky@chromium.org
+sky for chrome/ OWNERS
(Also CC'ing nasko@ FYI since we were talking about this recently.)
sky: friendly ping
lgtm https://codereview.chromium.org/1372263002/diff/100001/chrome/browser/rendere... File chrome/browser/renderer_host/thread_hop_resource_throttle.cc (right): https://codereview.chromium.org/1372263002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/thread_hop_resource_throttle.cc:49: content::BrowserThread::PostTaskAndReply( What would happen if we ran a nested message loop during this time? Would the task be blocked until the nested message loop completed?
On 2015/10/06 20:06:03, sky wrote: > lgtm > > https://codereview.chromium.org/1372263002/diff/100001/chrome/browser/rendere... > File chrome/browser/renderer_host/thread_hop_resource_throttle.cc (right): > > https://codereview.chromium.org/1372263002/diff/100001/chrome/browser/rendere... > chrome/browser/renderer_host/thread_hop_resource_throttle.cc:49: > content::BrowserThread::PostTaskAndReply( > What would happen if we ran a nested message loop during this time? Would the > task be blocked until the nested message loop completed? NOT LGTM (sorry, I hit the wrong button).
https://codereview.chromium.org/1372263002/diff/100001/chrome/browser/rendere... File chrome/browser/renderer_host/thread_hop_resource_throttle.cc (right): https://codereview.chromium.org/1372263002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/thread_hop_resource_throttle.cc:49: content::BrowserThread::PostTaskAndReply( On 2015/10/06 20:06:03, sky wrote: > What would happen if we ran a nested message loop during this time? Would the > task be blocked until the nested message loop completed? I expect it would, yeah, unless that nested message loop runs random other tasks of ours. I think a lot of those dialogs now happen on other threads (see BaseShellDialogImpl), but that is definitely one thing that we'd end up blocking on, were we to go this route. I know in the past, windowed mode NPAPI on Windows could cause a things to block like UI -> plugin -> renderer, which is why sync IPCs that don't spin up renderer nested message loops can't be handled on the UI thread on Windows. jam tells me this is no longer an issue with NPAPI gone. https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... I'd expect such a loop would also block much of anything else that happens since a lot of the browser hits the UI thread for some reason or another. The alternative would be to mirror more state on the IO thread since a number of things want more detailed information at resource loader handling, but we don't currently have that information. (Except for extensions and WebView, which maintains adhoc maps.) I'd originally proposed toying with an RFHIOState or similar sort of object, but I got pushback and was asked to look into the feasibility to just hitting the UI thread more. Hence this field trial.
(Updated CL description to make it clearer that I'm only kicking this code in on a field trial.)
On 2015/10/06 20:18:44, David Benjamin wrote: > (Updated CL description to make it clearer that I'm only kicking this code in on > a field trial.) My worry is that if you initiated some load, then triggered a nested message loop for some reason the load wouldn't complete until the message loop exits. I'm worried you could hit that in practice.
davidben@chromium.org changed reviewers: + jam@chromium.org
On 2015/10/06 20:49:21, sky wrote: > On 2015/10/06 20:18:44, David Benjamin wrote: > > (Updated CL description to make it clearer that I'm only kicking this code in > on > > a field trial.) > > My worry is that if you initiated some load, then triggered a nested message > loop for some reason the load wouldn't complete until the message loop exits. > I'm worried you could hit that in practice. Are you thinking of any case in particular? If it's reachable in practice, this's also part of what this field trial would hope to discover. Most of the dialogs on Windows I know of are actually already resolved. I guess window.alert() still does this, but window.alert is application modal and will hold up everything anyway, so that's a lost cause. It's certainly something to worry about, but this would I assume be done in tandem with some application-modal dialog (otherwise everything we do on the UI thread at all is suspect). Does resource loading really need to be treated as special there? On non-Windows platforms, even before NPAPI, BrowserMessageFilter happily let you go as far as handling a sync IPC on the UI thread. +jam in case he wants to chime in, since he was the one who asked that I look into the performance impacts here.
Menus run a nested message loop. So, say you started a load in a page and then right clicked on the page. A nested message loop is running until the menu closes. There are no doubt other places. -Scott On Fri, Oct 9, 2015 at 1:20 PM, <davidben@chromium.org> wrote: > On 2015/10/06 20:49:21, sky wrote: >> >> On 2015/10/06 20:18:44, David Benjamin wrote: >> > (Updated CL description to make it clearer that I'm only kicking this >> > code > > in >> >> on >> > a field trial.) > > >> My worry is that if you initiated some load, then triggered a nested >> message >> loop for some reason the load wouldn't complete until the message loop >> exits. >> I'm worried you could hit that in practice. > > > Are you thinking of any case in particular? If it's reachable in practice, > this's also part of what this field trial would hope to discover. Most of > the > dialogs on Windows I know of are actually already resolved. I guess > window.alert() still does this, but window.alert is application modal and > will > hold up everything anyway, so that's a lost cause. > > It's certainly something to worry about, but this would I assume be done in > tandem with some application-modal dialog (otherwise everything we do on the > UI > thread at all is suspect). Does resource loading really need to be treated > as > special there? On non-Windows platforms, even before NPAPI, > BrowserMessageFilter > happily let you go as far as handling a sync IPC on the UI thread. > > +jam in case he wants to chime in, since he was the one who asked that I > look > into the performance impacts here. > > https://codereview.chromium.org/1372263002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/10/09 21:51:54, sky wrote: > Menus run a nested message loop. So, say you started a load in a page > and then right clicked on the page. A nested message loop is running > until the menu closes. There are no doubt other places. Does it still now that we're all views-based and other magic? I played with this page on Windows, Mac, Linux, and CrOS and wasn't able to get updating the navigation state and titlebar to get stuck right-clicking around. https://davidben.net/navigate-in-10-seconds.html
On 2015/10/09 22:06:21, David Benjamin wrote: > On 2015/10/09 21:51:54, sky wrote: > > Menus run a nested message loop. So, say you started a load in a page > > and then right clicked on the page. A nested message loop is running > > until the menu closes. There are no doubt other places. > > Does it still now that we're all views-based and other magic? I played with this > page on Windows, Mac, Linux, and CrOS and wasn't able to get updating the > navigation state and titlebar to get stuck right-clicking around. > > https://davidben.net/navigate-in-10-seconds.html Huh. It seems to even be impervious to another tab showing an alert(). I was actually expecting that one to trip it, but I guess we do our own dialog now and just block input?
On Fri, Oct 9, 2015 at 3:06 PM, <davidben@chromium.org> wrote: > On 2015/10/09 21:51:54, sky wrote: >> >> Menus run a nested message loop. So, say you started a load in a page >> and then right clicked on the page. A nested message loop is running >> until the menu closes. There are no doubt other places. > > > Does it still now that we're all views-based and other magic? Yes. > I played with > this > page on Windows, Mac, Linux, and CrOS and wasn't able to get updating the > navigation state and titlebar to get stuck right-clicking around. For views turns out we enable recursive task processing. See MenuMessageLoopAura. Two places that I'm pretty sure run nested message loop and don't enable recursive task processing are drag and drop (say drag a bookmark) and right clicking on the area above the tabstrip on windows. I have no doubt there are others, hence my concern. -Scott > > https://davidben.net/navigate-in-10-seconds.html > > https://codereview.chromium.org/1372263002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/10/09 22:10:07, David Benjamin wrote: > On 2015/10/09 22:06:21, David Benjamin wrote: > > On 2015/10/09 21:51:54, sky wrote: > > > Menus run a nested message loop. So, say you started a load in a page > > > and then right clicked on the page. A nested message loop is running > > > until the menu closes. There are no doubt other places. > > > > Does it still now that we're all views-based and other magic? I played with > this > > page on Windows, Mac, Linux, and CrOS and wasn't able to get updating the > > navigation state and titlebar to get stuck right-clicking around. > > > > https://davidben.net/navigate-in-10-seconds.html > > Huh. It seems to even be impervious to another tab showing an alert(). I was > actually expecting that one to trip it, but I guess we do our own dialog now and > just block input? Sorry about the repeated messages. :-) Do nested message loops just run PostTasks on Windows? I'm not very familiar with Windows message loops. But I just tried it with the certificate viewer with M45 and that didn't trip over that page either, but it doesn't stop being a nested event loop until https://crrev.com/342225 which isn't in M45.
On 2015/10/09 22:16:58, sky wrote: > On Fri, Oct 9, 2015 at 3:06 PM, <mailto:davidben@chromium.org> wrote: > > On 2015/10/09 21:51:54, sky wrote: > >> > >> Menus run a nested message loop. So, say you started a load in a page > >> and then right clicked on the page. A nested message loop is running > >> until the menu closes. There are no doubt other places. > > > > > > Does it still now that we're all views-based and other magic? > > Yes. > > > I played with > > this > > page on Windows, Mac, Linux, and CrOS and wasn't able to get updating the > > navigation state and titlebar to get stuck right-clicking around. > > For views turns out we enable recursive task processing. See > MenuMessageLoopAura. > > Two places that I'm pretty sure run nested message loop and don't > enable recursive task processing are drag and drop (say drag a > bookmark) and right clicking on the area above the tabstrip on > windows. I toyed with both of those cases and that page is at least impervious to this. Navigation certainly needs to hit the UI thread and update all kinds of state. But I'll confirm next week specifically about how this throttle behaves on for those two cases. > I have no doubt there are others, hence my concern. > > -Scott
On 2015/10/09 22:22:32, David Benjamin wrote: > On 2015/10/09 22:16:58, sky wrote: > > On Fri, Oct 9, 2015 at 3:06 PM, <mailto:davidben@chromium.org> wrote: > > > On 2015/10/09 21:51:54, sky wrote: > > >> > > >> Menus run a nested message loop. So, say you started a load in a page > > >> and then right clicked on the page. A nested message loop is running > > >> until the menu closes. There are no doubt other places. > > > > > > > > > Does it still now that we're all views-based and other magic? > > > > Yes. > > > > > I played with > > > this > > > page on Windows, Mac, Linux, and CrOS and wasn't able to get updating the > > > navigation state and titlebar to get stuck right-clicking around. > > > > For views turns out we enable recursive task processing. See > > MenuMessageLoopAura. > > > > Two places that I'm pretty sure run nested message loop and don't > > enable recursive task processing are drag and drop (say drag a > > bookmark) and right clicking on the area above the tabstrip on > > windows. > > I toyed with both of those cases and that page is at least impervious to this. > Navigation certainly needs to hit the UI thread and update all kinds of state. > But I'll confirm next week specifically about how this throttle behaves on for > those two cases. Okay, that took longer than expected, but I can confirm that right-clicking random places, dragging tabs around, dragging bookmarks around, and dragging text around all keep that thread hop intact. > > I have no doubt there are others, hence my concern. > > > > -Scott
On 2015/10/16 22:10:40, davidben wrote: > On 2015/10/09 22:22:32, David Benjamin wrote: > > On 2015/10/09 22:16:58, sky wrote: > > > On Fri, Oct 9, 2015 at 3:06 PM, <mailto:davidben@chromium.org> wrote: > > > > On 2015/10/09 21:51:54, sky wrote: > > > >> > > > >> Menus run a nested message loop. So, say you started a load in a page > > > >> and then right clicked on the page. A nested message loop is running > > > >> until the menu closes. There are no doubt other places. > > > > > > > > > > > > Does it still now that we're all views-based and other magic? > > > > > > Yes. > > > > > > > I played with > > > > this > > > > page on Windows, Mac, Linux, and CrOS and wasn't able to get updating the > > > > navigation state and titlebar to get stuck right-clicking around. > > > > > > For views turns out we enable recursive task processing. See > > > MenuMessageLoopAura. > > > > > > Two places that I'm pretty sure run nested message loop and don't > > > enable recursive task processing are drag and drop (say drag a > > > bookmark) and right clicking on the area above the tabstrip on > > > windows. > > > > I toyed with both of those cases and that page is at least impervious to this. > > Navigation certainly needs to hit the UI thread and update all kinds of state. > > But I'll confirm next week specifically about how this throttle behaves on for > > those two cases. > > Okay, that took longer than expected, but I can confirm that right-clicking > random places, dragging tabs around, dragging bookmarks around, and dragging > text around all keep that thread hop intact. > > > > I have no doubt there are others, hence my concern. > > > > > > -Scott So... thoughts? I would like to move forward on this. The alternative to moving to the UI thread would be to replicate quite a lot of UI state on the IO thread, which I was prepared to do, but I was pushed to investigating this first. I wasn't able to find a case where we have a nested event loop that doesn't enable recursive task processing like this (all of the cases cited so far have been fine). I suspect that any cases where we do still have a long-lived one but don't enable recursive task processing ought to be considered a bug since pausing the UI thread locks up everything. I can't imagine a state where we're okay locking up the UI, failing to notice the renderer navigating, and everything else, but that it's very important that new resource requests from the renderer go through. (Note that I am NOT advocating moving any of the actual network logic UI; this is just at a few key "control points" like the resource starting and redirects.) I am also intentionally doing this field trial first. The actual design won't be a ResourceThrottle anyway, but I don't want to sink time into that until we've even answered the question of whether this will work at all. Certainly there are questions about whether nested event loops and other things will be a problem, but I wasn't able to find such a case, so in absence of that, it seems this experiment is appropriate. It's easy and minimally invasive. (jam: Do you have thoughts since you were the main person to object to adding more IO thread state?)
Ok, LGTM
The CQ bit was checked by davidben@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1372263002/#ps100001 (title: "asvitkine")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1372263002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1372263002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/3cc6d6a77c53a12e37d82128620c2b5609e8f247 Cr-Commit-Position: refs/heads/master@{#356142} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
