Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
180843016:
Cache total_delayable_count for in_flight_requests in ResourceScheduler (Closed)
|
Created:
6 years, 9 months ago by hong.zheng Modified:
6 years, 8 months ago Reviewers:
James Simonsen, jochen (gone - plz use gerrit), kenneth.r.christiansen, davidben, abarth-chromium CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, mmenke Base URL:
https://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionCache total_delayable_count for in_flight_requests in ResourceScheduler
In ResourceScheduler::GetNumDelayableRequestsInFlight,
total_delayable_count has no relation to active_request_host,
so we do not need to compute it for each request.
The patch can boost BrowsingBench workload >5% performance.
BUG=347454
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266165
Patch Set 1 #
Total comments: 7
Patch Set 2 : turn Client into a class, add unittest #
Total comments: 20
Patch Set 3 : move most of logic in ResourceScheduler to Client #
Total comments: 5
Patch Set 4 : change indentation #Patch Set 5 : rebase #
Total comments: 6
Patch Set 6 : new accounting in ReprioritizeRequest #
Total comments: 2
Patch Set 7 : add delayable state to ScheduledResourceRequest #
Total comments: 12
Patch Set 8 : update accounting method #
Messages
Total messages: 55 (0 generated)
simonjam@, could you please review this CL? Thanks in advance Hong
I've never heard of this benchmark and I'm surprised something like this shows up on it. Do we run this on chromeperf.appspot.com? We should make sure there's at least one benchmark on chromeperf that shows this is an issue and that this CL improves it. https://codereview.chromium.org/180843016/diff/1/AUTHORS File AUTHORS (right): https://codereview.chromium.org/180843016/diff/1/AUTHORS#newcode142 AUTHORS:142: Hong Zheng <hong.zheng@intel.com> This needs to be in alphabetical order. https://codereview.chromium.org/180843016/diff/1/content/browser/loader/resou... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/180843016/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:180: struct ResourceScheduler::Client { This isn't a struct anymore. To keep these methods, you'd need to make it a class, make all the variables private, and add appropriate accessors, so nobody misuses it by bypassing the member functions. If we were to turn this into a class, it'd probably be best to move most of the logic in this file to Client. Basically, there are (supposed to be) two levels of scheduling: between tabs and within a tab. We only do the latter right now, though we've long talked about prioritizing the foreground tab above the rest. With two classes, we should probably divide up the work so that the ResourceScheduler is responsible for between tab priority and Client is responsible for within tab priority. https://codereview.chromium.org/180843016/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:206: void InsertFlightRequests(ScheduledResourceRequest* request) { This should be InsertInFlightRequest(). https://codereview.chromium.org/180843016/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:212: size_t EraseFlightRequests(ScheduledResourceRequest* request) { And EraseInFlightRequest(). https://codereview.chromium.org/180843016/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:222: void ClearFlightRequests() { ClearInFlightRequests() https://codereview.chromium.org/180843016/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:398: client->total_delayable_count--; Please add a unit test that shows the old code was broken and that this fixes it. That'll prevent someone from screwing it up again. https://codereview.chromium.org/180843016/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:398: client->total_delayable_count--; I don't like that total_delayable_count is sometimes modified directly and sometimes modified via member functions on Client. It either needs to be one style or the other.
Thanks for comments! On 2014/03/05 02:30:59, James Simonsen wrote: > I've never heard of this benchmark and I'm surprised something like this shows > up on it. The BrowsingBench™ benchmark provides a standardized, industry-accepted method of evaluating Web browser performance. http://www.eembc.org/benchmark/browsing_sl.php. The CL can improve >5% performance in BrowsingBench on Samsung Galaxy S4 >Do we run this on chromeperf.appspot.com? We should make sure there's > at least one benchmark on chromeperf that shows this is an issue and that this > CL improves it. I don't know how to run benchmarks on chromeperf. Does everyone have the access to run? Could you tell me how to do it, or could you please help me to do it? Thanks in advance. > https://codereview.chromium.org/180843016/diff/1/AUTHORS > File AUTHORS (right): > > https://codereview.chromium.org/180843016/diff/1/AUTHORS#newcode142 > AUTHORS:142: Hong Zheng <mailto:hong.zheng@intel.com> > This needs to be in alphabetical order. Done > https://codereview.chromium.org/180843016/diff/1/content/browser/loader/resou... > File content/browser/loader/resource_scheduler.cc (right): > > https://codereview.chromium.org/180843016/diff/1/content/browser/loader/resou... > content/browser/loader/resource_scheduler.cc:180: struct > ResourceScheduler::Client { > This isn't a struct anymore. To keep these methods, you'd need to make it a > class, make all the variables private, and add appropriate accessors, so nobody > misuses it by bypassing the member functions. Done > If we were to turn this into a class, it'd probably be best to move most of the > logic in this file to Client. Basically, there are (supposed to be) two levels > of scheduling: between tabs and within a tab. We only do the latter right now, > though we've long talked about prioritizing the foreground tab above the rest. > With two classes, we should probably divide up the work so that the > ResourceScheduler is responsible for between tab priority and Client is > responsible for within tab priority. I agreed with you and thought we could divide this work into two steps. First, turn Client into a class, like this CL. Second, separate Client from ResourceScheduler. Is it OK? > https://codereview.chromium.org/180843016/diff/1/content/browser/loader/resou... > content/browser/loader/resource_scheduler.cc:206: void > InsertFlightRequests(ScheduledResourceRequest* request) { > This should be InsertInFlightRequest(). Done > https://codereview.chromium.org/180843016/diff/1/content/browser/loader/resou... > content/browser/loader/resource_scheduler.cc:212: size_t > EraseFlightRequests(ScheduledResourceRequest* request) { > And EraseInFlightRequest(). Done > https://codereview.chromium.org/180843016/diff/1/content/browser/loader/resou... > content/browser/loader/resource_scheduler.cc:222: void ClearFlightRequests() { > ClearInFlightRequests() Done > https://codereview.chromium.org/180843016/diff/1/content/browser/loader/resou... > content/browser/loader/resource_scheduler.cc:398: > client->total_delayable_count--; > Please add a unit test that shows the old code was broken and that this fixes > it. That'll prevent someone from screwing it up again. Done > https://codereview.chromium.org/180843016/diff/1/content/browser/loader/resou... > content/browser/loader/resource_scheduler.cc:398: > client->total_delayable_count--; > I don't like that total_delayable_count is sometimes modified directly and > sometimes modified via member functions on Client. It either needs to be one > style or the other. Done
PTAL Thanks
Sorry I was away on vacation. I'll get to the rest of this in a bit, but I wanted to respond to this now since it's quick... On 2014/03/12 08:49:45, hong.zheng wrote: > Thanks for comments! > > On 2014/03/05 02:30:59, James Simonsen wrote: > > I've never heard of this benchmark and I'm surprised something like this shows > > up on it. > The BrowsingBench™ benchmark provides a standardized, industry-accepted method > of evaluating Web browser performance. > http://www.eembc.org/benchmark/browsing_sl.php. The CL can improve >5% > performance in BrowsingBench on Samsung Galaxy S4 > >Do we run this on chromeperf.appspot.com? We should make sure there's > > at least one benchmark on chromeperf that shows this is an issue and that this > > CL improves it. > > I don't know how to run benchmarks on chromeperf. Does everyone have the access > to run? Could you tell me how to do it, or could you please help me to do it? > Thanks in advance. All of the benchmarks are run via Telemetry. You can run all Telemetry benchmarks locally. The chromeperf dashboard just shows a plot from running all our Telemetry benchmarks against a continuous build on known hardware. You can learn about Telemetry here: http://www.chromium.org/developers/telemetry Specifically, I would look into how feasible it is to turn BrowsingBench into a page set and extract measurements from it. I'm not an expert here, you might want to reach out to tonyg@ who can direct you to the right person to help.
The CQ bit was checked by hong.zheng@intel.com
The CQ bit was unchecked by hong.zheng@intel.com
Thanks for your reply and help. On 2014/03/17 21:11:55, James Simonsen wrote: > Sorry I was away on vacation. I'll get to the rest of this in a bit, but I > wanted to respond to this now since it's quick... > > On 2014/03/12 08:49:45, hong.zheng wrote: > > Thanks for comments! > > > > On 2014/03/05 02:30:59, James Simonsen wrote: > > > I've never heard of this benchmark and I'm surprised something like this > shows > > > up on it. > > The BrowsingBench™ benchmark provides a standardized, industry-accepted method > > of evaluating Web browser performance. > > http://www.eembc.org/benchmark/browsing_sl.php. The CL can improve >5% > > performance in BrowsingBench on Samsung Galaxy S4 > > >Do we run this on chromeperf.appspot.com? We should make sure there's > > > at least one benchmark on chromeperf that shows this is an issue and that > this > > > CL improves it. > > > > I don't know how to run benchmarks on chromeperf. Does everyone have the > access > > to run? Could you tell me how to do it, or could you please help me to do it? > > Thanks in advance. > > All of the benchmarks are run via Telemetry. You can run all Telemetry > benchmarks locally. The chromeperf dashboard just shows a plot from running all > our Telemetry benchmarks against a continuous build on known hardware. You can > learn about Telemetry here: > > http://www.chromium.org/developers/telemetry > I learned about Telemetry and collected the results(attached in http://code.google.com/p/chromium/issues/detail?id=347454 ). But I don't know how to analyse the result. Could you please help me to analyse it or coach me how to do it? Thanks in advance. I used below command to collect result in Samsung Note3. tools/perf/run_benchmark -v --browser=android-content-shell --allow-live-sites page_cycler.top_10_mobile In addition, I don't have google.com account to download src-internal(Telemetry recommended). And I tried to record a Page Set, but failed. command: tools/perf/record_wpr --browser=android-content-shell tools/perf/page_sets/top_10_mobile.json failed log: " Traceback (most recent call last): <module> at tools/perf/record_wpr:17 sys.exit(record_wpr.Main(_perf_dir)) Main at tools/telemetry/telemetry/page/record_wpr.py:160 results = page_runner.Run(recorder, ps, expectations, options) Run at tools/telemetry/telemetry/page/page_runner.py:311 pages = _ShuffleAndFilterPageSet(page_set, finder_options) _ShuffleAndFilterPageSet at tools/telemetry/telemetry/page/page_runner.py:392 if not page.disabled and page_filter.PageFilter.IsSelected(page)] IsSelected at tools/telemetry/telemetry/page/page_filter.py:63 if cls._exclude_labels and HasLabelIn(page, cls._exclude_labels): Locals: cls : <class 'telemetry.page.page_filter.PageFilter'> page : <telemetry.page.page.Page object at 0x2ef4310> AttributeError: type object 'PageFilter' has no attribute '_exclude_labels' " Only use "--allow-live-sites" results to analyse is OK? > Specifically, I would look into how feasible it is to turn BrowsingBench into a > page set and extract measurements from it. I'm not an expert here, you might > want to reach out to tonyg@ who can direct you to the right person to help.
On 2014/03/19 11:59:41, hong.zheng wrote: > Thanks for your reply and help. > > On 2014/03/17 21:11:55, James Simonsen wrote: > > Sorry I was away on vacation. I'll get to the rest of this in a bit, but I > > wanted to respond to this now since it's quick... > > > > On 2014/03/12 08:49:45, hong.zheng wrote: > > > Thanks for comments! > > > > > > On 2014/03/05 02:30:59, James Simonsen wrote: > > > > I've never heard of this benchmark and I'm surprised something like this > > shows > > > > up on it. > > > The BrowsingBench™ benchmark provides a standardized, industry-accepted > method > > > of evaluating Web browser performance. > > > http://www.eembc.org/benchmark/browsing_sl.php. The CL can improve >5% > > > performance in BrowsingBench on Samsung Galaxy S4 > > > >Do we run this on chromeperf.appspot.com? We should make sure there's > > > > at least one benchmark on chromeperf that shows this is an issue and that > > this > > > > CL improves it. > > > > > > I don't know how to run benchmarks on chromeperf. Does everyone have the > > access > > > to run? Could you tell me how to do it, or could you please help me to do > it? > > > Thanks in advance. > > > > All of the benchmarks are run via Telemetry. You can run all Telemetry > > benchmarks locally. The chromeperf dashboard just shows a plot from running > all > > our Telemetry benchmarks against a continuous build on known hardware. You can > > learn about Telemetry here: > > > > http://www.chromium.org/developers/telemetry > > > > I learned about Telemetry and collected the results(attached in > http://code.google.com/p/chromium/issues/detail?id=347454 ). But I don't know > how to analyse the result. Could you please help me to analyse it or coach me > how to do it? Thanks in advance. > I used below command to collect result in Samsung Note3. > tools/perf/run_benchmark -v --browser=android-content-shell --allow-live-sites > page_cycler.top_10_mobile > > In addition, I don't have http://google.com account to download src-internal(Telemetry > recommended). And I tried to record a Page Set, but failed. > command: tools/perf/record_wpr --browser=android-content-shell > tools/perf/page_sets/top_10_mobile.json > failed log: > " > Traceback (most recent call last): > <module> at tools/perf/record_wpr:17 > sys.exit(record_wpr.Main(_perf_dir)) > Main at tools/telemetry/telemetry/page/record_wpr.py:160 > results = page_runner.Run(recorder, ps, expectations, options) > Run at tools/telemetry/telemetry/page/page_runner.py:311 > pages = _ShuffleAndFilterPageSet(page_set, finder_options) > _ShuffleAndFilterPageSet at tools/telemetry/telemetry/page/page_runner.py:392 > if not page.disabled and page_filter.PageFilter.IsSelected(page)] > IsSelected at tools/telemetry/telemetry/page/page_filter.py:63 > if cls._exclude_labels and HasLabelIn(page, cls._exclude_labels): > > Locals: > cls : <class 'telemetry.page.page_filter.PageFilter'> > page : <telemetry.page.page.Page object at 0x2ef4310> > > AttributeError: type object 'PageFilter' has no attribute '_exclude_labels' > " > > Only use "--allow-live-sites" results to analyse is OK? Honestly, I don't even work on Chromium any more. I don't remember how to do this. I'm just stuck in this OWNERS file. I think this group should be able to help you out with Telemetry: https://groups.google.com/a/chromium.org/forum/#!forum/telemetry You can also reach them by sending an email to telemetry@chromium.org I'll get back to the review soon. Sorry for the delay.
On 2014/03/19 20:31:09, James Simonsen wrote: > Honestly, I don't even work on Chromium any more. I don't remember how to do > this. I'm just stuck in this OWNERS file. > > I think this group should be able to help you out with Telemetry: > > https://groups.google.com/a/chromium.org/forum/#%21forum/telemetry > > You can also reach them by sending an email to mailto:telemetry@chromium.org > > I'll get back to the review soon. Sorry for the delay. Thanks for your quick reply. Hope you can early come back and review the CL :-)
On 2014/03/12 08:49:45, hong.zheng wrote: > > If we were to turn this into a class, it'd probably be best to move most of > the > > logic in this file to Client. Basically, there are (supposed to be) two levels > > of scheduling: between tabs and within a tab. We only do the latter right now, > > though we've long talked about prioritizing the foreground tab above the rest. > > With two classes, we should probably divide up the work so that the > > ResourceScheduler is responsible for between tab priority and Client is > > responsible for within tab priority. > > I agreed with you and thought we could divide this work into two steps. First, > turn Client into a class, like this CL. Second, separate Client from > ResourceScheduler. Is it OK? I don't think it's sensible in the state it's in now. It really should either just be a simple struct (simpler than what you had earlier) or switch entirely to the class. It's fine to do that in separate CLs, but I don't want it to be half of each. https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:204: RequestQueue& pending_requests() { I think these accessors should go away (read later comments), but if they were to stick around, we don't return mutable references in our style guide. You should return a pointer instead. https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:212: bool IsDelayableRequestInFlightRequest(ScheduledResourceRequest* r) { IsDelayableInFlightRequest() https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:212: bool IsDelayableRequestInFlightRequest(ScheduledResourceRequest* r) { Avoid abbreviations: r -> request https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:213: bool contained = ContainsKey(in_flight_requests_, r); I think it'd be perfectly readable to just have: return ContainsKey(...) && IsDelayableRequest(...); https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:237: void InsertInFlightRequests(ScheduledResourceRequest* request) { Should be singular. You're only inserting one. InsertInFlightRequest. https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:243: size_t EraseInFlightRequests(ScheduledResourceRequest* request) { Make singular. https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:311: client->pending_requests().Insert(request.get(), url_request->priority()); pending_requests is now an implementation detail of Client. Instead of accessing it directly, you should provide functions on Client that do what you want. So now, these functions should look like so: scoped_ptr<ResourceThrottle> ResourceScheduler::ScheduleRequest(...) { ... Client *client = it->second; client->ScheduleRequest(request); return request.PassAs<ResourceThrottle>(); } And of course, all of the code that is here should move to a new function: Client::ScheduleRequest(). https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:330: if (client->pending_requests().IsQueued(request)) { Same. Move this code to Client::RemoveRequest(). https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:360: // FYI, ResourceDispatcherHost cancels all of the requests after this function Same. Move this to Client::RemoveAllRequests(). https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:398: client->set_has_body(true); Probably want to make this client->SetHasBody(true) and call LoadAnyStartablePendingRequests() there (if has_body is true). FYI, you only use set_has_body style if it's a simple method, but if it calls the other function, it's no longer simple, so you have to change to SetHasBody style. https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:416: client->set_using_spdy_proxy(true); Follow the same pattern as the previous one. https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:421: void ResourceScheduler::StartRequest(ScheduledResourceRequest* request, This whole function should move to Client. It should only be called from other functions on client. https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:444: Client *client = client_it->second; Move to Client::ReprioritizeRequest(). https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:445: bool is_delayable_request = Rename to was_delayable_request. https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:455: if (client->IsDelayableRequestInFlightRequest(request)) This looks redundant at first glance, because you made the exact same call earlier. Can you put this in a new bool like so: bool is_now_delayable_request = client->IsDelayable... Then this can read: else if (!was_delayable_request && is_now_delayable_request) I think that's a lot easier to understand. https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:464: request->url_request()->priority()); nit: indenting is wrong now. https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:472: void ResourceScheduler::LoadAnyStartablePendingRequests(Client* client) { This whole function can move to Client. https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:509: size_t ResourceScheduler::GetNumSameHostRequestsInFlight( This whole function can move to Client. https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:527: // ShouldStartRequest is the main scheduling algorithm. This whole function can move to Client. https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:606: int ResourceScheduler::GetDelayableCountInFlightRequest( This probably won't be needed anymore.
Thank you. On 2014/03/21 01:58:09, James Simonsen wrote: > On 2014/03/12 08:49:45, hong.zheng wrote: > I don't think it's sensible in the state it's in now. It really should either > just be a simple struct (simpler than what you had earlier) or switch entirely > to the class. It's fine to do that in separate CLs, but I don't want it to be > half of each. > Done. Moved most of logic into Client > https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... > File content/browser/loader/resource_scheduler.cc (right): > > https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:204: RequestQueue& > pending_requests() { > I think these accessors should go away (read later comments), but if they were > to stick around, we don't return mutable references in our style guide. You > should return a pointer instead. > Done. > https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:212: bool > IsDelayableRequestInFlightRequest(ScheduledResourceRequest* r) { > IsDelayableInFlightRequest() > Done. > https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:212: bool > IsDelayableRequestInFlightRequest(ScheduledResourceRequest* r) { > Avoid abbreviations: r -> request > Done. > https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:213: bool contained = > ContainsKey(in_flight_requests_, r); > I think it'd be perfectly readable to just have: > > return ContainsKey(...) && IsDelayableRequest(...); > Done. > https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:237: void > InsertInFlightRequests(ScheduledResourceRequest* request) { > Should be singular. You're only inserting one. InsertInFlightRequest. > Done. > https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:243: size_t > EraseInFlightRequests(ScheduledResourceRequest* request) { > Make singular. > Done. > https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:311: > client->pending_requests().Insert(request.get(), url_request->priority()); > pending_requests is now an implementation detail of Client. Instead of accessing > it directly, you should provide functions on Client that do what you want. > > So now, these functions should look like so: > > scoped_ptr<ResourceThrottle> ResourceScheduler::ScheduleRequest(...) { > ... > Client *client = it->second; > client->ScheduleRequest(request); > return request.PassAs<ResourceThrottle>(); > } > > And of course, all of the code that is here should move to a new function: > Client::ScheduleRequest(). > Done. > https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:330: if > (client->pending_requests().IsQueued(request)) { > Same. Move this code to Client::RemoveRequest(). > Done. > https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:360: // FYI, ResourceDispatcherHost > cancels all of the requests after this function > Same. Move this to Client::RemoveAllRequests(). > Done. > https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:398: client->set_has_body(true); > Probably want to make this client->SetHasBody(true) and call > LoadAnyStartablePendingRequests() there (if has_body is true). FYI, you only use > set_has_body style if it's a simple method, but if it calls the other function, > it's no longer simple, so you have to change to SetHasBody style. > Done. Move them into a function Client::OnWillInsertBody > https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:416: > client->set_using_spdy_proxy(true); > Follow the same pattern as the previous one. > Done. > https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:421: void > ResourceScheduler::StartRequest(ScheduledResourceRequest* request, > This whole function should move to Client. It should only be called from other > functions on client. > Done. > https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:444: Client *client = > client_it->second; > Move to Client::ReprioritizeRequest(). > Done. > https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:445: bool is_delayable_request = > Rename to was_delayable_request. > Done. > https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:455: if > (client->IsDelayableRequestInFlightRequest(request)) > This looks redundant at first glance, because you made the exact same call > earlier. Can you put this in a new bool like so: > > bool is_now_delayable_request = client->IsDelayable... > > Then this can read: > > else if (!was_delayable_request && is_now_delayable_request) > > I think that's a lot easier to understand. > Done. > https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:464: > request->url_request()->priority()); > nit: indenting is wrong now. > Done. > https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:472: void > ResourceScheduler::LoadAnyStartablePendingRequests(Client* client) { > This whole function can move to Client. > Done. > https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:509: size_t > ResourceScheduler::GetNumSameHostRequestsInFlight( > This whole function can move to Client. > Done. > https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:527: // ShouldStartRequest is the > main scheduling algorithm. > This whole function can move to Client. > Done. > https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:606: int > ResourceScheduler::GetDelayableCountInFlightRequest( > This probably won't be needed anymore. This function is designed for unittests
This is looking great. Thanks! Any progress on the Telemetry benchmark? It's important that we track this. I'd always thought this code wasn't critical for performance, but apparently your benchmark says otherwise. It's possible we're missing other things too. https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... > > content/browser/loader/resource_scheduler.cc:606: int > > ResourceScheduler::GetDelayableCountInFlightRequest( > > This probably won't be needed anymore. > > This function is designed for unittests Tests shouldn't need to peek at the internal state of the object. They should just observe the external behavior of the object. We should have a test that shows the old code misbehaved by scheduling incorrectly and that it now schedules the correct requests at the correct time. https://codereview.chromium.org/180843016/diff/40001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/180843016/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:202: void RemoveRequest(ScheduledResourceRequest* request) { The indentation is wrong for this function and all of the following ones. It should be two spaces, like ScheduleRequest() above. https://codereview.chromium.org/180843016/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:215: scoped_ptr<RequestSet> RemoveAllRequests() { You can just return a RequestSet here. No need for the scoped_ptr. RVO will prevent the copy. https://codereview.chromium.org/180843016/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:242: net::RequestPriority old_priority, Indentation is wrong here. These args should line up with the line above. Have you tried using clang-format? https://codereview.chromium.org/180843016/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:524: scoped_ptr<RequestSet> client_unowened_requests = client->RemoveAllRequests(); unowened -> unowned https://codereview.chromium.org/180843016/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:589: request->url_request()->SetPriority(new_priority); I'd move this down a line, after the comment.
Thanks On 2014/03/27 22:19:56, James Simonsen wrote: > This is looking great. Thanks! > > Any progress on the Telemetry benchmark? It's important that we track this. I'd > always thought this code wasn't critical for performance, but apparently your > benchmark says otherwise. It's possible we're missing other things too. > I can't run telemetry successfully, and report a bug. https://code.google.com/p/chromium/issues/detail?id=354721 As we talked, i think the CL can land on its own without a Telemetry test > https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... > > > content/browser/loader/resource_scheduler.cc:606: int > > > ResourceScheduler::GetDelayableCountInFlightRequest( > > > This probably won't be needed anymore. > > > > This function is designed for unittests > > Tests shouldn't need to peek at the internal state of the object. They should > just observe the external behavior of the object. > > We should have a test that shows the old code misbehaved by scheduling > incorrectly and that it now schedules the correct requests at the correct time. > Because the CL doesn't change the schedule behavior, it only records delayable request count, which is internal state. Therefore to delete ResourceScheduler::GetDelayableCountInFlightRequest > https://codereview.chromium.org/180843016/diff/40001/content/browser/loader/r... > File content/browser/loader/resource_scheduler.cc (right): > > https://codereview.chromium.org/180843016/diff/40001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:202: void > RemoveRequest(ScheduledResourceRequest* request) { > The indentation is wrong for this function and all of the following ones. It > should be two spaces, like ScheduleRequest() above. > Done > https://codereview.chromium.org/180843016/diff/40001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:215: scoped_ptr<RequestSet> > RemoveAllRequests() { > You can just return a RequestSet here. No need for the scoped_ptr. RVO will > prevent the copy. > Done > https://codereview.chromium.org/180843016/diff/40001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:242: net::RequestPriority > old_priority, > Indentation is wrong here. These args should line up with the line above. Have > you tried using clang-format? > Done. I didn't use clang-format, which may be wrong indentation reason. > https://codereview.chromium.org/180843016/diff/40001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:524: scoped_ptr<RequestSet> > client_unowened_requests = client->RemoveAllRequests(); > unowened -> unowned > Done > https://codereview.chromium.org/180843016/diff/40001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:589: > request->url_request()->SetPriority(new_priority); > I'd move this down a line, after the comment. Done
lgtm Looks great. Thanks for your patience. On 2014/04/03 11:49:47, hong.zheng wrote: https://codereview.chromium.org/180843016/diff/20001/content/browser/loader/r... > > > > content/browser/loader/resource_scheduler.cc:606: int > > > > ResourceScheduler::GetDelayableCountInFlightRequest( > > > > This probably won't be needed anymore. > > > > > > This function is designed for unittests > > > > Tests shouldn't need to peek at the internal state of the object. They should > > just observe the external behavior of the object. > > > > We should have a test that shows the old code misbehaved by scheduling > > incorrectly and that it now schedules the correct requests at the correct > time. > > > > Because the CL doesn't change the schedule behavior, it only records delayable > request count, which is internal state. Therefore to delete > ResourceScheduler::GetDelayableCountInFlightRequest Oh I see. I misunderstood earlier. Yeah, you're right, we don't need that unit test.
The CQ bit was checked by hong.zheng@intel.com
The CQ bit was unchecked by hong.zheng@intel.com
The CQ bit was checked by hong.zheng@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hong.zheng@intel.com/180843016/60001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for AUTHORS: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file AUTHORS Hunk #1 FAILED at 118. 1 out of 1 hunk FAILED -- saving rejects to file AUTHORS.rej Patch: AUTHORS Index: AUTHORS diff --git a/AUTHORS b/AUTHORS index 349f73aa017c75c86a6c72d7cc68d796fce4d4d1..9d0fba65ee7c16e7ad30adfa3fe3527e1cd3d6aa 100644 --- a/AUTHORS +++ b/AUTHORS @@ -118,6 +118,7 @@ Haitao Feng <haitao.feng@intel.com> Halton Huo <halton.huo@intel.com> Haojian Wu <hokein.wu@gmail.com> Hautio Kari <khautio@gmail.com> +Hong Zheng <hong.zheng@intel.com> Hongbo Min <hongbo.min@intel.com> Horia Olaru <horia.olaru@gmail.com> Horia Olaru <olaru@adobe.com>
The CQ bit was checked by hong.zheng@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hong.zheng@intel.com/180843016/80001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
The CQ bit was unchecked by hong.zheng@intel.com
The CQ bit was checked by hong.zheng@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hong.zheng@intel.com/180843016/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel tryserver.chromium on linux_chromium_rel
The CQ bit was checked by hong.zheng@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hong.zheng@intel.com/180843016/80001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
The CQ bit was checked by hong.zheng@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hong.zheng@intel.com/180843016/80001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
The CQ bit was checked by hong.zheng@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hong.zheng@intel.com/180843016/80001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
davidben@, PTAL, because simonjam@ has been removed from the OWNERS file.
Sorry about having comments for you after you already got a review from simonjam. Mostly style nits, but there is one thing I noticed that might cause the accounting to get a little off. https://codereview.chromium.org/180843016/diff/80001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/180843016/diff/80001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:232: } Nit: Doesn't look like this is ever called outside ResourceScheduler::Client. You can probably just refer to the member directly. https://codereview.chromium.org/180843016/diff/80001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:260: it != in_flight_requests_.end(); ++it) { Nit: judging from codesearch, I think the style is to indent this to the paren. (One more space.) https://codereview.chromium.org/180843016/diff/80001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:302: } Hrm. Would this block get confused if we create a request before we learn something something supports SPDY and then reprioritize it after? (Or the inverse, though it looks like we never remove the supports SPDY bit? That's odd.) Admittedly, this is a really contrived case since the top-level is presumably going to be over the network and we'll learn at that point that SPDY is supported. I suppose it could be cached or something... What if there was some state on the ScheduledResourceRequest for tracking this, or if Client maintained two sets of requests, in_flight_requests_ + delayable_in_flight_requests_? https://codereview.chromium.org/180843016/diff/80001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:355: ScheduledResourceRequest* request) { Nit: This line should be indented only 6 spaces (2 + 4). https://codereview.chromium.org/180843016/diff/80001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:364: it != in_flight_requests_.end(); ++it) { Nit: Indent this line to the parenthesis. https://codereview.chromium.org/180843016/diff/80001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:493: size_t total_delayable_count_; Nit: Could you add a comment here, something along the lines of: // The number of delayable in-flight requests. I had to read it for a bit to be sure this is specifically the in-flight ones.
Thanks for comments. On 2014/04/16 16:44:09, David Benjamin wrote: > Sorry about having comments for you after you already got a review from > simonjam. Mostly style nits, but there is one thing I noticed that might cause > the accounting to get a little off. > > https://codereview.chromium.org/180843016/diff/80001/content/browser/loader/r... > File content/browser/loader/resource_scheduler.cc (right): > > https://codereview.chromium.org/180843016/diff/80001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:232: } > Nit: Doesn't look like this is ever called outside ResourceScheduler::Client. > You can probably just refer to the member directly. > Done. > https://codereview.chromium.org/180843016/diff/80001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:260: it != > in_flight_requests_.end(); ++it) { > Nit: judging from codesearch, I think the style is to indent this to the paren. > (One more space.) > Done. > https://codereview.chromium.org/180843016/diff/80001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:302: } > Hrm. Would this block get confused if we create a request before we learn > something something supports SPDY and then reprioritize it after? (Or the > inverse, though it looks like we never remove the supports SPDY bit? That's > odd.) Admittedly, this is a really contrived case since the top-level is > presumably going to be over the network and we'll learn at that point that SPDY > is supported. I suppose it could be cached or something... > The old accounting method in ReprioritizeRequest has problem in special case. I currently use another method to account. > What if there was some state on the ScheduledResourceRequest for tracking this, > or if Client maintained two sets of requests, in_flight_requests_ + > delayable_in_flight_requests_? > We know in_flight_requests_ contains two parts: immediately issued requests and delayable requests, and delayable requests never exceed 10. I think it isn't necessary to divide in_flight_request into in_flight_requests_ + delayable_in_flight_requests_. We only care the delayable requests count, not care they are immediately_issued_requests or delayable_requests > https://codereview.chromium.org/180843016/diff/80001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:355: ScheduledResourceRequest* > request) { > Nit: This line should be indented only 6 spaces (2 + 4). > Done. > https://codereview.chromium.org/180843016/diff/80001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:364: it != > in_flight_requests_.end(); ++it) { > Nit: Indent this line to the parenthesis. > Done. > https://codereview.chromium.org/180843016/diff/80001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:493: size_t total_delayable_count_; > Nit: Could you add a comment here, something along the lines of: > > // The number of delayable in-flight requests. > > I had to read it for a bit to be sure this is specifically the in-flight ones. Done.
https://codereview.chromium.org/180843016/diff/100001/content/browser/loader/... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/180843016/diff/100001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:282: bool was_delayable_request = IsDelayableInFlightRequest(request); Sorry, I wasn't clear, though this version is definitely cleaner. I meant that IsDelayableInFlightRequest() here may return a different value than when the request had started. In fact, it's actually easier to trigger. Imagining the following less contrived scenario: 1. Open some page somewhere on example.com. 2. Page makes a low-priority request to a.example.com, a SPDY-supporting server. At this point, we've never seen a.example.com before, so http_server_properties.SupportsSpdy returns false. Request adds to total_delayable_count_. 3. SPDY is negotiated and we persist that a.example.com supports SPDY. 4. Request completes. Now that SupportsSpdy is true, it is not counted as delayable, so total_delayable_count_ is NOT decremented. 5. For more fun, repeat steps 2-4 10 times. Now you're stuck. :-) Hence which I suggested adding state to ScheduledResourceRequest or having two separate buckets. Test coverage for this case is probably also a good plan, as it is subtle. Certainly a comment. https://codereview.chromium.org/180843016/diff/100001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:327: return erased; Nit: Move the DCHECK in RemoveRequest to here and have this return void. Maybe even DCHECK_EQ(1u, erased). If erased isn't 1, the total_delayable_count_ logic is wrong anyway.
Sorry for late reply, because of a travel On 2014/04/17 18:38:06, David Benjamin wrote: > https://codereview.chromium.org/180843016/diff/100001/content/browser/loader/... > File content/browser/loader/resource_scheduler.cc (right): > > https://codereview.chromium.org/180843016/diff/100001/content/browser/loader/... > content/browser/loader/resource_scheduler.cc:282: bool was_delayable_request = > IsDelayableInFlightRequest(request); > Sorry, I wasn't clear, though this version is definitely cleaner. I meant that > IsDelayableInFlightRequest() here may return a different value than when the > request had started. > > In fact, it's actually easier to trigger. Imagining the following less contrived > scenario: > > 1. Open some page somewhere on http://example.com. > > 2. Page makes a low-priority request to http://a.example.com, a SPDY-supporting server. > At this point, we've never seen http://a.example.com before, so > http_server_properties.SupportsSpdy returns false. Request adds to > total_delayable_count_. > > 3. SPDY is negotiated and we persist that http://a.example.com supports SPDY. > > 4. Request completes. Now that SupportsSpdy is true, it is not counted as > delayable, so total_delayable_count_ is NOT decremented. > > 5. For more fun, repeat steps 2-4 10 times. Now you're stuck. :-) > > Hence which I suggested adding state to ScheduledResourceRequest or having two > separate buckets. Test coverage for this case is probably also a good plan, as > it is subtle. Certainly a comment. > Done. I misunderstood you last time. Thanks for your detailed explanation. > https://codereview.chromium.org/180843016/diff/100001/content/browser/loader/... > content/browser/loader/resource_scheduler.cc:327: return erased; > Nit: Move the DCHECK in RemoveRequest to here and have this return void. Maybe > even DCHECK_EQ(1u, erased). If erased isn't 1, the total_delayable_count_ logic > is wrong anyway. Done
Thanks for the test! A few additional comments below. https://codereview.chromium.org/180843016/diff/120001/content/browser/loader/... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/180843016/diff/120001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:201: // the state of whether the request is delayable in in_flight_requests_ Style nit: we use capitals and periods in our comments. // True if the request is delayable in |in_flight_requests_|. https://codereview.chromium.org/180843016/diff/120001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:296: //the priority and SPDY-supporting may be changed, so judge again Style nit: Capitalize and period, space after //. // The priority and SPDY support may have changed, so update the // delayable count. https://codereview.chromium.org/180843016/diff/120001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:331: if (in_flight_requests_.size() == 0) { Sorry, I missed this earlier. Is this just a special-case for if something goes wrong with the accounting? We should getting the accounting correct in the first place. Could you make this always use the normal codepath and add a DCHECK like: DCHECK_LE(total_delayable_count_, in_flight_requests_.size()); https://codereview.chromium.org/180843016/diff/120001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:359: ScheduledResourceRequest* request, bool add) { This function is a little bizarre. The invariant we actually want to maintain is total_delayable_count_ == number of requests with accounted_as_delayable_request bit true. So how about making this function instead be void SetRequestDelayable(ScheduledResourceRequest* request, bool delayable) { if (request->accounted_as_delayable_request() == delayable) return; if (delayable) total_delayable_count_++; else total_delayable_count_--; request->set_accounted_as_delayable_request(delayable); } Then ReprioritizeRequest and EraseInFlightRequest don't need to actually check accounted_as_delayable_request, just call this with IsDelayableRequest(request) and false, respectively. https://codereview.chromium.org/180843016/diff/120001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:365: } Style nit: newline. https://codereview.chromium.org/180843016/diff/120001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:438: if (num_delayable_requests_in_flight >= kMaxNumDelayableRequestsPerClient) { Just realized there is one other behavior change relating to the SPDY thing: Say you spray 10 image loads to a SPDY-capable server that we haven't learned is SPDY-capable. Then we learn it's SPDY-capable. Now you make another image load to a non-SPDY capable server. Before, that image load would go through because we'd re-evaluate delayability. With this CL we wouldn't. I think this is fine. The old logic I believe also didn't work if, say, the non-SPDY-capable image load came before we learned the first server was SPDY-capable. I just wanted to mention it. https://codereview.chromium.org/180843016/diff/120001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:442: if (ShouldKeepSearching(host_port_pair)) { By the way, this CL doesn't avoid the O(N) iteration if your particular benchmark were to spray a non-delayable request like an XHR or scripts or styles rather than spray (I'm guessing) image loads. I suppose spraying images is conceivable for some image-heavy site. I don't know how common an XHR spray would be. (If there were only delayable requests, then it would cut out the O(N) iteration; either you have more than 10 delayable, or you have under 10 in which case iterating over each isn't too bad.) Don't need to fix it for this CL since it's already kinda big, but it might be something we could consider. https://codereview.chromium.org/180843016/diff/120001/content/browser/loader/... File content/browser/loader/resource_scheduler_unittest.cc (right): https://codereview.chromium.org/180843016/diff/120001/content/browser/loader/... content/browser/loader/resource_scheduler_unittest.cc:505: TEST_F(ResourceSchedulerTest, newSpdyHostInDelayableRequests) { Style nit: NewSpdyHostInDelayableRequest (capital N) https://codereview.chromium.org/180843016/diff/120001/content/browser/loader/... content/browser/loader/resource_scheduler_unittest.cc:509: scoped_ptr<TestRequest> low1_spdy(NewRequest("http://spdyhost1:8080/low", net::LOWEST)); Style nit: Wrap this to 80 chars. https://codereview.chromium.org/180843016/diff/120001/content/browser/loader/... content/browser/loader/resource_scheduler_unittest.cc:509: scoped_ptr<TestRequest> low1_spdy(NewRequest("http://spdyhost1:8080/low", net::LOWEST)); Maybe add a comment here so it's clear what's being tested: // Cancel a request after we learn the server supports SPDY. https://codereview.chromium.org/180843016/diff/120001/content/browser/loader/... content/browser/loader/resource_scheduler_unittest.cc:523: scoped_ptr<TestRequest> low2_spdy(NewRequest("http://spdyhost2:8080/low", net::IDLE)); Style nit: Wrap this to 80 chars. https://codereview.chromium.org/180843016/diff/120001/content/browser/loader/... content/browser/loader/resource_scheduler_unittest.cc:523: scoped_ptr<TestRequest> low2_spdy(NewRequest("http://spdyhost2:8080/low", net::IDLE)); Maybe add a comment here so it's clear what's being tested: // Reprioritize a request after we learn the server supports SPDY.
Thanks for comments. On 2014/04/23 19:14:34, David Benjamin wrote: > Thanks for the test! A few additional comments below. > > https://codereview.chromium.org/180843016/diff/120001/content/browser/loader/... > File content/browser/loader/resource_scheduler.cc (right): > > https://codereview.chromium.org/180843016/diff/120001/content/browser/loader/... > content/browser/loader/resource_scheduler.cc:201: // the state of whether the > request is delayable in in_flight_requests_ > Style nit: we use capitals and periods in our comments. > > // True if the request is delayable in |in_flight_requests_|. > Done. > https://codereview.chromium.org/180843016/diff/120001/content/browser/loader/... > content/browser/loader/resource_scheduler.cc:296: //the priority and > SPDY-supporting may be changed, so judge again > Style nit: Capitalize and period, space after //. > > // The priority and SPDY support may have changed, so update the > // delayable count. > Done. > https://codereview.chromium.org/180843016/diff/120001/content/browser/loader/... > content/browser/loader/resource_scheduler.cc:331: if (in_flight_requests_.size() > == 0) { > Sorry, I missed this earlier. Is this just a special-case for if something goes > wrong with the accounting? We should getting the accounting correct in the first > place. > > Could you make this always use the normal codepath and add a DCHECK like: > DCHECK_LE(total_delayable_count_, in_flight_requests_.size()); > Done. This block is designed in case accounting is wrong. But you are right that DCHECK is better, in which we can find the wrong as soon as possible. > https://codereview.chromium.org/180843016/diff/120001/content/browser/loader/... > content/browser/loader/resource_scheduler.cc:359: ScheduledResourceRequest* > request, bool add) { > This function is a little bizarre. The invariant we actually want to maintain is > total_delayable_count_ == number of requests with > accounted_as_delayable_request bit true. > > So how about making this function instead be > void SetRequestDelayable(ScheduledResourceRequest* request, > bool delayable) { > if (request->accounted_as_delayable_request() == delayable) > return; > if (delayable) > total_delayable_count_++; > else > total_delayable_count_--; > request->set_accounted_as_delayable_request(delayable); > } > > Then ReprioritizeRequest and EraseInFlightRequest don't need to actually check > accounted_as_delayable_request, just call this with IsDelayableRequest(request) > and false, respectively. > Done. > https://codereview.chromium.org/180843016/diff/120001/content/browser/loader/... > content/browser/loader/resource_scheduler.cc:365: } > Style nit: newline. > Done. > https://codereview.chromium.org/180843016/diff/120001/content/browser/loader/... > content/browser/loader/resource_scheduler.cc:438: if > (num_delayable_requests_in_flight >= kMaxNumDelayableRequestsPerClient) { > Just realized there is one other behavior change relating to the SPDY thing: > > Say you spray 10 image loads to a SPDY-capable server that we haven't learned is > SPDY-capable. Then we learn it's SPDY-capable. Now you make another image load > to a non-SPDY capable server. Before, that image load would go through because > we'd re-evaluate delayability. With this CL we wouldn't. > > I think this is fine. The old logic I believe also didn't work if, say, the > non-SPDY-capable image load came before we learned the first server was > SPDY-capable. I just wanted to mention it. > I also realized the problem. As you said, it is hard to cover all cases. > https://codereview.chromium.org/180843016/diff/120001/content/browser/loader/... > content/browser/loader/resource_scheduler.cc:442: if > (ShouldKeepSearching(host_port_pair)) { > By the way, this CL doesn't avoid the O(N) iteration if your particular > benchmark were to spray a non-delayable request like an XHR or scripts or styles > rather than spray (I'm guessing) image loads. > > I suppose spraying images is conceivable for some image-heavy site. I don't know > how common an XHR spray would be. > > (If there were only delayable requests, then it would cut out the O(N) > iteration; either you have more than 10 delayable, or you have under 10 in which > case iterating over each isn't too bad.) > > Don't need to fix it for this CL since it's already kinda big, but it might be > something we could consider. > Yes. The CL mainly focus on image, delayable request. > https://codereview.chromium.org/180843016/diff/120001/content/browser/loader/... > File content/browser/loader/resource_scheduler_unittest.cc (right): > > https://codereview.chromium.org/180843016/diff/120001/content/browser/loader/... > content/browser/loader/resource_scheduler_unittest.cc:505: > TEST_F(ResourceSchedulerTest, newSpdyHostInDelayableRequests) { > Style nit: NewSpdyHostInDelayableRequest (capital N) > Done. > https://codereview.chromium.org/180843016/diff/120001/content/browser/loader/... > content/browser/loader/resource_scheduler_unittest.cc:509: > scoped_ptr<TestRequest> low1_spdy(NewRequest("http://spdyhost1:8080/low", > net::LOWEST)); > Style nit: Wrap this to 80 chars. > Done. > https://codereview.chromium.org/180843016/diff/120001/content/browser/loader/... > content/browser/loader/resource_scheduler_unittest.cc:509: > scoped_ptr<TestRequest> low1_spdy(NewRequest("http://spdyhost1:8080/low", > net::LOWEST)); > Maybe add a comment here so it's clear what's being tested: > > // Cancel a request after we learn the server supports SPDY. > Done. > https://codereview.chromium.org/180843016/diff/120001/content/browser/loader/... > content/browser/loader/resource_scheduler_unittest.cc:523: > scoped_ptr<TestRequest> low2_spdy(NewRequest("http://spdyhost2:8080/low", > net::IDLE)); > Style nit: Wrap this to 80 chars. > Done. > https://codereview.chromium.org/180843016/diff/120001/content/browser/loader/... > content/browser/loader/resource_scheduler_unittest.cc:523: > scoped_ptr<TestRequest> low2_spdy(NewRequest("http://spdyhost2:8080/low", > net::IDLE)); > Maybe add a comment here so it's clear what's being tested: > > // Reprioritize a request after we learn the server supports SPDY. Done.
LGTM. Thanks for bearing through the extra round of comments.
The CQ bit was checked by hong.zheng@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hong.zheng@intel.com/180843016/140001
Message was sent while issue was closed.
Change committed as 266165 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
