Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2682163002:
[ResourceScheduler] Yield after starting several requests (Closed)
Description[ResourceScheduler] Yield after starting several requests
This CL periodically yields the ResourceScheduler after starting several
requests. This gives existing IO tasks (such as active network requests) a
chance to run between network starts. This should result in less jank and more
interleaved network request tasks.
BUG=655585
Review-Url: https://codereview.chromium.org/2682163002
Cr-Commit-Position: refs/heads/master@{#449895}
Committed: https://chromium.googlesource.com/chromium/src/+/1a8d5f3805dbae5242cf7dce14df3807344dae37
Patch Set 1 #Patch Set 2 : Default to 5 #Patch Set 3 : Nits #Patch Set 4 : New trigger #Patch Set 5 : Bug fixes #Patch Set 6 : Nits #Patch Set 7 : Test nits #
Total comments: 15
Patch Set 8 : Address comments from PS7 #
Total comments: 2
Patch Set 9 : Fix variable name #
Total comments: 1
Depends on Patchset: Messages
Total messages: 60 (38 generated)
The CQ bit was checked by jkarlin@chromium.org to run a CQ dry run
jkarlin@chromium.org changed reviewers: + csharrison@chromium.org
csharrison@ PTAL at everything, thanks!
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 ========== [ResourceScheduler] Yield after starting several requests This CL periodically yields the ResourceScheduler after starting several requests. This gives existing IO tasks (such as active network requests) a chance to run between network starts. This should result in less jank and more interleaved network request tasks. BUG=655585 ========== to ========== [ResourceScheduler] Yield after starting several requests This CL periodically yields the ResourceScheduler after starting several requests. This gives existing IO tasks (such as active network requests) a chance to run between network starts. This should result in less jank and more interleaved network request tasks. BUG=655585 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #4 (id:60001) has been deleted
Can you make it so tests use this mode by default?
The CQ bit was checked by jkarlin@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 checked by jkarlin@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 checked by jkarlin@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.
Patchset #7 (id:140001) has been deleted
Patchset #6 (id:120001) has been deleted
The CQ bit was checked by jkarlin@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 checked by jkarlin@chromium.org to run a CQ dry run
Patchset #6 (id:160001) has been deleted
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 checked by jkarlin@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...
On 2017/02/08 18:10:42, Charlie Harrison wrote: > Can you make it so tests use this mode by default? Done. PTAL. Thanks!
Generally looks good. I'm thinking of a way to rename "ResumeAfterYielding" to be a bit clearer that it can run even if we never yielded, maybe "ResumeIfSchedulerWasYielded"? https://codereview.chromium.org/2682163002/diff/200001/content/browser/loader... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/2682163002/diff/200001/content/browser/loader... content/browser/loader/resource_scheduler.cc:75: START_YIELDED, nit: I think START_WAS_YIELDED is a bit better. https://codereview.chromium.org/2682163002/diff/200001/content/browser/loader... content/browser/loader/resource_scheduler.cc:385: if (ShouldStartRequest(request) == START_REQUEST && Eek, I am a little worried about sync XHRs here. Maybe not a big deal in practice? https://codereview.chromium.org/2682163002/diff/200001/content/browser/loader... content/browser/loader/resource_scheduler.cc:762: requests_since_yielding_ = 0; // A single request can trigger this method, so don't continue searching // unless we really yielded. bool should_search_for_requests = requests_since_yielding_ > max_requests_before_yielding_; requests_since_yielding_ = 0; if (should_search_for_requests) LoadAnyStartable... https://codereview.chromium.org/2682163002/diff/200001/content/browser/loader... content/browser/loader/resource_scheduler.cc:774: if (requests_since_yielding_ == 0) { The fact that we post a task immediately deserves some explanation. https://codereview.chromium.org/2682163002/diff/200001/content/browser/loader... content/browser/loader/resource_scheduler.cc:780: return requests_since_yielding_++ >= max_requests_before_yielding_; optional: Can you put the increment on a separate line? This adds cognitive burden for me. https://codereview.chromium.org/2682163002/diff/200001/content/browser/loader... File content/browser/loader/resource_scheduler.h (right): https://codereview.chromium.org/2682163002/diff/200001/content/browser/loader... content/browser/loader/resource_scheduler.h:145: bool yielding_scheduler_; Without seeing the type, "yielding_scheduler_" implies to me another object that is in charge of doing this scheduler. I would prefer "enable_request_yielding_" maybe? https://codereview.chromium.org/2682163002/diff/200001/content/browser/loader... File content/browser/loader/resource_scheduler_unittest.cc (right): https://codereview.chromium.org/2682163002/diff/200001/content/browser/loader... content/browser/loader/resource_scheduler_unittest.cc:55: const int kMaxRequestsBeforeYielding = 5; // sync with .cc. Won't this be wrong if the variation param changes?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jkarlin@chromium.org to run a CQ dry run
Thanks! PTAL. https://codereview.chromium.org/2682163002/diff/200001/content/browser/loader... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/2682163002/diff/200001/content/browser/loader... content/browser/loader/resource_scheduler.cc:75: START_YIELDED, On 2017/02/09 19:08:04, Charlie Harrison wrote: > nit: I think START_WAS_YIELDED is a bit better. Done. https://codereview.chromium.org/2682163002/diff/200001/content/browser/loader... content/browser/loader/resource_scheduler.cc:385: if (ShouldStartRequest(request) == START_REQUEST && On 2017/02/09 19:08:04, Charlie Harrison wrote: > Eek, I am a little worried about sync XHRs here. Maybe not a big deal in > practice? Nice catch. I've changed it to not yield for sync requests and added a test. https://codereview.chromium.org/2682163002/diff/200001/content/browser/loader... content/browser/loader/resource_scheduler.cc:762: requests_since_yielding_ = 0; On 2017/02/09 19:08:04, Charlie Harrison wrote: > // A single request can trigger this method, so don't continue searching > // unless we really yielded. > bool should_search_for_requests = requests_since_yielding_ > > max_requests_before_yielding_; > requests_since_yielding_ = 0; > if (should_search_for_requests) > LoadAnyStartable... Thanks, I forgot to add that back when I switched from starting task when first yielding to first request. https://codereview.chromium.org/2682163002/diff/200001/content/browser/loader... content/browser/loader/resource_scheduler.cc:774: if (requests_since_yielding_ == 0) { On 2017/02/09 19:08:04, Charlie Harrison wrote: > The fact that we post a task immediately deserves some explanation. Done. https://codereview.chromium.org/2682163002/diff/200001/content/browser/loader... content/browser/loader/resource_scheduler.cc:780: return requests_since_yielding_++ >= max_requests_before_yielding_; On 2017/02/09 19:08:04, Charlie Harrison wrote: > optional: Can you put the increment on a separate line? This adds cognitive > burden for me. Done. https://codereview.chromium.org/2682163002/diff/200001/content/browser/loader... File content/browser/loader/resource_scheduler.h (right): https://codereview.chromium.org/2682163002/diff/200001/content/browser/loader... content/browser/loader/resource_scheduler.h:145: bool yielding_scheduler_; On 2017/02/09 19:08:04, Charlie Harrison wrote: > Without seeing the type, "yielding_scheduler_" implies to me another object that > is in charge of doing this scheduler. > > I would prefer "enable_request_yielding_" maybe? Changed to yielding_scheduler_enabled_. How does that sound? https://codereview.chromium.org/2682163002/diff/200001/content/browser/loader... File content/browser/loader/resource_scheduler_unittest.cc (right): https://codereview.chromium.org/2682163002/diff/200001/content/browser/loader... content/browser/loader/resource_scheduler_unittest.cc:55: const int kMaxRequestsBeforeYielding = 5; // sync with .cc. On 2017/02/09 19:08:04, Charlie Harrison wrote: > Won't this be wrong if the variation param changes? The variation param isn't part of any of the tests. The tests just use the default value.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
+kinuko FYI, this is an approach we are trying to get rid of some task starvation on the IO thread. It uses a similar trick we do for reprioritization. LGTM https://codereview.chromium.org/2682163002/diff/200001/content/browser/loader... File content/browser/loader/resource_scheduler.h (right): https://codereview.chromium.org/2682163002/diff/200001/content/browser/loader... content/browser/loader/resource_scheduler.h:145: bool yielding_scheduler_; On 2017/02/09 19:48:13, jkarlin wrote: > On 2017/02/09 19:08:04, Charlie Harrison wrote: > > Without seeing the type, "yielding_scheduler_" implies to me another object > that > > is in charge of doing this scheduler. > > > > I would prefer "enable_request_yielding_" maybe? > > Changed to yielding_scheduler_enabled_. How does that sound? Sounds good! https://codereview.chromium.org/2682163002/diff/220001/content/browser/loader... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/2682163002/diff/220001/content/browser/loader... content/browser/loader/resource_scheduler.cc:762: bool yieldedRequests = nit: yielded_requests. Too much blink style for you!
jkarlin@chromium.org changed reviewers: + isherman@chromium.org
isherman@chromium.org: Please review changes in testing/variations/ Thanks!
The CQ bit was checked by jkarlin@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...
Thanks! https://codereview.chromium.org/2682163002/diff/220001/content/browser/loader... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/2682163002/diff/220001/content/browser/loader... content/browser/loader/resource_scheduler.cc:762: bool yieldedRequests = On 2017/02/09 20:43:29, Charlie Harrison wrote: > nit: yielded_requests. Too much blink style for you! Done.
Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/09 20:43:29, Charlie Harrison wrote: > +kinuko FYI, this is an approach we are trying to get rid of some task > starvation on the IO thread. It uses a similar trick we do for reprioritization. Thanks for cc-ing, helps me to have better context. Yeah yielding approach would generally work, s-g-t-m.
testing/variations lgtm
The CQ bit was checked by jkarlin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from csharrison@chromium.org Link to the patchset: https://codereview.chromium.org/2682163002/#ps240001 (title: "Fix variable name")
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": 240001, "attempt_start_ts": 1486926693048360,
"parent_rev": "29d19bee1db32db8ba890b906383ba743959c08f", "commit_rev":
"1a8d5f3805dbae5242cf7dce14df3807344dae37"}
Message was sent while issue was closed.
Description was changed from ========== [ResourceScheduler] Yield after starting several requests This CL periodically yields the ResourceScheduler after starting several requests. This gives existing IO tasks (such as active network requests) a chance to run between network starts. This should result in less jank and more interleaved network request tasks. BUG=655585 ========== to ========== [ResourceScheduler] Yield after starting several requests This CL periodically yields the ResourceScheduler after starting several requests. This gives existing IO tasks (such as active network requests) a chance to run between network starts. This should result in less jank and more interleaved network request tasks. BUG=655585 Review-Url: https://codereview.chromium.org/2682163002 Cr-Commit-Position: refs/heads/master@{#449895} Committed: https://chromium.googlesource.com/chromium/src/+/1a8d5f3805dbae5242cf7dce14df... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:240001) as https://chromium.googlesource.com/chromium/src/+/1a8d5f3805dbae5242cf7dce14df...
Message was sent while issue was closed.
On 2017/02/12 20:14:09, commit-bot: I haz the power wrote: > Committed patchset #9 (id:240001) as > https://chromium.googlesource.com/chromium/src/+/1a8d5f3805dbae5242cf7dce14df... According to https://findit-for-me.appspot.com/waterfall/flake?master_name=tryserver.chrom..., this may be the culprit why FileSystemProviderApiTest.ReadFile has become flaky. Since I have not idea what this patch is doing, I'll leave it up to the owners to see if the analysis makes sense and revert this patch if needed.
Message was sent while issue was closed.
On 2017/02/13 14:55:19, Sergiy Byelozyorov wrote: > On 2017/02/12 20:14:09, commit-bot: I haz the power wrote: > > Committed patchset #9 (id:240001) as > > > https://chromium.googlesource.com/chromium/src/+/1a8d5f3805dbae5242cf7dce14df... > > According to > https://findit-for-me.appspot.com/waterfall/flake?master_name=tryserver.chrom..., > this may be the culprit why FileSystemProviderApiTest.ReadFile has become flaky. > Since I have not idea what this patch is doing, I'll leave it up to the owners > to see if the analysis makes sense and revert this patch if needed. Looks like the test has been disabled in https://bugs.chromium.org/p/chromium/issues/detail?id=691449, so you may need to re-enable the test afterwards.
Message was sent while issue was closed.
On 2017/02/13 15:55:01, Sergiy Byelozyorov wrote: > On 2017/02/13 14:55:19, Sergiy Byelozyorov wrote: > > On 2017/02/12 20:14:09, commit-bot: I haz the power wrote: > > > Committed patchset #9 (id:240001) as > > > > > > https://chromium.googlesource.com/chromium/src/+/1a8d5f3805dbae5242cf7dce14df... > > > > According to > > > https://findit-for-me.appspot.com/waterfall/flake?master_name=tryserver.chrom..., > > this may be the culprit why FileSystemProviderApiTest.ReadFile has become > flaky. > > Since I have not idea what this patch is doing, I'll leave it up to the owners > > to see if the analysis makes sense and revert this patch if needed. > > Looks like the test has been disabled in > https://bugs.chromium.org/p/chromium/issues/detail?id=691449, so you may need to > re-enable the test afterwards. Sergiy: This patch just delays starting some requests for a bit. I'm guessing that the delay is revealing a race in your test. The error of the test is: [31214:31214:0213/042805.046761:INFO:CONSOLE(0)] "[FAIL] readFileWithOpenedFilesLimitSuccess: API Test Error in readFileWithOpenedFilesLimitSuccess Actual: basket fullI have a of fruits. Expected: I have a basket full of fruits. Which definitely looks racy.
Message was sent while issue was closed.
On 2017/02/13 19:23:18, jkarlin wrote: > On 2017/02/13 15:55:01, Sergiy Byelozyorov wrote: > > On 2017/02/13 14:55:19, Sergiy Byelozyorov wrote: > > > On 2017/02/12 20:14:09, commit-bot: I haz the power wrote: > > > > Committed patchset #9 (id:240001) as > > > > > > > > > > https://chromium.googlesource.com/chromium/src/+/1a8d5f3805dbae5242cf7dce14df... > > > > > > According to > > > > > > https://findit-for-me.appspot.com/waterfall/flake?master_name=tryserver.chrom..., > > > this may be the culprit why FileSystemProviderApiTest.ReadFile has become > > flaky. > > > Since I have not idea what this patch is doing, I'll leave it up to the > owners > > > to see if the analysis makes sense and revert this patch if needed. > > > > Looks like the test has been disabled in > > https://bugs.chromium.org/p/chromium/issues/detail?id=691449, so you may need > to > > re-enable the test afterwards. > > Sergiy: This patch just delays starting some requests for a bit. I'm guessing > that the delay is revealing a race in your test. > > The error of the test is: > [31214:31214:0213/042805.046761:INFO:CONSOLE(0)] "[FAIL] > readFileWithOpenedFilesLimitSuccess: API Test Error in > readFileWithOpenedFilesLimitSuccess > Actual: basket fullI have a of fruits. > Expected: I have a basket full of fruits. > > Which definitely looks racy. It's not my test, I am just working on flakiness tooling and this was an arbitrary flaky test that I picked up to run FindIt on it. Looks like FindIt found the correct CL making it flaky, but it's not the culprit CL. I've provided feedback to the FindIt team using Feedback UI on the page for this test. Thanks for looking into it.
Message was sent while issue was closed.
On 2017/02/16 14:10:54, Sergiy Byelozyorov wrote: > On 2017/02/13 19:23:18, jkarlin wrote: > > On 2017/02/13 15:55:01, Sergiy Byelozyorov wrote: > > > On 2017/02/13 14:55:19, Sergiy Byelozyorov wrote: > > > > On 2017/02/12 20:14:09, commit-bot: I haz the power wrote: > > > > > Committed patchset #9 (id:240001) as > > > > > > > > > > > > > > > https://chromium.googlesource.com/chromium/src/+/1a8d5f3805dbae5242cf7dce14df... > > > > > > > > According to > > > > > > > > > > https://findit-for-me.appspot.com/waterfall/flake?master_name=tryserver.chrom..., > > > > this may be the culprit why FileSystemProviderApiTest.ReadFile has become > > > flaky. > > > > Since I have not idea what this patch is doing, I'll leave it up to the > > owners > > > > to see if the analysis makes sense and revert this patch if needed. > > > > > > Looks like the test has been disabled in > > > https://bugs.chromium.org/p/chromium/issues/detail?id=691449, so you may > need > > to > > > re-enable the test afterwards. > > > > Sergiy: This patch just delays starting some requests for a bit. I'm guessing > > that the delay is revealing a race in your test. > > > > The error of the test is: > > [31214:31214:0213/042805.046761:INFO:CONSOLE(0)] "[FAIL] > > readFileWithOpenedFilesLimitSuccess: API Test Error in > > readFileWithOpenedFilesLimitSuccess > > Actual: basket fullI have a of fruits. > > Expected: I have a basket full of fruits. > > > > Which definitely looks racy. > > It's not my test, I am just working on flakiness tooling and this was an > arbitrary flaky test that I picked up to run FindIt on it. Looks like FindIt > found the correct CL making it flaky, but it's not the culprit CL. I've provided > feedback to the FindIt team using Feedback UI on the page for this test. Thanks > for looking into it. I'm looking into it. The flakiness looks related to this CL, and it's a very bad one. We're corrupting files exposed via File System Provider API.
Message was sent while issue was closed.
mtomasz@chromium.org changed reviewers: + mtomasz@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2682163002/diff/240001/content/browser/loader... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/2682163002/diff/240001/content/browser/loader... content/browser/loader/resource_scheduler.cc:386: (!request->is_async() || !ShouldYieldScheduler())) { This doesn't seem right. It was guaranteed that non HTTP/HTTPS requests will never be delayed (https://cs.chromium.org/chromium/src/content/browser/loader/resource_schedule...). This CL releases this guarantee. As a result, blob requests became delayable, which caused data corruption for filed provided via File System Provider API. I think the logic here could be simpler. It used to be ShouldStartRequest which would contain the entire logic for deciding whether to start a request immediately or later. However, now part of the logic lays is moved to ShouldYieldScheduler. Can we move ShouldYieldScheduler to ShouldStartRequest while fixing the blob requests?
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:240001) has been created in https://codereview.chromium.org/2708323002/ by mtomasz@chromium.org. The reason for reverting is: Regresses blob requests making them delayable..
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:240001) has been created in https://codereview.chromium.org/2708173003/ by jkarlin@chromium.org. The reason for reverting is: Adds a check for http(s) before yielding the scheduler..
Message was sent while issue was closed.
On 2017/02/22 15:12:47, jkarlin wrote: > A revert of this CL (patchset #9 id:240001) has been created in > https://codereview.chromium.org/2708173003/ by mailto:jkarlin@chromium.org. > > The reason for reverting is: Adds a check for http(s) before yielding the > scheduler.. Bah, I meant to revert the revert, not this CL. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
