Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2711633003:
Reland of [ResourceScheduler] Yield after starting several requests (Closed)
DescriptionReland of [ResourceScheduler] Yield after starting several requests (patchset
#1 id:1 of https://codereview.chromium.org/2708323002/ )
The original patch was reverted because it delayed non-http(s) requests. This reland fixes that issue.
Original issue's description:
> Revert of [ResourceScheduler] Yield after starting several requests (patchset #9 id:240001 of https://codereview.chromium.org/2682163002/ )
>
> Reason for revert:
> Regresses blob requests making them delayable.
>
> Original issue's 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
>
> TBR=csharrison@chromium.org,isherman@chromium.org,jkarlin@chromium.org
> # Not skipping CQ checks because original CL landed more than 1 days ago.
> BUG=655585
>
> Review-Url: https://codereview.chromium.org/2708323002
> Cr-Commit-Position: refs/heads/master@{#451946}
> Committed: https://chromium.googlesource.com/chromium/src/+/a9757ebf44c92e009d075322ad35f008b0bfa1ad
BUG=655585
Review-Url: https://codereview.chromium.org/2711633003
Cr-Commit-Position: refs/heads/master@{#452216}
Committed: https://chromium.googlesource.com/chromium/src/+/bf88dc8ed93ded5866c4c2965803ae24d89050b6
Patch Set 1 #Patch Set 2 : Add test, see that it breaks without fix #Patch Set 3 : Fix, and now tests pass #
Total comments: 10
Patch Set 4 : Address comments from PS3 #
Depends on Patchset: Messages
Total messages: 39 (25 generated)
Created Reland of [ResourceScheduler] Yield after starting several requests
Description was changed from ========== Reland of [ResourceScheduler] Yield after starting several requests (patchset #1 id:1 of https://codereview.chromium.org/2708323002/ ) Reason for revert: Adds a check for http(s) before yielding the scheduler. Original issue's description: > Revert of [ResourceScheduler] Yield after starting several requests (patchset #9 id:240001 of https://codereview.chromium.org/2682163002/ ) > > Reason for revert: > Regresses blob requests making them delayable. > > Original issue's 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/+/1a8d5f3805dbae5242cf7dce14df... > > TBR=csharrison@chromium.org,isherman@chromium.org,jkarlin@chromium.org > # Not skipping CQ checks because original CL landed more than 1 days ago. > BUG=655585 > > Review-Url: https://codereview.chromium.org/2708323002 > Cr-Commit-Position: refs/heads/master@{#451946} > Committed: https://chromium.googlesource.com/chromium/src/+/a9757ebf44c92e009d075322ad35... TBR=csharrison@chromium.org,isherman@chromium.org,mtomasz@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=655585 ========== to ========== Reland of [ResourceScheduler] Yield after starting several requests (patchset #1 id:1 of https://codereview.chromium.org/2708323002/ ) The original patch was reverted because it delayed non-http(s) requests. This reland fixes that issue. Original issue's description: > Revert of [ResourceScheduler] Yield after starting several requests (patchset #9 id:240001 of https://codereview.chromium.org/2682163002/ ) > > Reason for revert: > Regresses blob requests making them delayable. > > Original issue's 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/+/1a8d5f3805dbae5242cf7dce14df... > > TBR=csharrison@chromium.org,isherman@chromium.org,jkarlin@chromium.org > # Not skipping CQ checks because original CL landed more than 1 days ago. > BUG=655585 > > Review-Url: https://codereview.chromium.org/2708323002 > Cr-Commit-Position: refs/heads/master@{#451946} > Committed: https://chromium.googlesource.com/chromium/src/+/a9757ebf44c92e009d075322ad35... BUG=655585 ==========
jkarlin@chromium.org changed reviewers: - csharrison@chromium.org, isherman@chromium.org, mtomasz@chromium.org
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: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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...
Description was changed from ========== Reland of [ResourceScheduler] Yield after starting several requests (patchset #1 id:1 of https://codereview.chromium.org/2708323002/ ) The original patch was reverted because it delayed non-http(s) requests. This reland fixes that issue. Original issue's description: > Revert of [ResourceScheduler] Yield after starting several requests (patchset #9 id:240001 of https://codereview.chromium.org/2682163002/ ) > > Reason for revert: > Regresses blob requests making them delayable. > > Original issue's 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/+/1a8d5f3805dbae5242cf7dce14df... > > TBR=csharrison@chromium.org,isherman@chromium.org,jkarlin@chromium.org > # Not skipping CQ checks because original CL landed more than 1 days ago. > BUG=655585 > > Review-Url: https://codereview.chromium.org/2708323002 > Cr-Commit-Position: refs/heads/master@{#451946} > Committed: https://chromium.googlesource.com/chromium/src/+/a9757ebf44c92e009d075322ad35... BUG=655585 ========== to ========== Reland of [ResourceScheduler] Yield after starting several requests (patchset #1 id:1 of https://codereview.chromium.org/2708323002/ ) The original patch was reverted because it delayed non-http(s) requests. This reland fixes that issue. Original issue's description: > Revert of [ResourceScheduler] Yield after starting several requests (patchset #9 id:240001 of https://codereview.chromium.org/2682163002/ ) > > Reason for revert: > Regresses blob requests making them delayable. > > Original issue's 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/+/1a8d5f3805dbae5242cf7dce14df... > > TBR=csharrison@chromium.org,isherman@chromium.org,jkarlin@chromium.org > # Not skipping CQ checks because original CL landed more than 1 days ago. > BUG=655585 > > Review-Url: https://codereview.chromium.org/2708323002 > Cr-Commit-Position: refs/heads/master@{#451946} > Committed: https://chromium.googlesource.com/chromium/src/+/a9757ebf44c92e009d075322ad35... BUG=655585 ==========
jkarlin@chromium.org changed reviewers: + csharrison@chromium.org, isherman@chromium.org
csharrison@chromium.org: PTAL. The logic is a bit different now, it's in ShouldStartRequest. isherman@chromium.org: Please review changes in testing/variations/ Thanks!
On 2017/02/22 16:41:21, jkarlin wrote: > isherman@chromium.org: Please review changes in testing/variations/ Did anything change in testing/variations?
On 2017/02/22 16:44:01, Ilya Sherman wrote: > On 2017/02/22 16:41:21, jkarlin wrote: > > mailto:isherman@chromium.org: Please review changes in testing/variations/ > > Did anything change in testing/variations? Nope.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lg mostly nits https://codereview.chromium.org/2711633003/diff/140001/content/browser/loader... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/2711633003/diff/140001/content/browser/loader... content/browser/loader/resource_scheduler.cc:777: void ResumeAfterYielding() { nit: I think a better name is "ResumeIfYielded" https://codereview.chromium.org/2711633003/diff/140001/content/browser/loader... content/browser/loader/resource_scheduler.cc:790: if (!yielding_scheduler_enabled_) nit: merge this if statement into the below one: if (yielding_scheduler_enabled_ && started_requests_since_yielding_ >= max_requests_before_yielding_)... https://codereview.chromium.org/2711633003/diff/140001/content/browser/loader... content/browser/loader/resource_scheduler.cc:863: bool did_scheduler_yield_; Do we really need this member? Can't we just use started_requests_since_yielding_ >= max_requests_before_yielding_? https://codereview.chromium.org/2711633003/diff/140001/content/browser/loader... File content/browser/loader/resource_scheduler_unittest.cc (right): https://codereview.chromium.org/2711633003/diff/140001/content/browser/loader... content/browser/loader/resource_scheduler_unittest.cc:384: TEST_F(ResourceSchedulerTest, SchedulerDoesNotYieldForAlternativeSchemas) { nit: I think the plural is "Schemes"?
The CQ bit was checked by jkarlin@chromium.org to run a CQ dry run
Addressed comments. PTAL, thanks! https://codereview.chromium.org/2711633003/diff/140001/content/browser/loader... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/2711633003/diff/140001/content/browser/loader... content/browser/loader/resource_scheduler.cc:777: void ResumeAfterYielding() { On 2017/02/22 17:42:14, Charlie Harrison wrote: > nit: I think a better name is "ResumeIfYielded" Done. https://codereview.chromium.org/2711633003/diff/140001/content/browser/loader... content/browser/loader/resource_scheduler.cc:790: if (!yielding_scheduler_enabled_) On 2017/02/22 17:42:14, Charlie Harrison wrote: > nit: merge this if statement into the below one: > > if (yielding_scheduler_enabled_ && started_requests_since_yielding_ >= > max_requests_before_yielding_)... Done. https://codereview.chromium.org/2711633003/diff/140001/content/browser/loader... content/browser/loader/resource_scheduler.cc:863: bool did_scheduler_yield_; On 2017/02/22 17:42:14, Charlie Harrison wrote: > Do we really need this member? Can't we just use > started_requests_since_yielding_ >= max_requests_before_yielding_? We need it. started_requests_since_yielding_ won't increment for yielded requests (but will increment for unyieldable requests, e.g., synchronous). https://codereview.chromium.org/2711633003/diff/140001/content/browser/loader... File content/browser/loader/resource_scheduler_unittest.cc (right): https://codereview.chromium.org/2711633003/diff/140001/content/browser/loader... content/browser/loader/resource_scheduler_unittest.cc:384: TEST_F(ResourceSchedulerTest, SchedulerDoesNotYieldForAlternativeSchemas) { On 2017/02/22 17:42:14, Charlie Harrison wrote: > nit: I think the plural is "Schemes"? Thanks!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM
https://codereview.chromium.org/2711633003/diff/140001/content/browser/loader... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/2711633003/diff/140001/content/browser/loader... content/browser/loader/resource_scheduler.cc:863: bool did_scheduler_yield_; On 2017/02/22 17:49:25, jkarlin wrote: > On 2017/02/22 17:42:14, Charlie Harrison wrote: > > Do we really need this member? Can't we just use > > started_requests_since_yielding_ >= max_requests_before_yielding_? > > We need it. started_requests_since_yielding_ won't increment for yielded > requests (but will increment for unyieldable requests, e.g., synchronous). Ah, what you said was a little more subtle. If we use >= then we can't be sure if we yielded or not and we'd rename this "scheduler_possibly_yielded". We'd sometimes run LoadAnyStartablePendingRequests extra times but we wouldn't need to keep track of did_scheduler_yield_. WDYT?
https://codereview.chromium.org/2711633003/diff/140001/content/browser/loader... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/2711633003/diff/140001/content/browser/loader... content/browser/loader/resource_scheduler.cc:863: bool did_scheduler_yield_; On 2017/02/22 17:58:37, jkarlin wrote: > On 2017/02/22 17:49:25, jkarlin wrote: > > On 2017/02/22 17:42:14, Charlie Harrison wrote: > > > Do we really need this member? Can't we just use > > > started_requests_since_yielding_ >= max_requests_before_yielding_? > > > > We need it. started_requests_since_yielding_ won't increment for yielded > > requests (but will increment for unyieldable requests, e.g., synchronous). > > Ah, what you said was a little more subtle. If we use >= then we can't be sure > if we yielded or not and we'd rename this "scheduler_possibly_yielded". We'd > sometimes run LoadAnyStartablePendingRequests extra times but we wouldn't need > to keep track of did_scheduler_yield_. > > WDYT? I thought about that but it seemed weird and hard to understand. I'm happy with the bool.
isherman@: PTAL at testing/variations/. Nothing changed since your approval pre-revert. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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.
On 2017/02/22 18:04:22, jkarlin wrote: > isherman@: PTAL at testing/variations/. Nothing changed since your approval > pre-revert. Thanks! LGTM, and feel free to TBR unchanged and previously reviewed files in the future.
The CQ bit was checked by jkarlin@chromium.org
On 2017/02/22 20:22:29, Ilya Sherman wrote: > On 2017/02/22 18:04:22, jkarlin wrote: > > isherman@: PTAL at testing/variations/. Nothing changed since your approval > > pre-revert. Thanks! > > LGTM, and feel free to TBR unchanged and previously reviewed files in the > future. Got it, thanks!
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": 160001, "attempt_start_ts": 1487798787542180,
"parent_rev": "194bb15b9740165e8ca0ec4a4ee898887876946a", "commit_rev":
"bf88dc8ed93ded5866c4c2965803ae24d89050b6"}
Message was sent while issue was closed.
Description was changed from ========== Reland of [ResourceScheduler] Yield after starting several requests (patchset #1 id:1 of https://codereview.chromium.org/2708323002/ ) The original patch was reverted because it delayed non-http(s) requests. This reland fixes that issue. Original issue's description: > Revert of [ResourceScheduler] Yield after starting several requests (patchset #9 id:240001 of https://codereview.chromium.org/2682163002/ ) > > Reason for revert: > Regresses blob requests making them delayable. > > Original issue's 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/+/1a8d5f3805dbae5242cf7dce14df... > > TBR=csharrison@chromium.org,isherman@chromium.org,jkarlin@chromium.org > # Not skipping CQ checks because original CL landed more than 1 days ago. > BUG=655585 > > Review-Url: https://codereview.chromium.org/2708323002 > Cr-Commit-Position: refs/heads/master@{#451946} > Committed: https://chromium.googlesource.com/chromium/src/+/a9757ebf44c92e009d075322ad35... BUG=655585 ========== to ========== Reland of [ResourceScheduler] Yield after starting several requests (patchset #1 id:1 of https://codereview.chromium.org/2708323002/ ) The original patch was reverted because it delayed non-http(s) requests. This reland fixes that issue. Original issue's description: > Revert of [ResourceScheduler] Yield after starting several requests (patchset #9 id:240001 of https://codereview.chromium.org/2682163002/ ) > > Reason for revert: > Regresses blob requests making them delayable. > > Original issue's 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/+/1a8d5f3805dbae5242cf7dce14df... > > TBR=csharrison@chromium.org,isherman@chromium.org,jkarlin@chromium.org > # Not skipping CQ checks because original CL landed more than 1 days ago. > BUG=655585 > > Review-Url: https://codereview.chromium.org/2708323002 > Cr-Commit-Position: refs/heads/master@{#451946} > Committed: https://chromium.googlesource.com/chromium/src/+/a9757ebf44c92e009d075322ad35... BUG=655585 Review-Url: https://codereview.chromium.org/2711633003 Cr-Commit-Position: refs/heads/master@{#452216} Committed: https://chromium.googlesource.com/chromium/src/+/bf88dc8ed93ded5866c4c2965803... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:160001) as https://chromium.googlesource.com/chromium/src/+/bf88dc8ed93ded5866c4c2965803... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
