Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1053153003:
Remove swap ack throttling to regain performance regression on freon (Closed)
|
Created:
5 years, 8 months ago by alexst (slow to review) Modified:
5 years, 8 months ago CC:
chromium-reviews, cc-bugs_chromium.org, scheduler-bugs_chromium.org, Sami Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove swap ack throttling to regain performance regression on freon.
In the past eglSwapBuffers used to return early, before the swap actually happened and descheduling in the driver. This resulted in the scheduler beginning the next frame early and hitting the page flip deadline in time more often.
Freon returns swap ack when page flip actually happened, but with the side effect of scheduling shifted forward, resulting in more missed vblanks and reduced performance.
This change restores original performance on freon.
BUG=465599
Committed: https://crrev.com/faa924ff38e80006a27ac85fcc95f3dcbe441d55
Cr-Commit-Position: refs/heads/master@{#324465}
Patch Set 1 #
Total comments: 2
Patch Set 2 : #Patch Set 3 : #
Total comments: 10
Patch Set 4 : unittests #
Messages
Total messages: 28 (10 generated)
alexst@chromium.org changed reviewers: + brianderson@chromium.org
danakj@chromium.org changed reviewers: + danakj@chromium.org
https://codereview.chromium.org/1053153003/diff/1/cc/scheduler/scheduler_stat... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/1053153003/diff/1/cc/scheduler/scheduler_stat... cc/scheduler/scheduler_state_machine.cc:428: #if !defined(ARCH_CPU_ARM_FAMILY) This should be a LayerTreeSetting->SchedulerSetting set in the embedder. Please no platform #ifs inside cc/. Then we can not unit test the paths easily.
lgtm if this is turned on for all platforms https://codereview.chromium.org/1053153003/diff/1/cc/scheduler/scheduler_stat... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/1053153003/diff/1/cc/scheduler/scheduler_stat... cc/scheduler/scheduler_state_machine.cc:428: #if !defined(ARCH_CPU_ARM_FAMILY) On 2015/04/06 16:32:51, danakj wrote: > This should be a LayerTreeSetting->SchedulerSetting set in the embedder. Please > no platform #ifs inside cc/. Then we can not unit test the paths easily. Let's just enable this on all platforms and see if there's still any browser test flake. I can help you debug if there is. I'm pretty sure the flake is just because of poorly written tests, but on the off chance that it's not, I'd hate for only one class of devices to be broken. Enabling it on all devices will also give us some real feedback from the perf bots concerning tradeoffs. There's a theoretical concern that this will increase CPU fighting with the main thread. If it does, we may want to hold off on this and be smarter about when we throttle vs. when we don't.
Okay, new patch is up.
lgtm +Sami for fyi, since he had some concerns regarding CPU fighting if we were to enable this. We'll keep an eye on the perf bots.
The CQ bit was checked by alexst@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1053153003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Hi Brian, I need your guidance. In scheduler_state_machine_unittest.cc, the failing part seems to test the exact condition we just disabled. Since the code is gone, I removed the corresponding part of the test. In scheduler_unittest.cc, I annotated the failures, but I am not sure how to properly fix them. The subtleties of the interaction in the scheduler are a bit beyond my grasp.
https://codereview.chromium.org/1053153003/diff/40001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/1053153003/diff/40001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:1497: // but not a BeginMainFrame or draw. Update this comment. https://codereview.chromium.org/1053153003/diff/40001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:1520: // EXPECT_SINGLE_ACTION("ScheduledActionSendBeginMainFrame", client_); This action should be the 3rd one above and then EXPECT_NO_ACTION here is okay. https://codereview.chromium.org/1053153003/diff/40001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:1767: // but not a BeginMainFrame or draw. Ditto: update comment. https://codereview.chromium.org/1053153003/diff/40001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:1788: // EXPECT_SINGLE_ACTION("ScheduledActionSendBeginMainFrame", client_); Ditto: This action should be the 3rd one above and then EXPECT_NO_ACTION here is okay.
https://codereview.chromium.org/1053153003/diff/40001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/1053153003/diff/40001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:1520: // EXPECT_SINGLE_ACTION("ScheduledActionSendBeginMainFrame", client_); On 2015/04/09 02:01:37, brianderson wrote: > This action should be the 3rd one above and then EXPECT_NO_ACTION here is okay. Actually, I would expect this to be the 2nd action above since it's better to kick off the main thread earlier to increase parallelism. If it is the 3rd, open a separate bug and don't worry about it.
skyostil@chromium.org changed reviewers: + skyostil@chromium.org
I'm fine with going ahead with this. Let's just make sure doesn't make latency or jank significantly worse, particularly on the Android One bot.
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by alexst@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from brianderson@chromium.org Link to the patchset: https://codereview.chromium.org/1053153003/#ps80001 (title: "unittests")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1053153003/80001
thanks, PTAL https://codereview.chromium.org/1053153003/diff/40001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/1053153003/diff/40001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:1497: // but not a BeginMainFrame or draw. On 2015/04/09 02:01:37, brianderson wrote: > Update this comment. Done. https://codereview.chromium.org/1053153003/diff/40001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:1520: // EXPECT_SINGLE_ACTION("ScheduledActionSendBeginMainFrame", client_); On 2015/04/09 02:01:37, brianderson wrote: > This action should be the 3rd one above and then EXPECT_NO_ACTION here is okay. Done. https://codereview.chromium.org/1053153003/diff/40001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:1520: // EXPECT_SINGLE_ACTION("ScheduledActionSendBeginMainFrame", client_); On 2015/04/09 02:05:38, brianderson wrote: > On 2015/04/09 02:01:37, brianderson wrote: > > This action should be the 3rd one above and then EXPECT_NO_ACTION here is > okay. > > Actually, I would expect this to be the 2nd action above since it's better to > kick off the main thread earlier to increase parallelism. If it is the 3rd, open > a separate bug and don't worry about it. It's the 3rd action, I'll file a bug. https://codereview.chromium.org/1053153003/diff/40001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:1767: // but not a BeginMainFrame or draw. On 2015/04/09 02:01:37, brianderson wrote: > Ditto: update comment. Done. https://codereview.chromium.org/1053153003/diff/40001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:1788: // EXPECT_SINGLE_ACTION("ScheduledActionSendBeginMainFrame", client_); On 2015/04/09 02:01:37, brianderson wrote: > Ditto: This action should be the 3rd one above and then EXPECT_NO_ACTION here is > okay. Done.
The CQ bit was unchecked by commit-bot@chromium.org
This issue passed the CQ dry run.
lgtm
The CQ bit was checked by alexst@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1053153003/80001
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/faa924ff38e80006a27ac85fcc95f3dcbe441d55 Cr-Commit-Position: refs/heads/master@{#324465}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:80001) has been created in https://codereview.chromium.org/1087983003/ by brianderson@chromium.org. The reason for reverting is: Despite throughput wins, this increased hang rates, broke pdfs, and regressed latency.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
