Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2655393004:
Make ResourceScheduler work in OOPIF (Closed)
|
Created:
3 years, 10 months ago by Charlie Harrison Modified:
3 years, 10 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, Randy Smith (Not in Mondays), darin-cc_chromium.org, loading-reviews_chromium.org, site-isolation-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake ResourceScheduler work in OOPIF
This CL lets all frames send the "will insert body" IPC to the browser
process, rather than just main frames. This enables OOPIFs to get past
the throttling stage in ResourceScheduler.
Note that OnNavigate is still gated on main frame, so navigations within
an OOPIF will not reset the throttling state. This is a definite
performance bug, but is less critical than the current state, which
only allows a single delayable request at a time (so a hung request
will starve all delayable requests).
This CL also adds throttler-agnostic tests, as ResourceScheduler is
going away soon so we don't want to rely on its specific layering, as
it is being migrated to a global throttler in the network layer.
BUG=678206
Review-Url: https://codereview.chromium.org/2655393004
Cr-Commit-Position: refs/heads/master@{#447792}
Committed: https://chromium.googlesource.com/chromium/src/+/d86c35bcf47e34f09f295127796ea246746b5ab1
Patch Set 1 #Patch Set 2 : Add tests #
Total comments: 32
Patch Set 3 : mmenke + nasko reviews #
Total comments: 2
Patch Set 4 : self review #
Total comments: 29
Patch Set 5 : mmenke review #
Total comments: 2
Patch Set 6 : Make ResourceScheduler work in OOPIF #Messages
Total messages: 37 (22 generated)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Make ResourceScheduler work in OOPIF BUG=TODO ========== to ========== Make ResourceScheduler work in OOPIF BUG=678206 ==========
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Make ResourceScheduler work in OOPIF BUG=678206 ========== to ========== Make ResourceScheduler work in OOPIF This CL lets all frames send the "will insert body" IPC to the browser process, rather than just main frames. This enables OOPIFs to get past the throttling stage in ResourceScheduler. Note that OnNavigate is still gated on main frame, so navigations within an OOPIF will not reset the throttling state. This is a definite performance bug, but is less critical than the current state, which only allows a single delayable request at a time (so a hung request will starve all delayable requests). This CL also adds throttler-agnostic tests, as ResourceScheduler is going away soon so we don't want to rely on its specific layering, as it is being migrated to a global throttler in the network layer. BUG=678206 ==========
csharrison@chromium.org changed reviewers: + mmenke@chromium.org
mmenke: would you PTAL? I added the tests to site isolation tests because that's where the problem arose, but LMK if you'd prefer me to write a new test suite.
Oh, also I punted on a test with nested iframes just because it would make the fixture much more complicated and I think not much added coverage.
On 2017/01/30 19:15:35, Charlie Harrison wrote: > Oh, also I punted on a test with nested iframes just because it would make the > fixture much more complicated and I think not much added coverage. Should someone more familiar with navigation also take a look, clamy or nasko or somebody?
Sorry I should have been more specific. Could you specifically look at the test harness which is very net-y? The rest is pretty trivial and I will have a content owner review it.
csharrison@chromium.org changed reviewers: + nasko@chromium.org
nasko: Would you PTAL at everything?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2655393004/diff/10001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2655393004/diff/10001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:583: send_response_closures_.push_back(base::Bind(send, "", done)); I think you should document what "base::Bind(send, "", done)" does. It's not exactly clear that it creates a closure that just closes the socket without sending a response. https://codereview.chromium.org/2655393004/diff/10001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:588: for (auto it : delayed_requests_for_path_) { delayed_requests_for_path_ seems a bit confusing. "num_remaining_requests_to_delay_for_path_"? https://codereview.chromium.org/2655393004/diff/10001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:589: sum += it.second; if (it.second > 0) return; https://codereview.chromium.org/2655393004/diff/10001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:593: base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, it); This isn't threadsafe. DelayedResponse is called on the TestServer's own thread, not the UI thread. So you need to post a task over to the UI thread to add stuff to send_response_closures_, and then to resume, you need to post a task back over to the IO thread. https://codereview.chromium.org/2655393004/diff/10001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:613: SitePerProcessBrowserTest::HandleMockResource( Think this needs documentation. https://codereview.chromium.org/2655393004/diff/10001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:640: void SitePerProcessBrowserTest::SetDelayedRequestsForPath(std::string path, nit: const std::string& path? https://codereview.chromium.org/2655393004/diff/10001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:640: void SitePerProcessBrowserTest::SetDelayedRequestsForPath(std::string path, Think this needs documentation. https://codereview.chromium.org/2655393004/diff/10001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9256: // delayable request (e.g. video) starving all other delayable requests. Think this needs more details on how the tests actually work. https://codereview.chromium.org/2655393004/diff/10001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9260: SetDelayedRequestsForPath(path, 2); Not safe to do this after setting up the test server, since the test server runs on its own thread. https://codereview.chromium.org/2655393004/diff/10001/content/test/data/site_... File content/test/data/site_isolation/subframes_with_resources.html (right): https://codereview.chromium.org/2655393004/diff/10001/content/test/data/site_... content/test/data/site_isolation/subframes_with_resources.html:23: window.domAutomationController.send(true); Can this happen before ExecuteScriptAndExtractBool is executed? If so, does window.domAutomationController correctly handle that case? If not, why not?
https://codereview.chromium.org/2655393004/diff/10001/content/browser/loader/... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/2655393004/diff/10001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:424: // Can be called multiple times per RVH in the case of OOPIFs. nit: Let's not use the abbreviation OOPIF, as not everyone is familiar with it. https://codereview.chromium.org/2655393004/diff/10001/content/browser/loader/... File content/browser/loader/resource_scheduler_filter.cc (right): https://codereview.chromium.org/2655393004/diff/10001/content/browser/loader/... content/browser/loader/resource_scheduler_filter.cc:36: // propogate OnNavigate to the client associated with the OOPIF's RVH. This nit: "propagate"? https://codereview.chromium.org/2655393004/diff/10001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2655393004/diff/10001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9254: // Regression tests for crbug.com/678206, where the request throttling in nit: https:// https://codereview.chromium.org/2655393004/diff/10001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9287: EXPECT_TRUE(result); How do those tests manifest when they fail? Timeout? Crash? Maybe add this in the comment description of the tests cases. https://codereview.chromium.org/2655393004/diff/10001/content/browser/site_pe... File content/browser/site_per_process_browsertest.h (right): https://codereview.chromium.org/2655393004/diff/10001/content/browser/site_pe... content/browser/site_per_process_browsertest.h:73: std::map<std::string, int> delayed_requests_for_path_; All of these changes don't seem appropriate to add for this test class, as all of it will be used for a single test case. Why not create a new test class specifically for this test case? https://codereview.chromium.org/2655393004/diff/10001/content/test/data/site_... File content/test/data/site_isolation/subframe_resources.html (right): https://codereview.chromium.org/2655393004/diff/10001/content/test/data/site_... content/test/data/site_isolation/subframe_resources.html:10: numError++; Do we know this was called due to an error?
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Tried to address all concerns. I made the test suite a new class in the cc file. Many thanks for the helpful reviews. https://codereview.chromium.org/2655393004/diff/10001/content/browser/loader/... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/2655393004/diff/10001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:424: // Can be called multiple times per RVH in the case of OOPIFs. On 2017/01/31 15:50:34, nasko (very slow) wrote: > nit: Let's not use the abbreviation OOPIF, as not everyone is familiar with it. Done. https://codereview.chromium.org/2655393004/diff/10001/content/browser/loader/... File content/browser/loader/resource_scheduler_filter.cc (right): https://codereview.chromium.org/2655393004/diff/10001/content/browser/loader/... content/browser/loader/resource_scheduler_filter.cc:36: // propogate OnNavigate to the client associated with the OOPIF's RVH. This On 2017/01/31 15:50:34, nasko (very slow) wrote: > nit: "propagate"? Done. https://codereview.chromium.org/2655393004/diff/10001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2655393004/diff/10001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:583: send_response_closures_.push_back(base::Bind(send, "", done)); On 2017/01/30 23:03:51, mmenke (Out Feb 4 to March 5) wrote: > I think you should document what "base::Bind(send, "", done)" does. It's not > exactly clear that it creates a closure that just closes the socket without > sending a response. Done. https://codereview.chromium.org/2655393004/diff/10001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:588: for (auto it : delayed_requests_for_path_) { On 2017/01/30 23:03:51, mmenke (Out Feb 4 to March 5) wrote: > delayed_requests_for_path_ seems a bit confusing. > "num_remaining_requests_to_delay_for_path_"? Done. https://codereview.chromium.org/2655393004/diff/10001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:589: sum += it.second; On 2017/01/30 23:03:51, mmenke (Out Feb 4 to March 5) wrote: > if (it.second > 0) > return; Done. https://codereview.chromium.org/2655393004/diff/10001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:593: base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, it); On 2017/01/30 23:03:51, mmenke (Out Feb 4 to March 5) wrote: > This isn't threadsafe. DelayedResponse is called on the TestServer's own > thread, not the UI thread. So you need to post a task over to the UI thread to > add stuff to send_response_closures_, and then to resume, you need to post a > task back over to the IO thread. I thought this would be okay because send_response_closures_ is only ever accessed or modified on the test servers own thread. https://codereview.chromium.org/2655393004/diff/10001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:613: SitePerProcessBrowserTest::HandleMockResource( On 2017/01/30 23:03:51, mmenke (Out Feb 4 to March 5) wrote: > Think this needs documentation. Done. https://codereview.chromium.org/2655393004/diff/10001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:640: void SitePerProcessBrowserTest::SetDelayedRequestsForPath(std::string path, On 2017/01/30 23:03:51, mmenke (Out Feb 4 to March 5) wrote: > nit: const std::string& path? Done. https://codereview.chromium.org/2655393004/diff/10001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:640: void SitePerProcessBrowserTest::SetDelayedRequestsForPath(std::string path, On 2017/01/30 23:03:51, mmenke (Out Feb 4 to March 5) wrote: > Think this needs documentation. Added docs to the header. https://codereview.chromium.org/2655393004/diff/10001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9254: // Regression tests for crbug.com/678206, where the request throttling in On 2017/01/31 15:50:34, nasko (very slow) wrote: > nit: https:// Done. https://codereview.chromium.org/2655393004/diff/10001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9256: // delayable request (e.g. video) starving all other delayable requests. On 2017/01/30 23:03:51, mmenke (Out Feb 4 to March 5) wrote: > Think this needs more details on how the tests actually work. Done. https://codereview.chromium.org/2655393004/diff/10001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9260: SetDelayedRequestsForPath(path, 2); On 2017/01/30 23:03:51, mmenke (Out Feb 4 to March 5) wrote: > Not safe to do this after setting up the test server, since the test server runs > on its own thread. Ensured that the test server is not started until after this is called. https://codereview.chromium.org/2655393004/diff/10001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9287: EXPECT_TRUE(result); On 2017/01/31 15:50:34, nasko (very slow) wrote: > How do those tests manifest when they fail? Timeout? Crash? Maybe add this in > the comment description of the tests cases. They will time out, I've updated the description. https://codereview.chromium.org/2655393004/diff/10001/content/browser/site_pe... File content/browser/site_per_process_browsertest.h (right): https://codereview.chromium.org/2655393004/diff/10001/content/browser/site_pe... content/browser/site_per_process_browsertest.h:73: std::map<std::string, int> delayed_requests_for_path_; On 2017/01/31 15:50:34, nasko (very slow) wrote: > All of these changes don't seem appropriate to add for this test class, as all > of it will be used for a single test case. Why not create a new test class > specifically for this test case? Done. Subclass'd https://codereview.chromium.org/2655393004/diff/10001/content/test/data/site_... File content/test/data/site_isolation/subframe_resources.html (right): https://codereview.chromium.org/2655393004/diff/10001/content/test/data/site_... content/test/data/site_isolation/subframe_resources.html:10: numError++; On 2017/01/31 15:50:34, nasko (very slow) wrote: > Do we know this was called due to an error? No this is a holdover. I think this var should be renamed numDone. https://codereview.chromium.org/2655393004/diff/10001/content/test/data/site_... File content/test/data/site_isolation/subframes_with_resources.html (right): https://codereview.chromium.org/2655393004/diff/10001/content/test/data/site_... content/test/data/site_isolation/subframes_with_resources.html:23: window.domAutomationController.send(true); On 2017/01/30 23:03:51, mmenke (Out Feb 4 to March 5) wrote: > Can this happen before ExecuteScriptAndExtractBool is executed? If so, does > window.domAutomationController correctly handle that case? If not, why not? It cannot hapen before ExecuteScriptAndExtractBool, because the script executed is "createFrames()". Before that there are no subframes. https://codereview.chromium.org/2655393004/diff/30001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (left): https://codereview.chromium.org/2655393004/diff/30001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:15: #include "base/macros.h" remove this https://codereview.chromium.org/2655393004/diff/30001/content/browser/site_pe... File content/browser/site_per_process_browsertest.h (right): https://codereview.chromium.org/2655393004/diff/30001/content/browser/site_pe... content/browser/site_per_process_browsertest.h:10: #include "base/callback.h" removed this
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Test looks pretty good, though I'm very confused about those IO thread DCHECKs, as they aren't run on the IO thread. https://codereview.chromium.org/2655393004/diff/50001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2655393004/diff/50001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9202: const net::test_server::SendCompleteCallback& done) { Comment that this is called on the test server's thread? https://codereview.chromium.org/2655393004/diff/50001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9206: } This can be private (Inner classes can access even private class members of outer classes, though outer classes only have public access to their inner classes) https://codereview.chromium.org/2655393004/diff/50001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9218: protected: no need for protected - it doesn't really do anything. Nothing has access to this class in a manner where protected behaves differently from public. https://codereview.chromium.org/2655393004/diff/50001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9223: void SetDelayedRequestsForPath(const std::string& path, int num_delayed) { DCHECK that the test_server isn't started? https://codereview.chromium.org/2655393004/diff/50001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9237: DCHECK_CURRENTLY_ON(BrowserThread::IO); This DCHECK passes? The TestServer uses its own thread, not the IO thread - it can't even depend on BrowserThread. I'm very confused. https://codereview.chromium.org/2655393004/diff/50001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9244: // which nit: reflow comment. https://codereview.chromium.org/2655393004/diff/50001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9247: if (it->second) { nit: > 0, to make it clear this is an int we're looking at, and not a pointer.. https://codereview.chromium.org/2655393004/diff/50001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9256: // all the delayed tasks. Again, mention thread this is called on. https://codereview.chromium.org/2655393004/diff/50001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9264: base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, it); Why does this need to use PostTask? https://codereview.chromium.org/2655393004/diff/50001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9269: // which can call it at any time to finish a request. I'm having trouble following this comment. Maybe: "This class passes the callbacks needed to respond to a request to the underlying test fixture." https://codereview.chromium.org/2655393004/diff/50001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9272: DelayedResponse(RequestDelayingSitePerProcessBrowserTest* test_harness) explicit https://codereview.chromium.org/2655393004/diff/50001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9293: std::map<std::string, int> num_remaining_requests_to_delay_for_path_; include <string> https://codereview.chromium.org/2655393004/diff/50001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9303: // The tests work by delaying N requests in a cross-domain iframe. Once the n + nit: This paragraph uses N here and n below. https://codereview.chromium.org/2655393004/diff/50001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9307: // If the logic is not correct, these tests will time out, as the n + 1 request nit: n + 1 request seems a bit confusing - it could mean the n+1th (n+1st?) request, or n + 1 requests. n + 1th? (n + 1)th? request n + 1? Same as above.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Still looking into why the DCHECKs were being flaky, but addressed all other points. https://codereview.chromium.org/2655393004/diff/50001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2655393004/diff/50001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9202: const net::test_server::SendCompleteCallback& done) { On 2017/02/01 20:21:40, mmenke (Out Feb 4 to March 5) wrote: > Comment that this is called on the test server's thread? Done. https://codereview.chromium.org/2655393004/diff/50001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9202: const net::test_server::SendCompleteCallback& done) { On 2017/02/01 20:21:40, mmenke (Out Feb 4 to March 5) wrote: > Comment that this is called on the test server's thread? Done. https://codereview.chromium.org/2655393004/diff/50001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9206: } On 2017/02/01 20:21:40, mmenke (Out Feb 4 to March 5) wrote: > This can be private (Inner classes can access even private class members of > outer classes, though outer classes only have public access to their inner > classes) Done. https://codereview.chromium.org/2655393004/diff/50001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9218: protected: On 2017/02/01 20:21:40, mmenke (Out Feb 4 to March 5) wrote: > no need for protected - it doesn't really do anything. Nothing has access to > this class in a manner where protected behaves differently from public. Done. https://codereview.chromium.org/2655393004/diff/50001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9223: void SetDelayedRequestsForPath(const std::string& path, int num_delayed) { On 2017/02/01 20:21:40, mmenke (Out Feb 4 to March 5) wrote: > DCHECK that the test_server isn't started? Done. https://codereview.chromium.org/2655393004/diff/50001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9237: DCHECK_CURRENTLY_ON(BrowserThread::IO); On 2017/02/01 20:21:40, mmenke (Out Feb 4 to March 5) wrote: > This DCHECK passes? The TestServer uses its own thread, not the IO thread - it > can't even depend on BrowserThread. I'm very confused. Me too, I am investigating why. For now I removed it. https://codereview.chromium.org/2655393004/diff/50001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9244: // which On 2017/02/01 20:21:40, mmenke (Out Feb 4 to March 5) wrote: > nit: reflow comment. Done. https://codereview.chromium.org/2655393004/diff/50001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9247: if (it->second) { On 2017/02/01 20:21:40, mmenke (Out Feb 4 to March 5) wrote: > nit: > 0, to make it clear this is an int we're looking at, and not a pointer.. Done. https://codereview.chromium.org/2655393004/diff/50001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9256: // all the delayed tasks. On 2017/02/01 20:21:40, mmenke (Out Feb 4 to March 5) wrote: > Again, mention thread this is called on. Done. https://codereview.chromium.org/2655393004/diff/50001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9264: base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, it); On 2017/02/01 20:21:40, mmenke (Out Feb 4 to March 5) wrote: > Why does this need to use PostTask? It doesn't need to. https://codereview.chromium.org/2655393004/diff/50001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9269: // which can call it at any time to finish a request. On 2017/02/01 20:21:40, mmenke (Out Feb 4 to March 5) wrote: > I'm having trouble following this comment. > > Maybe: "This class passes the callbacks needed to respond to a request to the > underlying test fixture." Much better. Changed. https://codereview.chromium.org/2655393004/diff/50001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9272: DelayedResponse(RequestDelayingSitePerProcessBrowserTest* test_harness) On 2017/02/01 20:21:40, mmenke (Out Feb 4 to March 5) wrote: > explicit Done. https://codereview.chromium.org/2655393004/diff/50001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9293: std::map<std::string, int> num_remaining_requests_to_delay_for_path_; On 2017/02/01 20:21:41, mmenke (Out Feb 4 to March 5) wrote: > include <string> <string> is in the header. https://codereview.chromium.org/2655393004/diff/50001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9303: // The tests work by delaying N requests in a cross-domain iframe. Once the n + On 2017/02/01 20:21:40, mmenke (Out Feb 4 to March 5) wrote: > nit: This paragraph uses N here and n below. Changed to "n". https://codereview.chromium.org/2655393004/diff/50001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9307: // If the logic is not correct, these tests will time out, as the n + 1 request On 2017/02/01 20:21:40, mmenke (Out Feb 4 to March 5) wrote: > nit: n + 1 request seems a bit confusing - it could mean the n+1th (n+1st?) > request, or n + 1 requests. n + 1th? (n + 1)th? request n + 1? Same as > above. Changed to n + 1st.
browsertests LGTM, didn't look at the rest. https://codereview.chromium.org/2655393004/diff/70001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2655393004/diff/70001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9226: // called on the test server's thread. nit: Move second sentence (Or both?) to before start of method.
Thanks, nasko: PTAL? https://codereview.chromium.org/2655393004/diff/70001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2655393004/diff/70001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:9226: // called on the test server's thread. On 2017/02/01 22:38:14, mmenke (Out Feb 4 to March 5) wrote: > nit: Move second sentence (Or both?) to before start of method. Done.
LGTM
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2655393004/#ps90001 (title: "Make ResourceScheduler work in OOPIF")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 90001, "attempt_start_ts": 1486052714650460,
"parent_rev": "7551f612ef47cc7d5eaf27771728cde7e0b51f26", "commit_rev":
"d86c35bcf47e34f09f295127796ea246746b5ab1"}
Message was sent while issue was closed.
Description was changed from ========== Make ResourceScheduler work in OOPIF This CL lets all frames send the "will insert body" IPC to the browser process, rather than just main frames. This enables OOPIFs to get past the throttling stage in ResourceScheduler. Note that OnNavigate is still gated on main frame, so navigations within an OOPIF will not reset the throttling state. This is a definite performance bug, but is less critical than the current state, which only allows a single delayable request at a time (so a hung request will starve all delayable requests). This CL also adds throttler-agnostic tests, as ResourceScheduler is going away soon so we don't want to rely on its specific layering, as it is being migrated to a global throttler in the network layer. BUG=678206 ========== to ========== Make ResourceScheduler work in OOPIF This CL lets all frames send the "will insert body" IPC to the browser process, rather than just main frames. This enables OOPIFs to get past the throttling stage in ResourceScheduler. Note that OnNavigate is still gated on main frame, so navigations within an OOPIF will not reset the throttling state. This is a definite performance bug, but is less critical than the current state, which only allows a single delayable request at a time (so a hung request will starve all delayable requests). This CL also adds throttler-agnostic tests, as ResourceScheduler is going away soon so we don't want to rely on its specific layering, as it is being migrated to a global throttler in the network layer. BUG=678206 Review-Url: https://codereview.chromium.org/2655393004 Cr-Commit-Position: refs/heads/master@{#447792} Committed: https://chromium.googlesource.com/chromium/src/+/d86c35bcf47e34f09f295127796e... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:90001) as https://chromium.googlesource.com/chromium/src/+/d86c35bcf47e34f09f295127796e... |
