Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1230133005:
Fix Resource Priorities and Scheduling (Chrome Side) (Closed)
|
Created:
5 years, 5 months ago by Pat Meenan Modified:
5 years, 4 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis is a field trial for experimenting with several different resource scheduling changes (individually controllable):
- Increasing the priority of fonts
- Decreasing the priority of late-body scripts
- Increasing the priority of Async JS
- Reducing the contention during the initial load of head resources
Doc that describes the rationale behind all of the changes is here: https://docs.google.com/document/d/1bCDuq9H1ih9iNjgzyAL0gpwNFiEP4TZS-YLRp_RuMlc/edit?usp=sharing
Blink side of the change is here: https://codereview.chromium.org/1246493002/
BUG=317785, 517902, 517904
Committed: https://crrev.com/9ac66968b024f1d33427e4776bba672aab8ffa59
Cr-Commit-Position: refs/heads/master@{#343681}
Patch Set 1 #Patch Set 2 : Fixed categorization of delayable resources #Patch Set 3 : Updated Mode 2 to load lower priority resources in waves #Patch Set 4 : Fixed member init order #
Total comments: 2
Patch Set 5 : Cleanup #Patch Set 6 : Rebased #
Total comments: 6
Patch Set 7 : Individual toggles via finch trials/variations #Patch Set 8 : Cleaned up variation parsing #Patch Set 9 : Encoded the variations in the field trial group names #Patch Set 10 : Removed leftover debugging #Patch Set 11 : Moved blink options to use the field trial group name #Patch Set 12 : Fixed holdback of low priority resources #
Total comments: 38
Patch Set 13 : Initial changes from review #Patch Set 14 : Added unit test for blink settings #Patch Set 15 : Added unit tests #
Total comments: 4
Patch Set 16 : Updated comment #
Total comments: 57
Patch Set 17 : Changes from review feedback #
Total comments: 13
Patch Set 18 : Rebased #Patch Set 19 : Changes from review feedback #
Total comments: 4
Patch Set 20 : Switched the request classification to a bitfield of attributes #
Total comments: 14
Patch Set 21 : Fixed nits #Patch Set 22 : Fixed DCHECK attribute counting #Patch Set 23 : rebase #
Messages
Total messages: 51 (11 generated)
bmcquade@chromium.org changed reviewers: + bmcquade@chromium.org
https://codereview.chromium.org/1230133005/diff/60001/content/browser/loader/... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/1230133005/diff/60001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:323: std::string fetch_mode_cmd = Could we make ResourceScheduler constructor the canonical place for parsing the command line flag, and have ResourceScheduler pass the fetch mode in to this constructor when it creates a Client instance?
https://codereview.chromium.org/1230133005/diff/60001/content/browser/loader/... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/1230133005/diff/60001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:323: std::string fetch_mode_cmd = On 2015/07/22 17:46:48, Bryan McQuade wrote: > Could we make ResourceScheduler constructor the canonical place for parsing the > command line flag, and have ResourceScheduler pass the fetch mode in to this > constructor when it creates a Client instance? Done.
https://codereview.chromium.org/1230133005/diff/100001/content/browser/loader... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/1230133005/diff/100001/content/browser/loader... content/browser/loader/resource_scheduler.cc:823: in_flight_requests_.size() >= 2) { it's hard for me to say with certainty that this is the right thing for all pages. i'd want to be able to test just this change in isolation (same for previous comment) to really see if this is the right thing. could we separate this change out from the rest of your prioritization change and try it out as part of a separate patch? https://codereview.chromium.org/1230133005/diff/100001/content/browser/loader... content/browser/loader/resource_scheduler.cc:833: size_t higher_priority = 0; same as above (can we test this change separately from all of the other changes in a different patch)? https://codereview.chromium.org/1230133005/diff/100001/content/browser/loader... content/browser/loader/resource_scheduler.cc:1268: if (fetch_mode_ == 2 && old_priority_params.GreaterThan(new_priority_params)) are there cases where we demote priorities today? what are some examples and why do you want to disable them in fetch mode 2? I think I'd be more inclined to instead change the code that does the deprioritization to not do that instead, rather than ignoring the request to deprioritize silently here. That would likely be a surprise to anyone writing code and expecting a resource to get deprioritized.
https://codereview.chromium.org/1230133005/diff/100001/content/browser/loader... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/1230133005/diff/100001/content/browser/loader... content/browser/loader/resource_scheduler.cc:823: in_flight_requests_.size() >= 2) { On 2015/07/29 17:53:47, Bryan McQuade wrote: > it's hard for me to say with certainty that this is the right thing for all > pages. i'd want to be able to test just this change in isolation (same for > previous comment) to really see if this is the right thing. could we separate > this change out from the rest of your prioritization change and try it out as > part of a separate patch? This has all been ripped out and replaced with finch-controllable parameters. https://codereview.chromium.org/1230133005/diff/100001/content/browser/loader... content/browser/loader/resource_scheduler.cc:833: size_t higher_priority = 0; On 2015/07/29 17:53:47, Bryan McQuade wrote: > same as above (can we test this change separately from all of the other changes > in a different patch)? Also replaced and finch-controllable now. https://codereview.chromium.org/1230133005/diff/100001/content/browser/loader... content/browser/loader/resource_scheduler.cc:1268: if (fetch_mode_ == 2 && old_priority_params.GreaterThan(new_priority_params)) On 2015/07/29 17:53:47, Bryan McQuade wrote: > are there cases where we demote priorities today? what are some examples and why > do you want to disable them in fetch mode 2? I think I'd be more inclined to > instead change the code that does the deprioritization to not do that instead, > rather than ignoring the request to deprioritize silently here. That would > likely be a surprise to anyone writing code and expecting a resource to get > deprioritized. I'll debug this case separately and not make it part of this test. I saw it happen a few times in testing but there are supposed to be upstream protections to prevent it - like you said, better to go fix those. The case where it is supposed to be guarded against is when the same image is used in both visible and hidden parts of the page - to prevent the hidden version from lowering the priority of the visible one. That logic belongs on the blink side though.
pmeenan@chromium.org changed reviewers: + mmenke@chromium.org
mmenke@chromium.org: PTAL when you get a chance. This is a field trial experiment for several resource scheduling and prioritization changes we are experimenting with.
Not a full review https://codereview.chromium.org/1230133005/diff/220001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1230133005/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:1252: } What about the others? https://codereview.chromium.org/1230133005/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:1252: } Also, do we have any tests of this method? I'm paranoid. https://codereview.chromium.org/1230133005/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:1252: } I assume blink settings are generally tested at the blink layer, and not with integrationy-tests? https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... content/browser/loader/resource_scheduler.cc:15: #include "content/browser/loader/resource_scheduler.h" This should actually go at the top, before the <set> https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... content/browser/loader/resource_scheduler.cc:80: static const size_t kMaxNumThrottledRequestsPerClient = 1; +Default https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... content/browser/loader/resource_scheduler.cc:81: static const size_t kMaxNumDelayableWhileLayoutBlocking = 1; +Default https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... content/browser/loader/resource_scheduler.cc:82: static const net::RequestPriority kLayoutBlockingPriorityThreshold = net::LOW; +Default https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... content/browser/loader/resource_scheduler.cc:358: } Why are these per-client variables, and not in the scheduler itself? Also, not a big fan of having duplicate code to parse these. All this stuff also needs to be tested - both the implementation of the new rules, and the parsing of the field trial. https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... content/browser/loader/resource_scheduler.cc:678: return LAYOUT_BLOCKING_REQUEST; Add braces https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... content/browser/loader/resource_scheduler.cc:678: return LAYOUT_BLOCKING_REQUEST; Perhaps we should rename this? Either a request blocks layout, or it doesn't. Having a squishier meaning here seems like it calls for a squishier term. https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... content/browser/loader/resource_scheduler.cc:827: // Layout-blocking phase of resource loading Suggest comment above the if (I find it easier to parse, and matches the "High-priority and layout-blocking requests." comment above) https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... content/browser/loader/resource_scheduler.cc:827: // Layout-blocking phase of resource loading Add period (More consistent with other comments here, even though it's not a sentence, and makes it clearer that it's the end of the comment) https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... content/browser/loader/resource_scheduler.cc:828: size_t immediate_requests_in_flight_count = non_delayable_...? https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... content/browser/loader/resource_scheduler.cc:837: if (in_flight_requests_.size() > 0 && The other branch only does this if immediate_requests_in_flight_count > 0, while this branch does this if there's any request in flight. I don't suppose we could just apply that logic here, too, which would let us remove the else and merge the branches? This method is really a nightmare of hack on hack, would really like to prevent it from growing too much worse. I'm also not sure the difference between the two is at all significant. https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... content/browser/loader/resource_scheduler.cc:845: } else if (immediate_requests_in_flight_count > 0 && if "total_layout_blocking_count_" is greater than 0, isn't immediate_requests_in_flight_count *also* necessarily greater than 0? https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... content/browser/loader/resource_scheduler.cc:902: bool has_body_; Please add a description here, and maybe rename it to something more intuitive (HEAD responses, for example, have no bodies, but that's not what this is referring to) https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... content/browser/loader/resource_scheduler.cc:919: net::RequestPriority layout_blocking_priority_threshold_; These really need docs, and could use clearer names. "in_flight_layout_blocking_threshold_", for example, sounds like the number of in flight requests to block layout for.
Just one comment re: the question about tests. https://codereview.chromium.org/1230133005/diff/220001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1230133005/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:1252: } On 2015/08/07 16:55:10, mmenke wrote: > Also, do we have any tests of this method? I'm paranoid. There are some basic tests at https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... - I think we could benefit from adding some that test this new logic as well.
https://codereview.chromium.org/1230133005/diff/220001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1230133005/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:1243: if (split_group.size() == 5 && split_group[1].length() == 5) { you'll likely have Control,Default,and BlinkSettingsFlag groups, which won't match this pattern. Just worth adding a comment about this so future readers of the code know that you expect to get some group names that don't match the pattern, and it's correct to ignore them. https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... File content/browser/loader/resource_scheduler.cc (left): https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... content/browser/loader/resource_scheduler.cc:636: net::HostPortPair host_port_pair = why did the hostportpair logic get removed?
Still need to figure out how to test the variations on the scheduling logic in the resource scheduler but hopefully I addressed a lot of the other concerns/questions. The code is overly complicated right now because we have to maintain both code paths in addition to the finch-controllable settings. Presumably when we have the results of the field trial we can clean it up a lot (though a few of the numeric limits will stay as configurable so we can later do smarter tuning based on connection characteristics). https://codereview.chromium.org/1230133005/diff/220001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1230133005/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:1243: if (split_group.size() == 5 && split_group[1].length() == 5) { On 2015/08/07 18:02:26, Bryan McQuade wrote: > you'll likely have Control,Default,and BlinkSettingsFlag groups, which won't > match this pattern. Just worth adding a comment about this so future readers of > the code know that you expect to get some group names that don't match the > pattern, and it's correct to ignore them. Done. https://codereview.chromium.org/1230133005/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:1252: } On 2015/08/07 17:35:53, Bryan McQuade wrote: > On 2015/08/07 16:55:10, mmenke wrote: > > Also, do we have any tests of this method? I'm paranoid. > > There are some basic tests at > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... > - I think we could benefit from adding some that test this new logic as well. Done. https://codereview.chromium.org/1230133005/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:1252: } On 2015/08/07 16:55:10, mmenke wrote: > Also, do we have any tests of this method? I'm paranoid. Done. https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... File content/browser/loader/resource_scheduler.cc (left): https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... content/browser/loader/resource_scheduler.cc:636: net::HostPortPair host_port_pair = On 2015/08/07 18:02:26, Bryan McQuade wrote: > why did the hostportpair logic get removed? It was doing more harm than good. If the resource is from a H2/SPDY server then it will get issued long before any limit checks or counters are checked. If we're ONLY talking to a H2 server then all of the requests will get issued no matter what. If we are talking to a mix of servers, the request will still be issued to the H2 server but wouldn't have been accounted for or tracked as an in-flight delayable request so we'd allow an additional request on a H1 connection as well. Removing the check doesn't do anything in a H2 case and brings us a step closer to sanity in a mixed connection mode and behaves more like we'd like. https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... content/browser/loader/resource_scheduler.cc:15: #include "content/browser/loader/resource_scheduler.h" On 2015/08/07 16:55:10, mmenke wrote: > This should actually go at the top, before the <set> Done. https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... content/browser/loader/resource_scheduler.cc:80: static const size_t kMaxNumThrottledRequestsPerClient = 1; On 2015/08/07 16:55:10, mmenke wrote: > +Default Done. https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... content/browser/loader/resource_scheduler.cc:81: static const size_t kMaxNumDelayableWhileLayoutBlocking = 1; On 2015/08/07 16:55:11, mmenke wrote: > +Default Done. https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... content/browser/loader/resource_scheduler.cc:82: static const net::RequestPriority kLayoutBlockingPriorityThreshold = net::LOW; On 2015/08/07 16:55:11, mmenke wrote: > +Default Done. https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... content/browser/loader/resource_scheduler.cc:358: } On 2015/08/07 16:55:11, mmenke wrote: > Why are these per-client variables, and not in the scheduler itself? Also, not > a big fan of having duplicate code to parse these. > > All this stuff also needs to be tested - both the implementation of the new > rules, and the parsing of the field trial. Moved to the scheduler itself and pass into each client at construction. I was trying to keep the changes as small as possible with the hopes that it will all be ripped out once the field trial is done and a decision is made but it's not that much more to rip out the added code. As far as duplicate code for parsing, I could try to drop the actual code some place that both could reach though right now it's not 100% duplicate since they pull out different settings. https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... content/browser/loader/resource_scheduler.cc:678: return LAYOUT_BLOCKING_REQUEST; On 2015/08/07 16:55:11, mmenke wrote: > Add braces Done. https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... content/browser/loader/resource_scheduler.cc:678: return LAYOUT_BLOCKING_REQUEST; On 2015/08/07 16:55:11, mmenke wrote: > Perhaps we should rename this? Either a request blocks layout, or it doesn't. > Having a squishier meaning here seems like it calls for a squishier term. There are effectively 3 levels of groupings that we track. LAYOUT_BLOCKING_REQUEST (green columns on the doc) - This is basically CSS and is for tracking things that block layout even after we have made it to the body though head scripts could go in here if needed and wouldn't change the behavior. <Middle group> (yellow in the doc) - These are usually scripts and are resources that we allow to load while we are in "layout-blocking mode". This middle un-labled group comes into play below and is the difference between immediate and layout-blocking counts. DELAYABLE_REQUEST (red in the doc) - Images, Async scripts, etc. We limit the loading of these while in layout-blocking mode. I'm happy to change the names, I just don't have a good suggestion for what to call them. The LAYOUT_BLOCKING requests effectively keep us in layout-blocking mode beyond the point where has_body is flipped. https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... content/browser/loader/resource_scheduler.cc:827: // Layout-blocking phase of resource loading On 2015/08/07 16:55:11, mmenke wrote: > Suggest comment above the if (I find it easier to parse, and matches the > "High-priority and layout-blocking requests." comment above) Done. https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... content/browser/loader/resource_scheduler.cc:828: size_t immediate_requests_in_flight_count = On 2015/08/07 16:55:11, mmenke wrote: > non_delayable_...? Done. https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... content/browser/loader/resource_scheduler.cc:837: if (in_flight_requests_.size() > 0 && On 2015/08/07 16:55:11, mmenke wrote: > The other branch only does this if immediate_requests_in_flight_count > 0, while > this branch does this if there's any request in flight. I don't suppose we > could just apply that logic here, too, which would let us remove the else and > merge the branches? This method is really a nightmare of hack on hack, would > really like to prevent it from growing too much worse. > > I'm also not sure the difference between the two is at all significant. The difference between the two is pretty significant in that the other branch (the existing code basically) skips the logic entirely if the only thing in flight is delayable requests. That ends up causing a problem when head js does something like a document.write triggering more resource loads but where the non-delayable count could drop to zero while still in the head. The existing code would open the floodgates and allow for 10 delayable requests to get issued in that lull even though we are still in layout-blocking mode (I saw this happen on a few sites I was testing which is what made me change it in the new code). I REALLY want to rip the branching out and collapse the logic together but for the field trial to be fair I need to keep the old behavior intact. The new path looks really good in lab testing so as soon as the field trial concludes the hope is that we can rip out all of the conditional branching around the new settings and make it a lot cleaner. https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... content/browser/loader/resource_scheduler.cc:845: } else if (immediate_requests_in_flight_count > 0 && On 2015/08/07 16:55:11, mmenke wrote: > if "total_layout_blocking_count_" is greater than 0, isn't > immediate_requests_in_flight_count *also* necessarily greater than 0? Sadly, no. This is where the middle group of requests come into play (scripts basically) that are allowed to be requested while in layout-blocking mode but aren't categorized as layout-blocking themselves and they are not subjected to the same limits as the delayable requests. https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... content/browser/loader/resource_scheduler.cc:902: bool has_body_; On 2015/08/07 16:55:10, mmenke wrote: > Please add a description here, and maybe rename it to something more intuitive > (HEAD responses, for example, have no bodies, but that's not what this is > referring to) Done. https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... content/browser/loader/resource_scheduler.cc:919: net::RequestPriority layout_blocking_priority_threshold_; On 2015/08/07 16:55:10, mmenke wrote: > These really need docs, and could use clearer names. > > "in_flight_layout_blocking_threshold_", for example, sounds like the number of > in flight requests to block layout for. Added docs. Let me know if threshold still doesn't make sense to you.
Unrelated to this CL, but we have all this throttling and coalescing logic here that isn't currently in use. The field trial numbers didn't look like more than noise, when I skimmed them. Should decide at some point if we want to re-enable and keep that logic, or get rid of it entirely. I'll do a more thorough review of this next week. https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/1230133005/diff/220001/content/browser/loader... content/browser/loader/resource_scheduler.cc:845: } else if (immediate_requests_in_flight_count > 0 && On 2015/08/07 20:48:09, Pat Meenan wrote: > On 2015/08/07 16:55:11, mmenke wrote: > > if "total_layout_blocking_count_" is greater than 0, isn't > > immediate_requests_in_flight_count *also* necessarily greater than 0? > > Sadly, no. This is where the middle group of requests come into play (scripts > basically) that are allowed to be requested while in layout-blocking mode but > aren't categorized as layout-blocking themselves and they are not subjected to > the same limits as the delayable requests. Oh, I missed that this block is "!has_body_ || total_layout_blocking_count_ != 0" and not "!has_body_ && total_layout_blocking_count_ != 0".
I just added unit tests for the various permutations from the field trial code (which includes parsing the field trial spec). One change that I also made (earlier) was to the accounting of delayable requests. We were not counting in-flight H2 requests as delayable (against the total) so they were effectively "free" though still contending for bandwidth. In a H2-only case it won't matter since everything gets passed through but in a mixed case we'd end up with more delayable requests in flight than we expected. The change would hold back H1 delayable requests while H2 low-priority requests are in-flight. Technically it doesn't really have anything to do with the field trial logic and could be addressed separately if/when we take a look at fixing the scheduling of pages with mixed H1/H2 content so I'd be ok with putting that back but it does reduce the gains we see from reduction of contention in some cases (notably, any late-body or async js fretches from Google or Facebook on pages - like the Ajax API libraries or widget code).
On 2015/08/10 15:48:25, Pat Meenan wrote: > I just added unit tests for the various permutations from the field trial code > (which includes parsing the field trial spec). > > One change that I also made (earlier) was to the accounting of delayable > requests. We were not counting in-flight H2 requests as delayable (against the > total) so they were effectively "free" though still contending for bandwidth. > In a H2-only case it won't matter since everything gets passed through but in a > mixed case we'd end up with more delayable requests in flight than we expected. > > The change would hold back H1 delayable requests while H2 low-priority requests > are in-flight. > > Technically it doesn't really have anything to do with the field trial logic and > could be addressed separately if/when we take a look at fixing the scheduling of > pages with mixed H1/H2 content so I'd be ok with putting that back but it does > reduce the gains we see from reduction of contention in some cases (notably, any > late-body or async js fretches from Google or Facebook on pages - like the Ajax > API libraries or widget code). Sorry for not getting back to you on this today - I haven't forgotten about it.
Overall LGTM. https://codereview.chromium.org/1230133005/diff/280001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1230133005/diff/280001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:1262: // The field trial is configured to force users that specify the can you update this comment to start with "The field trials should be configured to" https://codereview.chromium.org/1230133005/diff/280001/content/browser/loader... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/1230133005/diff/280001/content/browser/loader... content/browser/loader/resource_scheduler.cc:806: in_flight_requests_.size() - in_flight_delayable_count_; on first glance this is potentially scary since, if in_flight_delayable_count_ is ever greater than in_flight_requests_.size(), the size_t wraps which could lead to unexpected behavior. i get the sense that it'd be a bug if this ever happened, but just to be more explicit about it, can we add CHECK(in_flight_requests_.size() >= in_flight_delayable_count_); right before this as a sanity check that this should never happen?
https://codereview.chromium.org/1230133005/diff/280001/content/browser/loader... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/1230133005/diff/280001/content/browser/loader... content/browser/loader/resource_scheduler.cc:806: in_flight_requests_.size() - in_flight_delayable_count_; On 2015/08/11 13:25:07, Bryan McQuade wrote: > on first glance this is potentially scary since, if in_flight_delayable_count_ > is ever greater than in_flight_requests_.size(), the size_t wraps which could > lead to unexpected behavior. i get the sense that it'd be a bug if this ever > happened, but just to be more explicit about it, can we add > CHECK(in_flight_requests_.size() >= in_flight_delayable_count_); right before > this as a sanity check that this should never happen? The only place we modify the classification of requests (SetRequestClassification) already has some sanity DCHECKs. I'm fine with adding more, if we want better DCHECKs. I don't think we want checks for sanity here - dchecks should be where we actually modify in_flight_delayable_count_ (And possibly in_flight_requests_.size()) make a lot more sense as sanity checks, since they give more useful information. If we just want a DCHECK for documentation purposes here (To say "this is true, we've thought of this), I am actually fine with that, just don't think it makes sense as a sanity check. Should not use CHECKs, unless there's some particular concern here, as they bloat up binary size.
https://codereview.chromium.org/1230133005/diff/280001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1230133005/diff/280001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:1262: // The field trial is configured to force users that specify the On 2015/08/11 13:25:07, Bryan McQuade wrote: > can you update this comment to start with "The field trials should be configured > to" Done.
https://codereview.chromium.org/1230133005/diff/300001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1230133005/diff/300001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:1253: blink_settings.push_back("fetchIncreasePriorities=true"); Is the code to implement these landed yet? https://codereview.chromium.org/1230133005/diff/300001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client_unittest.cc (right): https://codereview.chromium.org/1230133005/diff/300001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client_unittest.cc:155: static const char kGroupName[]; kFakeGroupName? https://codereview.chromium.org/1230133005/diff/300001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client_unittest.cc:161: static const char kPrioritiesGroupName[]; Everything below kDefaultGroupName just be inlined in the tests (Not sure about kDefaultGroupName - is group name "" by default or Default by default?). It's not at all clear that theye are all values for kResourcePrioritiesFieldTrialName, and separating them out from the tests makes maintenance more of a pain. https://codereview.chromium.org/1230133005/diff/300001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client_unittest.cc:244: TEST_F(BlinkSettingsFieldTrialTest, FieldTrialWithDefaultParams) { ResourcePrioritiesDefault? https://codereview.chromium.org/1230133005/diff/300001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client_unittest.cc:244: TEST_F(BlinkSettingsFieldTrialTest, FieldTrialWithDefaultParams) { This should probably go down with the other ResourcePriorities tests. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler.cc:5: #include "content/browser/loader/resource_scheduler.h" Add blank line after this include. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler.cc:315: size_t max_num_delayable_requests) I was actually thinking these could all be grabbed from the scheduler... i.e. scheduler_->max_num_delayable_while_layout_blocking(), since these don't vary on a per-client basis, and that pattern is already used elsewhere (e.g. scheduler_->limit_outstanding_requests()). https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler.cc:647: layout_blocking_priority_threshold_) { While we're here, is the second condition really needed? Seems like it could just be a DCHECK, if we really wanted to keep it. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler.cc:654: return LAYOUT_BLOCKING_REQUEST; On 2015/08/07 20:48:09, Pat Meenan wrote: > On 2015/08/07 16:55:11, mmenke wrote: > > Perhaps we should rename this? Either a request blocks layout, or it doesn't. > > > Having a squishier meaning here seems like it calls for a squishier term. > > There are effectively 3 levels of groupings that we track. > > LAYOUT_BLOCKING_REQUEST (green columns on the doc) - This is basically CSS and > is for tracking things that block layout even after we have made it to the body > though head scripts could go in here if needed and wouldn't change the behavior. > > <Middle group> (yellow in the doc) - These are usually scripts and are resources > that we allow to load while we are in "layout-blocking mode". This middle > un-labled group comes into play below and is the difference between immediate > and layout-blocking counts. > > DELAYABLE_REQUEST (red in the doc) - Images, Async scripts, etc. We limit the > loading of these while in layout-blocking mode. > > I'm happy to change the names, I just don't have a good suggestion for what to > call them. The LAYOUT_BLOCKING requests effectively keep us in layout-blocking > mode beyond the point where has_body is flipped. Having our enum names depend on deducing information on what blink is doing based on a deep understanding of blink's prioritization mapping seems like a huge layering violation. I can't really validate the code is correct without that understanding. If blink sent us a layout blocking bit, and we just used that, I'd feel differently. EARLY_IMMEDIATE_REQUESTS? EARLY_NON_DELAYABLE_REQUESTS? Both go well with "delayable", and make for a clearer difference between non-EARLY_REQUESTS (A classification this doesn't actually return, but we do implicitly have, and are currently called high priority requests, which is a bit bad, because there is a HIGH priority, but we classify MEDIUM and even LOW priority requests as high-priority requests). https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler.cc:660: return IN_FLIGHT_DELAYABLE_REQUEST; I don't think the SPDY change belongs in this CL. Fine with landing it first (should be fast) or second. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler.cc:765: in_flight_requests_.size() >= scheduler_->outstanding_request_limit()) { Note that you may run into this experiment, which also sets limit_outstanding_requests() to true. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler.cc:789: if (url_request.priority() >= layout_blocking_priority_threshold_) { This is kinda weird...We're looking at the layout blocking threshold, but url_request may not be layout blocking, based on when it was started. Should it be immediate_request_threshold_ / non_delayable_request_threshold_ - see earlier comment. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler.cc:815: // Allow the request if nothing is in flight or if the limit of This should probably say "block the request if..." https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler.cc:816: // concurrent lower priority requests has not been hit. This should either before the if, or inside the if's body - it describes the line before it as well as next two conditions. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler.cc:827: // active). Again, this should either before the if, or inside the if's body.
https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler.cc:819: max_num_delayable_while_layout_blocking_)) { max_num_delayable_while_layout_blocking_ is used with >=, while in_flight_layout_blocking_threshold_ is used with >. This seems inconsistent. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler.cc:959: // D - fetchIncreasePriorities (1 for true, 0 for false) I suggest an underscore between D and E, to separate out those that affect blink from those that affect the scheduler. Could even use different delimiters, though I suppose just using one is simpler. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... File content/browser/loader/resource_scheduler_unittest.cc (right): https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler_unittest.cc:2369: base::StringPrintf("ResourcePriorities/LayoutBlocking_000%d0_0_1_10/", Why the terminal slash on group names here? The tests you added to the other file don't have them. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler_unittest.cc:2400: const int kEnablePriorityIncrease = 1; Suggest giving the value of all relevant variables: in_flight_layout_blocking_threshold_, max_num_delayable_while_layout_blocking_, etc. Can either make a method that takes them all and returns a string, like: std::string trial = CreateResourcePrioritiesTrial( 0, // goats_teleported ... ); Or just have comments with them all. Or make constants for them all. Just want to be able to fairly easily verify them, and want anyone looking at this in the future to be able to do so as well (Admittedly, these will hopefully be gone in 3 months, once we decide on the best behaviors, but in the mean time...). https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler_unittest.cc:2438: // Prevents anly low priority requests from starting while more than anly -> any https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler_unittest.cc:2456: high2.reset(); EXPECT_FALSE(low2->started()); .... At least I think max_num_delayable_while_layout_blocking_ is 1 here? https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler_unittest.cc:2462: // Prevents anly low priority requests from starting while more than any https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler_unittest.cc:2483: high3.reset(); Again, should have EXPECT here for low2. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler_unittest.cc:2488: TEST_F(ResourceSchedulerTest, IncreaseLowLoadsUntilBodyInserted) { I think TwoDelayableLoadsUntilBodyInserted would be more consistent with your other test names. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler_unittest.cc:2492: kMaxNumDelayableWhileLayoutBlocking))); Any reason why the last 3 have no text describing the configuration? https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler_unittest.cc:2507: TEST_F(ResourceSchedulerTest, IncreaseLowLoadsUntilBodyInsertedLayoutBlocking) { UseLayoutBlockingThresholdOneAndTwoDelayableLoadsUntilBodyInserted? https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler_unittest.cc:2538: kMaxNumDelayableRequestsPerClient))); Should we have another simple test that checks the MaxNumDelayableRequestsPerClient when the other stuff is enabled? i.e. 00001_...? https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler_unittest.cc:2541: // We only load low priority resources if there's a body. nit: Avoid using "we" in comments.
https://codereview.chromium.org/1230133005/diff/300001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1230133005/diff/300001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:1253: blink_settings.push_back("fetchIncreasePriorities=true"); On 2015/08/11 17:57:23, mmenke wrote: > Is the code to implement these landed yet? Not yet. Currently under review: https://codereview.chromium.org/1246493002/ I'll address all of the other issues and let you know when the blink side has landed in case the names change. https://codereview.chromium.org/1230133005/diff/300001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client_unittest.cc (right): https://codereview.chromium.org/1230133005/diff/300001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client_unittest.cc:155: static const char kGroupName[]; On 2015/08/11 17:57:23, mmenke wrote: > kFakeGroupName? Done. https://codereview.chromium.org/1230133005/diff/300001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client_unittest.cc:161: static const char kPrioritiesGroupName[]; On 2015/08/11 17:57:23, mmenke wrote: > Everything below kDefaultGroupName just be inlined in the tests (Not sure about > kDefaultGroupName - is group name "" by default or Default by default?). It's > not at all clear that theye are all values for > kResourcePrioritiesFieldTrialName, and separating them out from the tests makes > maintenance more of a pain. Done. https://codereview.chromium.org/1230133005/diff/300001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client_unittest.cc:244: TEST_F(BlinkSettingsFieldTrialTest, FieldTrialWithDefaultParams) { On 2015/08/11 17:57:23, mmenke wrote: > ResourcePrioritiesDefault? Done. https://codereview.chromium.org/1230133005/diff/300001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client_unittest.cc:244: TEST_F(BlinkSettingsFieldTrialTest, FieldTrialWithDefaultParams) { On 2015/08/11 17:57:23, mmenke wrote: > This should probably go down with the other ResourcePriorities tests. Done. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler.cc:5: #include "content/browser/loader/resource_scheduler.h" On 2015/08/11 17:57:23, mmenke wrote: > Add blank line after this include. Done. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler.cc:315: size_t max_num_delayable_requests) On 2015/08/11 17:57:23, mmenke wrote: > I was actually thinking these could all be grabbed from the scheduler... i.e. > scheduler_->max_num_delayable_while_layout_blocking(), since these don't vary on > a per-client basis, and that pattern is already used elsewhere (e.g. > scheduler_->limit_outstanding_requests()). Done. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler.cc:647: layout_blocking_priority_threshold_) { On 2015/08/11 17:57:23, mmenke wrote: > While we're here, is the second condition really needed? Seems like it could > just be a DCHECK, if we really wanted to keep it. Done. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler.cc:654: return LAYOUT_BLOCKING_REQUEST; On 2015/08/11 17:57:23, mmenke wrote: > On 2015/08/07 20:48:09, Pat Meenan wrote: > > On 2015/08/07 16:55:11, mmenke wrote: > > > Perhaps we should rename this? Either a request blocks layout, or it > doesn't. > > > > > Having a squishier meaning here seems like it calls for a squishier term. > > > > There are effectively 3 levels of groupings that we track. > > > > LAYOUT_BLOCKING_REQUEST (green columns on the doc) - This is basically CSS and > > is for tracking things that block layout even after we have made it to the > body > > though head scripts could go in here if needed and wouldn't change the > behavior. > > > > <Middle group> (yellow in the doc) - These are usually scripts and are > resources > > that we allow to load while we are in "layout-blocking mode". This middle > > un-labled group comes into play below and is the difference between immediate > > and layout-blocking counts. > > > > DELAYABLE_REQUEST (red in the doc) - Images, Async scripts, etc. We limit the > > loading of these while in layout-blocking mode. > > > > I'm happy to change the names, I just don't have a good suggestion for what to > > call them. The LAYOUT_BLOCKING requests effectively keep us in > layout-blocking > > mode beyond the point where has_body is flipped. > > Having our enum names depend on deducing information on what blink is doing > based on a deep understanding of blink's prioritization mapping seems like a > huge layering violation. I can't really validate the code is correct without > that understanding. If blink sent us a layout blocking bit, and we just used > that, I'd feel differently. > > EARLY_IMMEDIATE_REQUESTS? EARLY_NON_DELAYABLE_REQUESTS? Both go well with > "delayable", and make for a clearer difference between non-EARLY_REQUESTS (A > classification this doesn't actually return, but we do implicitly have, and are > currently called high priority requests, which is a bit bad, because there is a > HIGH priority, but we classify MEDIUM and even LOW priority requests as > high-priority requests). It's a bit more complicated because there's also special logic associated with this enum. We stay in "layout-blocking" loading mode until we both got the HTML body and all of the layout-blocking requests have finished. It's also not ALL non-delayable requests, just those that are over the threshold. The ones that == the threshold are neither layout-blocking nor delayable. These requests keep us in the holdback mode so I'd be fine renaming it to something clearer but for better or worse we're currently referring that to layout-blocking mode because we're loading resources that are preventing the first page layout. An explicit bit from blink should be easy enough but I think we're always going to have a piece somewhere between blink and the raw net stack that understands both worlds and does prioritization that is content-specific, tab-aware (visible/hidden) and in the not-too-distant future iframe-aware. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler.cc:660: return IN_FLIGHT_DELAYABLE_REQUEST; On 2015/08/11 17:57:23, mmenke wrote: > I don't think the SPDY change belongs in this CL. Fine with landing it first > (should be fast) or second. Agreed. We'll address this separately because there's a bigger issue of SPDY request prioritization when a page spans multiple hosts (or mixes SPDY/HTTP1). https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler.cc:765: in_flight_requests_.size() >= scheduler_->outstanding_request_limit()) { On 2015/08/11 17:57:23, mmenke wrote: > Note that you may run into this experiment, which also sets > limit_outstanding_requests() to true. Acknowledged. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler.cc:789: if (url_request.priority() >= layout_blocking_priority_threshold_) { On 2015/08/11 17:57:23, mmenke wrote: > This is kinda weird...We're looking at the layout blocking threshold, but > url_request may not be layout blocking, based on when it was started. Should it > be immediate_request_threshold_ / non_delayable_request_threshold_ - see earlier > comment. I can rename this if we come to a decision on what we want to call the line that we're using to determine what we currently classify as layout-blocking. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler.cc:815: // Allow the request if nothing is in flight or if the limit of On 2015/08/11 17:57:23, mmenke wrote: > This should probably say "block the request if..." Done. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler.cc:816: // concurrent lower priority requests has not been hit. On 2015/08/11 17:57:23, mmenke wrote: > This should either before the if, or inside the if's body - it describes the > line before it as well as next two conditions. Done. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler.cc:819: max_num_delayable_while_layout_blocking_)) { On 2015/08/11 19:27:28, mmenke wrote: > max_num_delayable_while_layout_blocking_ is used with >=, while > in_flight_layout_blocking_threshold_ is used with >. This seems inconsistent. They have slightly different behaviors which is why the names are different (max vs threshold). Max is a saturation point and you'd never expect to exceed it. In the case of the threshold, once the number of in-flight immediate requests drops down to (or below) that value we start allowing lower priority requests through. In this case the request being checked is delayable and directly impacts the max_num_delayable result while the threshold is checking the state of other request priorities before determining if it should let this one through. I might be able to switch the "threshold" to a "min_num_immediate_to_allow_delayable" or something like that to make it clearer but it would still be the same comparison. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler.cc:827: // active). On 2015/08/11 17:57:23, mmenke wrote: > Again, this should either before the if, or inside the if's body. Done. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler.cc:959: // D - fetchIncreasePriorities (1 for true, 0 for false) On 2015/08/11 19:27:28, mmenke wrote: > I suggest an underscore between D and E, to separate out those that affect blink > from those that affect the scheduler. Could even use different delimiters, > though I suppose just using one is simpler. Though D is also a special case because it affects both. I basically set it up so that the first block was all of the bit fields that turn things on and off regardless of where the implementation had to be and the trailing values after the underscores were for the settings that had explicit numeric values. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... File content/browser/loader/resource_scheduler_unittest.cc (right): https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler_unittest.cc:2369: base::StringPrintf("ResourcePriorities/LayoutBlocking_000%d0_0_1_10/", On 2015/08/11 19:27:28, mmenke wrote: > Why the terminal slash on group names here? The tests you added to the other > file don't have them. The other file didn't use the command-line parsing code path for the field trials, it created them directly and matched the existing checks in the file (the field trial name and group name were not parsed, they were passed explicitly). The existing field trial checks for the token limits in here used the command-line parsing path for field trials which uses slashes to separate the field trial and group name (and terminates in a slash) and I used the same path for consistency. I can switch all of the field trial code in here to use the explicit field trial creation if you'd prefer. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler_unittest.cc:2400: const int kEnablePriorityIncrease = 1; On 2015/08/11 19:27:28, mmenke wrote: > Suggest giving the value of all relevant variables: > in_flight_layout_blocking_threshold_, max_num_delayable_while_layout_blocking_, > etc. Can either make a method that takes them all and returns a string, like: > > std::string trial = CreateResourcePrioritiesTrial( > 0, // goats_teleported > ... > ); > > Or just have comments with them all. > > Or make constants for them all. Just want to be able to fairly easily verify > them, and want anyone looking at this in the future to be able to do so as well > (Admittedly, these will hopefully be gone in 3 months, once we decide on the > best behaviors, but in the mean time...). Done. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler_unittest.cc:2438: // Prevents anly low priority requests from starting while more than On 2015/08/11 19:27:28, mmenke wrote: > anly -> any Done. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler_unittest.cc:2456: high2.reset(); On 2015/08/11 19:27:28, mmenke wrote: > EXPECT_FALSE(low2->started()); .... At least I think > max_num_delayable_while_layout_blocking_ is 1 here? Done. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler_unittest.cc:2462: // Prevents anly low priority requests from starting while more than On 2015/08/11 19:27:28, mmenke wrote: > any Done. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler_unittest.cc:2483: high3.reset(); On 2015/08/11 19:27:28, mmenke wrote: > Again, should have EXPECT here for low2. Done. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler_unittest.cc:2488: TEST_F(ResourceSchedulerTest, IncreaseLowLoadsUntilBodyInserted) { On 2015/08/11 19:27:28, mmenke wrote: > I think TwoDelayableLoadsUntilBodyInserted would be more consistent with your > other test names. Done. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler_unittest.cc:2492: kMaxNumDelayableWhileLayoutBlocking))); On 2015/08/11 19:27:28, mmenke wrote: > Any reason why the last 3 have no text describing the configuration? No good reason - just added a comment explaining the test conditions for the ones that were missing them. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler_unittest.cc:2507: TEST_F(ResourceSchedulerTest, IncreaseLowLoadsUntilBodyInsertedLayoutBlocking) { On 2015/08/11 19:27:28, mmenke wrote: > UseLayoutBlockingThresholdOneAndTwoDelayableLoadsUntilBodyInserted? Done. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler_unittest.cc:2538: kMaxNumDelayableRequestsPerClient))); On 2015/08/11 19:27:28, mmenke wrote: > Should we have another simple test that checks the > MaxNumDelayableRequestsPerClient when the other stuff is enabled? i.e. > 00001_...? Done. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler_unittest.cc:2541: // We only load low priority resources if there's a body. On 2015/08/11 19:27:28, mmenke wrote: > nit: Avoid using "we" in comments. Done.
Quick response. Should have time for another pass today. really shouldn't land this until the blink side change lands. https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/1230133005/diff/300001/content/browser/loader... content/browser/loader/resource_scheduler.cc:654: return LAYOUT_BLOCKING_REQUEST; On 2015/08/12 16:20:59, Pat Meenan wrote: > On 2015/08/11 17:57:23, mmenke wrote: > > On 2015/08/07 20:48:09, Pat Meenan wrote: > > > On 2015/08/07 16:55:11, mmenke wrote: > > > > Perhaps we should rename this? Either a request blocks layout, or it > > doesn't. > > > > > > > Having a squishier meaning here seems like it calls for a squishier term. > > > > > > There are effectively 3 levels of groupings that we track. > > > > > > LAYOUT_BLOCKING_REQUEST (green columns on the doc) - This is basically CSS > and > > > is for tracking things that block layout even after we have made it to the > > body > > > though head scripts could go in here if needed and wouldn't change the > > behavior. > > > > > > <Middle group> (yellow in the doc) - These are usually scripts and are > > resources > > > that we allow to load while we are in "layout-blocking mode". This middle > > > un-labled group comes into play below and is the difference between > immediate > > > and layout-blocking counts. > > > > > > DELAYABLE_REQUEST (red in the doc) - Images, Async scripts, etc. We limit > the > > > loading of these while in layout-blocking mode. > > > > > > I'm happy to change the names, I just don't have a good suggestion for what > to > > > call them. The LAYOUT_BLOCKING requests effectively keep us in > > layout-blocking > > > mode beyond the point where has_body is flipped. > > > > Having our enum names depend on deducing information on what blink is doing > > based on a deep understanding of blink's prioritization mapping seems like a > > huge layering violation. I can't really validate the code is correct without > > that understanding. If blink sent us a layout blocking bit, and we just used > > that, I'd feel differently. > > > > EARLY_IMMEDIATE_REQUESTS? EARLY_NON_DELAYABLE_REQUESTS? Both go well with > > "delayable", and make for a clearer difference between non-EARLY_REQUESTS (A > > classification this doesn't actually return, but we do implicitly have, and > are > > currently called high priority requests, which is a bit bad, because there is > a > > HIGH priority, but we classify MEDIUM and even LOW priority requests as > > high-priority requests). > > It's a bit more complicated because there's also special logic associated with > this enum. We stay in "layout-blocking" loading mode until we both got the HTML > body and all of the layout-blocking requests have finished. It's also not ALL > non-delayable requests, just those that are over the threshold. The ones that > == the threshold are neither layout-blocking nor delayable. > > These requests keep us in the holdback mode so I'd be fine renaming it to > something clearer but for better or worse we're currently referring that to > layout-blocking mode because we're loading resources that are preventing the > first page layout. An explicit bit from blink should be easy enough but I think > we're always going to have a piece somewhere between blink and the raw net stack > that understands both worlds and does prioritization that is content-specific, > tab-aware (visible/hidden) and in the not-too-distant future iframe-aware. Talking on another thread about getting this information from the renderer, which would make me much happier... Let's leave it alone for now.
https://codereview.chromium.org/1230133005/diff/320001/content/browser/loader... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/1230133005/diff/320001/content/browser/loader... content/browser/loader/resource_scheduler.cc:635: } nit: Remove braces, to be more consistent with the rest of this file https://codereview.chromium.org/1230133005/diff/320001/content/browser/loader... content/browser/loader/resource_scheduler.cc:652: } Can we add two more classifications here? DELAYABLE_REQUEST and HIGH_PRIORITY_REQUEST? Then we can just look at the classifications in ShouldStartRequest. I keep on getting confused about what a HIGH_PRIORITY request is (Didn't realize it included requests with a priority of the layout_blocking_priority_threshold(), and not above it). I think having all the classifications explicitly here would make the code much clearer. https://codereview.chromium.org/1230133005/diff/320001/content/browser/loader... content/browser/loader/resource_scheduler.cc:808: if (in_flight_requests_.size() > 0 && I think we can remove this line - if in_flight_requests_.size() is 0, then total_layout_blocking_count_ will also be 0, so we won't reach this point. Or should we reconsider the conditions of the first if? It seems kinda weird that if we have a layout blocking request, high priority requests count against low priority requests, but if we don't, they don't. https://codereview.chromium.org/1230133005/diff/320001/content/browser/loader... File content/browser/loader/resource_scheduler.h (right): https://codereview.chromium.org/1230133005/diff/320001/content/browser/loader... content/browser/loader/resource_scheduler.h:169: // layout-blocking. Suggesting adding something like "or high priority, if the end of the body has already been received." https://codereview.chromium.org/1230133005/diff/320001/content/browser/loader... content/browser/loader/resource_scheduler.h:170: net::RequestPriority layout_blocking_priority_threshold() const { layout_blocking_or_high_priority_threshold (And same for all of these)? https://codereview.chromium.org/1230133005/diff/320001/content/browser/loader... File content/browser/loader/resource_scheduler_unittest.cc (right): https://codereview.chromium.org/1230133005/diff/320001/content/browser/loader... content/browser/loader/resource_scheduler_unittest.cc:2689: ScopedVector<TestRequest> lows_differenthosts; nit: *_different_host. Goes for all of these.
https://codereview.chromium.org/1230133005/diff/320001/content/browser/loader... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/1230133005/diff/320001/content/browser/loader... content/browser/loader/resource_scheduler.cc:808: if (in_flight_requests_.size() > 0 && On 2015/08/12 17:30:08, mmenke wrote: > I think we can remove this line - if in_flight_requests_.size() is 0, then > total_layout_blocking_count_ will also be 0, so we won't reach this point. Or > should we reconsider the conditions of the first if? It seems kinda weird that > if we have a layout blocking request, high priority requests count against low > priority requests, but if we don't, they don't. Oh, right...the no body yet case. Think I forgot about that before.
FYI, the blink-side landed last night. Only thing I wasn't 100% clear on was the change to the enum classification (below). https://codereview.chromium.org/1230133005/diff/320001/content/browser/loader... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/1230133005/diff/320001/content/browser/loader... content/browser/loader/resource_scheduler.cc:635: } On 2015/08/12 17:30:08, mmenke wrote: > nit: Remove braces, to be more consistent with the rest of this file Done. https://codereview.chromium.org/1230133005/diff/320001/content/browser/loader... content/browser/loader/resource_scheduler.cc:652: } On 2015/08/12 17:30:08, mmenke wrote: > Can we add two more classifications here? > > DELAYABLE_REQUEST and HIGH_PRIORITY_REQUEST? Then we can just look at the > classifications in ShouldStartRequest. > > I keep on getting confused about what a HIGH_PRIORITY request is (Didn't realize > it included requests with a priority of the > layout_blocking_priority_threshold(), and not above it). I think having all the > classifications explicitly here would make the code much clearer. Would you want them broken out into two flags (in-flight or not and the categorization)? Otherwise we'll have both DELAYABLE and IN_FLIGHT_DELAYABLE but not IN_FLIGHT versions of the others. It's also possible to go ahead and create IN_FLIGHT versions of all of them since ShouldStartRequest will never be checking an in-flight request. https://codereview.chromium.org/1230133005/diff/320001/content/browser/loader... content/browser/loader/resource_scheduler.cc:808: if (in_flight_requests_.size() > 0 && On 2015/08/12 17:33:59, mmenke wrote: > On 2015/08/12 17:30:08, mmenke wrote: > > I think we can remove this line - if in_flight_requests_.size() is 0, then > > total_layout_blocking_count_ will also be 0, so we won't reach this point. Or > > should we reconsider the conditions of the first if? It seems kinda weird > that > > if we have a layout blocking request, high priority requests count against low > > priority requests, but if we don't, they don't. > > Oh, right...the no body yet case. Think I forgot about that before. Not sure if it helps any but I beefed up the comment for this chunk of logic to hopefully be clearer. It might also help if I factor out the check for the layout-blocking loading pahse into a separate function to make it clearer and bury the actual logic (though this is the only place it has to make that check). https://codereview.chromium.org/1230133005/diff/320001/content/browser/loader... File content/browser/loader/resource_scheduler.h (right): https://codereview.chromium.org/1230133005/diff/320001/content/browser/loader... content/browser/loader/resource_scheduler.h:169: // layout-blocking. On 2015/08/12 17:30:08, mmenke wrote: > Suggesting adding something like "or high priority, if the end of the body has > already been received." Done. https://codereview.chromium.org/1230133005/diff/320001/content/browser/loader... content/browser/loader/resource_scheduler.h:170: net::RequestPriority layout_blocking_priority_threshold() const { On 2015/08/12 17:30:08, mmenke wrote: > layout_blocking_or_high_priority_threshold (And same for all of these)? Makes sense for this one but the others are really JUST layout-blocking (though enable_layout_blocking_threshold should be enable_in_flight_layout_blocking_threshold) https://codereview.chromium.org/1230133005/diff/320001/content/browser/loader... File content/browser/loader/resource_scheduler_unittest.cc (right): https://codereview.chromium.org/1230133005/diff/320001/content/browser/loader... content/browser/loader/resource_scheduler_unittest.cc:2689: ScopedVector<TestRequest> lows_differenthosts; On 2015/08/12 17:30:09, mmenke wrote: > nit: *_different_host. Goes for all of these. Done.
https://codereview.chromium.org/1230133005/diff/360001/content/browser/loader... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/1230133005/diff/360001/content/browser/loader... content/browser/loader/resource_scheduler.cc:651: } On 2015/08/13 12:11:03, Pat Meenan wrote: > On 2015/08/12 17:30:08, mmenke wrote: > > Can we add two more classifications here? > > > > DELAYABLE_REQUEST and HIGH_PRIORITY_REQUEST? Then we can just look at the > > classifications in ShouldStartRequest. > > > > I keep on getting confused about what a HIGH_PRIORITY request is (Didn't > realize > > it included requests with a priority of the > > layout_blocking_priority_threshold(), and not above it). I think having all > the > > classifications explicitly here would make the code much clearer. > > Would you want them broken out into two flags (in-flight or not and the > categorization)? Otherwise we'll have both DELAYABLE and IN_FLIGHT_DELAYABLE > but not IN_FLIGHT versions of the others. It's also possible to go ahead and > create IN_FLIGHT versions of all of them since ShouldStartRequest will never be > checking an in-flight request. I'm fine with any of the possible approaches here, actually. That having been said, I think either making it a combination of 3 flags (DELAYABLE, LAYOUT_BLOCKING, IN_FLIGHT) or removing the IN_FLIGHT state from the classification would be the cleanest - removing the IN_FLIGHT state would require a lot more refactoring, though. https://codereview.chromium.org/1230133005/diff/360001/content/browser/loader... File content/browser/loader/resource_scheduler.h (right): https://codereview.chromium.org/1230133005/diff/360001/content/browser/loader... content/browser/loader/resource_scheduler.h:172: // layout-blocking request). On 2015/08/13 12:11:03, Pat Meenan wrote: > On 2015/08/12 17:30:08, mmenke wrote: > > layout_blocking_or_high_priority_threshold (And same for all of these)? > > Makes sense for this one but the others are really JUST layout-blocking (though > enable_layout_blocking_threshold should be > enable_in_flight_layout_blocking_threshold) That's actually not true - "in_flight_layout_blocking_threshold" is compared to "non_delayable_requests_in_flight_count" rather than "total_layout_blocking_count_".
Sorry I've been somewhat slow on this review - had a number of reviews that require some thought this week.
https://codereview.chromium.org/1230133005/diff/360001/content/browser/loader... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/1230133005/diff/360001/content/browser/loader... content/browser/loader/resource_scheduler.cc:651: } On 2015/08/13 15:16:07, mmenke wrote: > On 2015/08/13 12:11:03, Pat Meenan wrote: > > On 2015/08/12 17:30:08, mmenke wrote: > > > Can we add two more classifications here? > > > > > > DELAYABLE_REQUEST and HIGH_PRIORITY_REQUEST? Then we can just look at the > > > classifications in ShouldStartRequest. > > > > > > I keep on getting confused about what a HIGH_PRIORITY request is (Didn't > > realize > > > it included requests with a priority of the > > > layout_blocking_priority_threshold(), and not above it). I think having all > > the > > > classifications explicitly here would make the code much clearer. > > > > Would you want them broken out into two flags (in-flight or not and the > > categorization)? Otherwise we'll have both DELAYABLE and IN_FLIGHT_DELAYABLE > > but not IN_FLIGHT versions of the others. It's also possible to go ahead and > > create IN_FLIGHT versions of all of them since ShouldStartRequest will never > be > > checking an in-flight request. > > I'm fine with any of the possible approaches here, actually. > > That having been said, I think either making it a combination of 3 flags > (DELAYABLE, LAYOUT_BLOCKING, IN_FLIGHT) or removing the IN_FLIGHT state from the > classification would be the cleanest - removing the IN_FLIGHT state would > require a lot more refactoring, though. Ok, it resulted in a bunch of changes but I think it's a lot cleaner now. I switched the classification to be a bitfield of "attributes" (layout-blocking, in-flight, delayable) and then those are what are used for scheduling decisions rather than comparing the priority. Still not 100% in love with calling them attributes so if you have a better name I'd be happy to change it. https://codereview.chromium.org/1230133005/diff/360001/content/browser/loader... File content/browser/loader/resource_scheduler.h (right): https://codereview.chromium.org/1230133005/diff/360001/content/browser/loader... content/browser/loader/resource_scheduler.h:172: // layout-blocking request). On 2015/08/13 15:16:07, mmenke wrote: > On 2015/08/13 12:11:03, Pat Meenan wrote: > > On 2015/08/12 17:30:08, mmenke wrote: > > > layout_blocking_or_high_priority_threshold (And same for all of these)? > > > > Makes sense for this one but the others are really JUST layout-blocking > (though > > enable_layout_blocking_threshold should be > > enable_in_flight_layout_blocking_threshold) > > That's actually not true - "in_flight_layout_blocking_threshold" is compared to > "non_delayable_requests_in_flight_count" rather than > "total_layout_blocking_count_". Thanks - managed to confuse myself apparently. Changed it to in_flight_non_delayable because enable_in_flight_layout_blocking_or_high_priority_threshold was just about impossible to fit in 80 characters and it makes the comparison cleaner.
LGTM, thanks for bearing with me! https://codereview.chromium.org/1230133005/diff/380001/content/browser/loader... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/1230133005/diff/380001/content/browser/loader... content/browser/loader/resource_scheduler.cc:44: typedef uint8_t RequestAttributes; nit: include <stdint.h> https://codereview.chromium.org/1230133005/diff/380001/content/browser/loader... content/browser/loader/resource_scheduler.cc:44: typedef uint8_t RequestAttributes; I think "using RequestAttributes = uint8_t;" is now preferred https://codereview.chromium.org/1230133005/diff/380001/content/browser/loader... content/browser/loader/resource_scheduler.cc:625: kAttributeInFlight | kAttributeDelayable)) { nit: +1 indent https://codereview.chromium.org/1230133005/diff/380001/content/browser/loader... content/browser/loader/resource_scheduler.cc:632: kAttributeInFlight | kAttributeDelayable)) { nit: +1 indent https://codereview.chromium.org/1230133005/diff/380001/content/browser/loader... content/browser/loader/resource_scheduler.cc:651: attributes = attributes | kAttributeInFlight; Any reason not to use |= on all of these? https://codereview.chromium.org/1230133005/diff/380001/content/browser/loader... content/browser/loader/resource_scheduler.cc:803: // non-delayable requests. Nit: "Non-delayable requests." (capitalize) https://codereview.chromium.org/1230133005/diff/380001/content/browser/loader... content/browser/loader/resource_scheduler.cc:806: } nit: Remove braces
Thanks for taking the time to really understand what is going on and push on the confusing bits. I think we ended up in a much better place than things were before (clearly with cleanup still to do). https://codereview.chromium.org/1230133005/diff/380001/content/browser/loader... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/1230133005/diff/380001/content/browser/loader... content/browser/loader/resource_scheduler.cc:44: typedef uint8_t RequestAttributes; On 2015/08/13 21:27:44, mmenke wrote: > nit: include <stdint.h> Done. https://codereview.chromium.org/1230133005/diff/380001/content/browser/loader... content/browser/loader/resource_scheduler.cc:44: typedef uint8_t RequestAttributes; On 2015/08/13 21:27:45, mmenke wrote: > I think "using RequestAttributes = uint8_t;" is now preferred Done. https://codereview.chromium.org/1230133005/diff/380001/content/browser/loader... content/browser/loader/resource_scheduler.cc:625: kAttributeInFlight | kAttributeDelayable)) { On 2015/08/13 21:27:45, mmenke wrote: > nit: +1 indent Done. https://codereview.chromium.org/1230133005/diff/380001/content/browser/loader... content/browser/loader/resource_scheduler.cc:632: kAttributeInFlight | kAttributeDelayable)) { On 2015/08/13 21:27:45, mmenke wrote: > nit: +1 indent Done. https://codereview.chromium.org/1230133005/diff/380001/content/browser/loader... content/browser/loader/resource_scheduler.cc:651: attributes = attributes | kAttributeInFlight; On 2015/08/13 21:27:45, mmenke wrote: > Any reason not to use |= on all of these? I wasn't sure if it would look too much like != and thought it would be easier to read when skimming but since it's stand-alone it looks readable enough with |=. Done. https://codereview.chromium.org/1230133005/diff/380001/content/browser/loader... content/browser/loader/resource_scheduler.cc:803: // non-delayable requests. On 2015/08/13 21:27:45, mmenke wrote: > Nit: "Non-delayable requests." (capitalize) Done. https://codereview.chromium.org/1230133005/diff/380001/content/browser/loader... content/browser/loader/resource_scheduler.cc:806: } On 2015/08/13 21:27:45, mmenke wrote: > nit: Remove braces I had them to be consistent with the rest of the function which had braces for all of the checks but to be consistent with the whole file and rest of code I also cleaned up the other instances in the function. Done.
The CQ bit was checked by pmeenan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmcquade@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1230133005/#ps400001 (title: "Fixed nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1230133005/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1230133005/400001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Hrm...Rather glad we have those checks
The CQ bit was checked by pmeenan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1230133005/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1230133005/420001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
On 2015/08/14 15:05:52, mmenke wrote: > Hrm...Rather glad we have those checks Running a dry-run of a fix just to make sure the try bots all look good with the fix. It was a problem with the DCHECK counting of the requests. It was only looking at the pending and in-flight lists but wasn't including the current request if it hadn't been added to a list yet. It wasn't an issue before because we used to only classify a request after a decision was made to start it or not and it was already in a queue. With the change to the classification, we now assign the attributes to a request as soon as it comes in and then make the start decision based on the attributes before putting it in a list. I fixed it so the DCHECK will also include the current request for it's accounting if it isn't in a list yet. I'll ping you after the CQ dry run is complete to take a look at the change before submitting for real.
The CQ bit was checked by pmeenan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1230133005/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1230133005/440001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mmenke@ couold you PTAL at the DCHECK counting fix when you get a chance: https://codereview.chromium.org/1230133005/diff2/400001:420001/content/browse... I also had to rebase after the fix but that was the last substantial change and fixed the issue with the trybots hitting the DCHECK. The dry-run on CQ ran clean this time.
LGTM. Not lovely, but I don't see a simpler solution.
The CQ bit was checked by pmeenan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmcquade@chromium.org Link to the patchset: https://codereview.chromium.org/1230133005/#ps440001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1230133005/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1230133005/440001
Message was sent while issue was closed.
Committed patchset #23 (id:440001)
Message was sent while issue was closed.
Patchset 23 (id:??) landed as https://crrev.com/9ac66968b024f1d33427e4776bba672aab8ffa59 Cr-Commit-Position: refs/heads/master@{#343681} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
