Fix Resource Priorities and Scheduling (Chrome Side)

content/browser/loader/resource_scheduler_unittest.cc

Index: content/browser/loader/resource_scheduler_unittest.cc
diff --git a/content/browser/loader/resource_scheduler_unittest.cc b/content/browser/loader/resource_scheduler_unittest.cc
index 3ac956667bfdafb138962616a723766662ad8c10..38af6b86d8579aa6dba3ae3c2757c7693383ef0d 100644
--- a/content/browser/loader/resource_scheduler_unittest.cc
+++ b/content/browser/loader/resource_scheduler_unittest.cc
@@ -384,10 +384,10 @@ TEST_F(ResourceSchedulerTest, OneLowLoadsUntilBodyInsertedExceptSpdy) {
   http_server_properties_.SetSupportsSpdy(
       net::HostPortPair("spdyhost", 443), true);
   scoped_ptr<TestRequest> high(NewRequest("http://host/high", net::HIGHEST));
-  scoped_ptr<TestRequest> low_spdy(
-      NewRequest("https://spdyhost/low", net::LOWEST));
   scoped_ptr<TestRequest> low(NewRequest("http://host/low", net::LOWEST));
   scoped_ptr<TestRequest> low2(NewRequest("http://host/low", net::LOWEST));
+  scoped_ptr<TestRequest> low_spdy(
+      NewRequest("https://spdyhost/low", net::LOWEST));
   EXPECT_TRUE(high->started());
   EXPECT_TRUE(low_spdy->started());
   EXPECT_TRUE(low->started());
@@ -707,8 +707,11 @@ TEST_F(ResourceSchedulerTest, NewSpdyHostInDelayableRequests) {
   http_server_properties_.SetSupportsSpdy(
       net::HostPortPair("spdyhost2", 8080), true);
   ChangeRequestPriority(low2_spdy.get(), net::LOWEST);
+  // Low priority in-flight requests on the SPDY connection are still considered
+  // delayable as far as the accounting goes and should still hold back the
+  // request on the non-spdy host.
   scoped_ptr<TestRequest> low2(NewRequest("http://host/low", net::LOWEST));
-  EXPECT_TRUE(low2->started());
+  EXPECT_FALSE(low2->started());
 }
 
 TEST_F(ResourceSchedulerTest, ThrottledClientCreation) {
@@ -2360,6 +2363,197 @@ TEST_F(ResourceSchedulerTest, RequestStartedAfterClientDeletedManyDelayable) {
   EXPECT_TRUE(lowest->started());
 }
 
+TEST_F(ResourceSchedulerTest, DefaultLayoutBlockingPriority) {
+  const int kEnablePriorityIncrease = 0;
+  ASSERT_TRUE(InitializeFieldTrials(
+      base::StringPrintf("ResourcePriorities/LayoutBlocking_000%d0_0_1_10/",

[mmenke, 2015/08/11 19:27:28]

Why the terminal slash on group names here?  The tests you added to the other
file don't have them.

[Pat Meenan, 2015/08/12 16:21:00]

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.

+                         kEnablePriorityIncrease)));
+  InitializeScheduler();
+  scoped_ptr<TestRequest> high(
+      NewRequest("http://hosthigh/high", net::HIGHEST));
+  scoped_ptr<TestRequest> high2(
+      NewRequest("http://hosthigh/high", net::HIGHEST));
+  scoped_ptr<TestRequest> medium(
+      NewRequest("http://hostmedium/medium", net::MEDIUM));
+  scoped_ptr<TestRequest> medium2(
+      NewRequest("http://hostmedium/medium", net::MEDIUM));
+  scoped_ptr<TestRequest> low(NewRequest("http://hostlow/low", net::LOW));
+  scoped_ptr<TestRequest> low2(NewRequest("http://hostlow/low", net::LOW));
+  scoped_ptr<TestRequest> lowest(NewRequest("http://hostlowest/lowest", net::LOWEST));
+  scoped_ptr<TestRequest> lowest2(
+      NewRequest("http://hostlowest/lowest", net::LOWEST));
+  EXPECT_TRUE(high->started());
+  EXPECT_TRUE(high2->started());
+  EXPECT_TRUE(medium->started());
+  EXPECT_TRUE(medium2->started());
+  EXPECT_TRUE(low->started());
+  EXPECT_TRUE(low2->started());
+  EXPECT_TRUE(lowest->started());
+  EXPECT_FALSE(lowest2->started());
+  lowest.reset();
+  EXPECT_TRUE(lowest2->started());
+}
+
+TEST_F(ResourceSchedulerTest, IncreaseLayoutBlockingPriority) {
+  // Changes the level of priorities that are allowed during layout-blocking
+  // from net::LOWEST to net::LOW.
+  const int kEnablePriorityIncrease = 1;

[mmenke, 2015/08/11 19:27:28]

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...).

[Pat Meenan, 2015/08/12 16:21:00]

Done.

+  ASSERT_TRUE(InitializeFieldTrials(
+      base::StringPrintf("ResourcePriorities/LayoutBlocking_000%d0_0_1_10/",
+                         kEnablePriorityIncrease)));
+  InitializeScheduler();
+  scoped_ptr<TestRequest> high(
+      NewRequest("http://hosthigh/high", net::HIGHEST));
+  scoped_ptr<TestRequest> high2(
+      NewRequest("http://hosthigh/high", net::HIGHEST));
+  scoped_ptr<TestRequest> medium(
+      NewRequest("http://hostmedium/medium", net::MEDIUM));
+  scoped_ptr<TestRequest> medium2(
+      NewRequest("http://hostmedium/medium", net::MEDIUM));
+  scoped_ptr<TestRequest> low(NewRequest("http://hostlow/low", net::LOW));
+  scoped_ptr<TestRequest> low2(NewRequest("http://hostlow/low", net::LOW));
+  scoped_ptr<TestRequest> lowest(NewRequest("http://hostlowest/lowest", net::LOWEST));
+  scoped_ptr<TestRequest> lowest2(
+      NewRequest("http://hostlowest/lowest", net::LOWEST));
+  EXPECT_TRUE(high->started());
+  EXPECT_TRUE(high2->started());
+  EXPECT_TRUE(medium->started());
+  EXPECT_TRUE(medium2->started());
+  EXPECT_TRUE(low->started());
+  EXPECT_FALSE(low2->started());
+  EXPECT_FALSE(lowest->started());
+  EXPECT_FALSE(lowest2->started());
+  low.reset();
+  EXPECT_TRUE(low2->started());
+  EXPECT_FALSE(lowest->started());
+  EXPECT_FALSE(lowest2->started());
+  low2.reset();
+  EXPECT_TRUE(lowest->started());
+  EXPECT_FALSE(lowest2->started());
+  lowest.reset();
+  EXPECT_TRUE(lowest2->started());
+}
+
+TEST_F(ResourceSchedulerTest, UseLayoutBlockingThresholdOne) {
+  // Prevents anly low priority requests from starting while more than

[mmenke, 2015/08/11 19:27:28]

anly -> any

[Pat Meenan, 2015/08/12 16:21:00]

Done.

+  // N high priority requests are pending (before body).
+  const int kLayoutBlockingThreshold = 1;
+  ASSERT_TRUE(InitializeFieldTrials(
+      base::StringPrintf("ResourcePriorities/LayoutBlocking_00001_%d_1_10/",
+                         kLayoutBlockingThreshold)));
+  InitializeScheduler();
+  scoped_ptr<TestRequest> high(NewRequest("http://host/high", net::HIGHEST));
+  scoped_ptr<TestRequest> high2(NewRequest("http://host/high", net::HIGHEST));
+  scoped_ptr<TestRequest> low(NewRequest("http://host/low", net::LOWEST));
+  scoped_ptr<TestRequest> low2(NewRequest("http://host/low", net::LOWEST));
+  EXPECT_TRUE(high->started());
+  EXPECT_TRUE(high2->started());
+  EXPECT_FALSE(low->started());
+  EXPECT_FALSE(low2->started());
+  high.reset();
+  EXPECT_TRUE(low->started());
+  EXPECT_FALSE(low2->started());
+  high2.reset();

[mmenke, 2015/08/11 19:27:28]

EXPECT_FALSE(low2->started()); ....  At least I think
max_num_delayable_while_layout_blocking_ is 1 here?

[Pat Meenan, 2015/08/12 16:21:00]

Done.

+  scheduler()->OnWillInsertBody(kChildId, kRouteId);
+  EXPECT_TRUE(low2->started());
+}
+
+TEST_F(ResourceSchedulerTest, UseLayoutBlockingThresholdTwo) {
+  // Prevents anly low priority requests from starting while more than

[mmenke, 2015/08/11 19:27:28]

any

[Pat Meenan, 2015/08/12 16:21:00]

Done.

+  // N high priority requests are pending (before body).
+  const int kLayoutBlockingThreshold = 2;
+  ASSERT_TRUE(InitializeFieldTrials(
+      base::StringPrintf("ResourcePriorities/LayoutBlockingTwo_00001_%d_1_10/",
+                         kLayoutBlockingThreshold)));
+  InitializeScheduler();
+  scoped_ptr<TestRequest> high(NewRequest("http://host/high", net::HIGHEST));
+  scoped_ptr<TestRequest> high2(NewRequest("http://host/high", net::HIGHEST));
+  scoped_ptr<TestRequest> high3(NewRequest("http://host/high", net::HIGHEST));
+  scoped_ptr<TestRequest> low(NewRequest("http://host/low", net::LOWEST));
+  scoped_ptr<TestRequest> low2(NewRequest("http://host/low", net::LOWEST));
+  EXPECT_TRUE(high->started());
+  EXPECT_TRUE(high2->started());
+  EXPECT_TRUE(high3->started());
+  EXPECT_FALSE(low->started());
+  EXPECT_FALSE(low2->started());
+  high.reset();
+  EXPECT_TRUE(low->started());
+  EXPECT_FALSE(low2->started());
+  high2.reset();
+  high3.reset();

[mmenke, 2015/08/11 19:27:28]

Again, should have EXPECT here for low2.

[Pat Meenan, 2015/08/12 16:21:00]

Done.

+  scheduler()->OnWillInsertBody(kChildId, kRouteId);
+  EXPECT_TRUE(low2->started());
+}
+
+TEST_F(ResourceSchedulerTest, IncreaseLowLoadsUntilBodyInserted) {

[mmenke, 2015/08/11 19:27:28]

I think TwoDelayableLoadsUntilBodyInserted would be more consistent with your
other test names.

[Pat Meenan, 2015/08/12 16:21:00]

Done.

+  const int kMaxNumDelayableWhileLayoutBlocking = 2;
+  ASSERT_TRUE(InitializeFieldTrials(
+      base::StringPrintf("ResourcePriorities/IncreaseLow_00000_0_%d_10/",
+                         kMaxNumDelayableWhileLayoutBlocking)));

[mmenke, 2015/08/11 19:27:28]

Any reason why the last 3 have no text describing the configuration?

[Pat Meenan, 2015/08/12 16:21:00]

No good reason - just added a comment explaining the test conditions for the
ones that were missing them.

+  InitializeScheduler();
+  scoped_ptr<TestRequest> high(NewRequest("http://host/high", net::HIGHEST));
+  scoped_ptr<TestRequest> low(NewRequest("http://host/low", net::LOWEST));
+  scoped_ptr<TestRequest> low2(NewRequest("http://host/low", net::LOWEST));
+  scoped_ptr<TestRequest> low3(NewRequest("http://host/low", net::LOWEST));
+  EXPECT_TRUE(high->started());
+  EXPECT_TRUE(low->started());
+  EXPECT_TRUE(low2->started());
+  EXPECT_FALSE(low3->started());
+  high.reset();
+  scheduler()->OnWillInsertBody(kChildId, kRouteId);
+  EXPECT_TRUE(low3->started());
+}
+
+TEST_F(ResourceSchedulerTest, IncreaseLowLoadsUntilBodyInsertedLayoutBlocking) {

[mmenke, 2015/08/11 19:27:28]

UseLayoutBlockingThresholdOneAndTwoDelayableLoadsUntilBodyInserted?

[Pat Meenan, 2015/08/12 16:21:00]

Done.

+  const int kLayoutBlockingThreshold = 1;
+  const int kMaxNumDelayableWhileLayoutBlocking = 2;
+  ASSERT_TRUE(InitializeFieldTrials(
+      base::StringPrintf("ResourcePriorities/IncreaseLow_00001_%d_%d_10/",
+                         kLayoutBlockingThreshold,
+                         kMaxNumDelayableWhileLayoutBlocking)));
+  InitializeScheduler();
+  scoped_ptr<TestRequest> high(NewRequest("http://host/high", net::HIGHEST));
+  scoped_ptr<TestRequest> high2(NewRequest("http://host/high", net::HIGHEST));
+  scoped_ptr<TestRequest> low(NewRequest("http://host/low", net::LOWEST));
+  scoped_ptr<TestRequest> low2(NewRequest("http://host/low", net::LOWEST));
+  scoped_ptr<TestRequest> low3(NewRequest("http://host/low", net::LOWEST));
+  EXPECT_TRUE(high->started());
+  EXPECT_TRUE(high2->started());
+  EXPECT_FALSE(low->started());
+  EXPECT_FALSE(low2->started());
+  EXPECT_FALSE(low3->started());
+  high.reset();
+  EXPECT_TRUE(low->started());
+  EXPECT_TRUE(low2->started());
+  EXPECT_FALSE(low3->started());
+  high2.reset();
+  scheduler()->OnWillInsertBody(kChildId, kRouteId);
+  EXPECT_TRUE(low3->started());
+}
+
+TEST_F(ResourceSchedulerTest, IncreaseMaxNumDelayableRequestsPerClient) {
+  const int kMaxNumDelayableRequestsPerClient = 25;
+  ASSERT_TRUE(InitializeFieldTrials(
+      base::StringPrintf("ResourcePriorities/IncreaseDelayable_00000_0_1_%d/",
+                         kMaxNumDelayableRequestsPerClient)));

[mmenke, 2015/08/11 19:27:28]

Should we have another simple test that checks the
MaxNumDelayableRequestsPerClient when the other stuff is enabled?  i.e.
00001_...?

[Pat Meenan, 2015/08/12 16:21:00]

Done.

+  InitializeScheduler();
+
+  // We only load low priority resources if there's a body.

[mmenke, 2015/08/11 19:27:28]

nit:  Avoid using "we" in comments.

[Pat Meenan, 2015/08/12 16:21:00]

Done.

+  scheduler()->OnWillInsertBody(kChildId, kRouteId);
+
+  // Queue requests from different hosts until we reach the total limit.
+  ScopedVector<TestRequest> lows_differenthosts;
+  for (int i = 0; i < kMaxNumDelayableRequestsPerClient; ++i) {
+    string url = "http://host" + base::IntToString(i) + "/low";
+    lows_differenthosts.push_back(NewRequest(url.c_str(), net::LOWEST));
+    EXPECT_TRUE(lows_differenthosts[i]->started());
+  }
+
+  scoped_ptr<TestRequest> last_differenthost(NewRequest("http://host_new/last",
+                                                        net::LOWEST));
+  EXPECT_FALSE(last_differenthost->started());
+}
+
 }  // unnamed namespace
 
 }  // namespace content