Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2932503003:
Add ash perftest on background blur filter. (Closed)
DescriptionAdd ash perftest on background blur filter.
Add an ash perftest to evaluate the background blur filter efficiency.
BUG=730841, 646062, 682082
TEST=perftest
Review-Url: https://codereview.chromium.org/2932503003
Cr-Commit-Position: refs/heads/master@{#479797}
Committed: https://chromium.googlesource.com/chromium/src/+/5c36dc747da0feb63a206edaf0dfc470471ade20
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add CompositorObserver::OnCompositingBeginMainFrame and fixed DEPS. #
Total comments: 16
Patch Set 3 : Fix for patch 2. #
Total comments: 10
Patch Set 4 : Fix nits in patch 3. #
Total comments: 8
Patch Set 5 : Fix nits in patch 4. #
Messages
Total messages: 48 (31 generated)
wutao@chromium.org changed reviewers: + oshima@chromium.org, reveman@chromium.org
Hi reveman@,
I wrote some perftest, please verify they are correct.
ui::DrawWaiterForTest::WaitForCompositingEnded(compositor_) can make sure the
last layer changes are processed, and we can continue to request drawing new
frame.
I attached some test results below.
Hi oshima@,
First time to add perftest to ash, please review the file location and DEPS are
correct.
Thanks,
Tao
[==========] Running 8 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 8 tests from AshBackgroundFilterBlurPerfTest
[ RUN ] AshBackgroundFilterBlurPerfTest.NoBlurBackgroundLayerBoundsChange
*RESULT AshBackgroundFilterBlurPerfTest: no_blur_background_layer_bounds_change=
161.35330200195312 runs/s
[ OK ] AshBackgroundFilterBlurPerfTest.NoBlurBackgroundLayerBoundsChange
(3000 ms)
[ RUN ] AshBackgroundFilterBlurPerfTest.NoBlurBlurLayerBoundsChange
*RESULT AshBackgroundFilterBlurPerfTest: no_blur_blur_layer_bounds_change=
123.61016082763672 runs/s
[ OK ] AshBackgroundFilterBlurPerfTest.NoBlurBlurLayerBoundsChange (2345
ms)
[ RUN ] AshBackgroundFilterBlurPerfTest.BackgroundLayerBoundsChange
*RESULT AshBackgroundFilterBlurPerfTest: background_layer_bounds_change=
54.98154830932617 runs/s
[ OK ] AshBackgroundFilterBlurPerfTest.BackgroundLayerBoundsChange (5340
ms)
[ RUN ] AshBackgroundFilterBlurPerfTest.BlurLayerBoundsChange
*RESULT AshBackgroundFilterBlurPerfTest: blur_layer_bounds_change=
61.29020690917969 runs/s
[ OK ] AshBackgroundFilterBlurPerfTest.BlurLayerBoundsChange (3732 ms)
[ RUN ] AshBackgroundFilterBlurPerfTest.NoBlurBackgroundLayerOpacityChange
Still waiting for the following processes to finish:
out/Debug/ash_perftests --gtest_filter=AshBackgroundFilterBlurPerfTest.*
--gtest_flagfile=/tmp/.com.google.Chrome.fHGKZT/.com.google.Chrome.hsfpnC
--single-process-tests --test-launcher-output=/tmp/.com.
google.Chrome.BPM6Bb/test_results.xml
*RESULT AshBackgroundFilterBlurPerfTest:
no_blur_background_layer_opacity_change= 96.80279541015625 runs/s
[ OK ] AshBackgroundFilterBlurPerfTest.NoBlurBackgroundLayerOpacityChange
(2344 ms)
[ RUN ] AshBackgroundFilterBlurPerfTest.NoBlurBlurLayerOpacityChange
*RESULT AshBackgroundFilterBlurPerfTest: no_blur_blur_layer_opacity_change=
85.03849792480469 runs/s
[ OK ] AshBackgroundFilterBlurPerfTest.NoBlurBlurLayerOpacityChange (2306
ms)
[ RUN ] AshBackgroundFilterBlurPerfTest.BackgroundLayerOpacityChange
*RESULT AshBackgroundFilterBlurPerfTest: background_layer_opacity_change=
65.5414047241211 runs/s
[ OK ] AshBackgroundFilterBlurPerfTest.BackgroundLayerOpacityChange (4280
ms)
[ RUN ] AshBackgroundFilterBlurPerfTest.BlurLayerOpacityChange
*RESULT AshBackgroundFilterBlurPerfTest: blur_layer_opacity_change=
60.00715255737305 runs/s
[ OK ] AshBackgroundFilterBlurPerfTest.BlurLayerOpacityChange (3939 ms)
[----------] 8 tests from AshBackgroundFilterBlurPerfTest (27290 ms total)
The CQ bit was checked by wutao@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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The tests failed on dependency check. I will fix in the next patch.
this looks great! just one question/comment. https://codereview.chromium.org/2932503003/diff/1/ash/perftests/ash_backgroun... File ash/perftests/ash_background_filter_blur_perftest.cc (right): https://codereview.chromium.org/2932503003/diff/1/ash/perftests/ash_backgroun... ash/perftests/ash_background_filter_blur_perftest.cc:79: ui::DrawWaiterForTest::WaitForCompositingEnded(compositor_); Can we just wait for a begin frame callback here and only do the WaitForCompositingEnded at the end? I think that synchronizing after each frame is not going to give us results that match real usage.
Description was changed from ========== Add ash perftest on background blur filter. Add an ash perftest to evaluate the background blur filter efficiency. BUG=730841,646062 TEST=perftest ========== to ========== Add ash perftest on background blur filter. Add an ash perftest to evaluate the background blur filter efficiency. BUG=730841,646062,682082 TEST=perftest ==========
wutao@chromium.org changed reviewers: + danakj@chromium.org
Hi reveman@, PTAL. Hi oshima@, please take a look of the files under ash/. Hi danakj@, please take a look of the files under compositor/. Thank you, Tao https://codereview.chromium.org/2932503003/diff/1/ash/perftests/ash_backgroun... File ash/perftests/ash_background_filter_blur_perftest.cc (right): https://codereview.chromium.org/2932503003/diff/1/ash/perftests/ash_backgroun... ash/perftests/ash_background_filter_blur_perftest.cc:79: ui::DrawWaiterForTest::WaitForCompositingEnded(compositor_); On 2017/06/12 20:13:17, reveman wrote: > Can we just wait for a begin frame callback here and only do the > WaitForCompositingEnded at the end? I think that synchronizing after each frame > is not going to give us results that match real usage. sg. I added a function OnCompositingBeginMainFrame to CompositorObserver.
The CQ bit was checked by wutao@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.
https://codereview.chromium.org/2932503003/diff/20001/ash/perftests/ash_backg... File ash/perftests/ash_background_filter_blur_perftest.cc (right): https://codereview.chromium.org/2932503003/diff/20001/ash/perftests/ash_backg... ash/perftests/ash_background_filter_blur_perftest.cc:78: compositor_->ScheduleFullRedraw(); Why this? https://codereview.chromium.org/2932503003/diff/20001/ash/perftests/ash_backg... ash/perftests/ash_background_filter_blur_perftest.cc:79: ui::DrawWaiterForTest::WaitForBeginMainFrame(compositor_); Why not WaitForCompositingDidCommit?
https://codereview.chromium.org/2932503003/diff/20001/ash/perftests/ash_backg... File ash/perftests/ash_background_filter_blur_perftest.cc (right): https://codereview.chromium.org/2932503003/diff/20001/ash/perftests/ash_backg... ash/perftests/ash_background_filter_blur_perftest.cc:23: std::unique_ptr<ui::Layer> CreateSolidColorLayer(SkColor color); new line https://codereview.chromium.org/2932503003/diff/20001/ash/perftests/ash_backg... ash/perftests/ash_background_filter_blur_perftest.cc:36: ui::Layer* root_layer_; initialize with nullptr as they're not initialized in ctor. https://codereview.chromium.org/2932503003/diff/20001/ash/perftests/ash_backg... ash/perftests/ash_background_filter_blur_perftest.cc:48: UpdateDisplay("800x600"); add comment that this is for consistency (even if the default size changed) https://codereview.chromium.org/2932503003/diff/20001/ash/perftests/ash_backg... ash/perftests/ash_background_filter_blur_perftest.cc:72: for (int i = 0; i < num_iteration; ++i) { shouldn't this start with 1, or you want to test empty bounds as well?
Hi danaki@, please look my reply and let me know what do you think. Another weird thing is that when I change the display size, there is no big difference to the tests results (speed) at my desktop env. Isn't the performance proportional to the pixels number? Thank you, Tao https://codereview.chromium.org/2932503003/diff/20001/ash/perftests/ash_backg... File ash/perftests/ash_background_filter_blur_perftest.cc (right): https://codereview.chromium.org/2932503003/diff/20001/ash/perftests/ash_backg... ash/perftests/ash_background_filter_blur_perftest.cc:23: std::unique_ptr<ui::Layer> CreateSolidColorLayer(SkColor color); On 2017/06/13 17:24:07, oshima wrote: > new line Will do. https://codereview.chromium.org/2932503003/diff/20001/ash/perftests/ash_backg... ash/perftests/ash_background_filter_blur_perftest.cc:36: ui::Layer* root_layer_; On 2017/06/13 17:24:07, oshima wrote: > initialize with nullptr as they're not initialized in ctor. Will do. https://codereview.chromium.org/2932503003/diff/20001/ash/perftests/ash_backg... ash/perftests/ash_background_filter_blur_perftest.cc:48: UpdateDisplay("800x600"); On 2017/06/13 17:24:07, oshima wrote: > add comment that this is for consistency (even if the default size changed) Will do. But weird thing that when I change the display size, there is no big difference to the tests results. Isn't the performance proportional to the pixels number? https://codereview.chromium.org/2932503003/diff/20001/ash/perftests/ash_backg... ash/perftests/ash_background_filter_blur_perftest.cc:72: for (int i = 0; i < num_iteration; ++i) { On 2017/06/13 17:24:07, oshima wrote: > shouldn't this start with 1, or you want to test empty bounds as well? Good catch! https://codereview.chromium.org/2932503003/diff/20001/ash/perftests/ash_backg... ash/perftests/ash_background_filter_blur_perftest.cc:78: compositor_->ScheduleFullRedraw(); On 2017/06/13 15:57:42, danakj wrote: > Why this? Here I want to calculate the time with fresh setup. The damaged area could depend on how we setup the tests, which might affect the calculation time. Therefore I mark the whole viewport to damage and needs redraw. But there are other things also "cached" or reused. e.g. the property nodes and render surface. We will not create them multiple times. Which means we are still not measuring the time with fresh setup. So this sounds ScheduleFullRedraw() is not necessary. WDYT? https://codereview.chromium.org/2932503003/diff/20001/ash/perftests/ash_backg... ash/perftests/ash_background_filter_blur_perftest.cc:79: ui::DrawWaiterForTest::WaitForBeginMainFrame(compositor_); On 2017/06/13 15:57:42, danakj wrote: > Why not WaitForCompositingDidCommit? I found there is no implementation of compositor->BeginMainFrame. What is the reason? By adding this will make the performance worse? I think there is slightly difference between BeginMainFrame and DidCommit in this test: If we waiting for DidCommit, then the layer changes in the current loop will be effective at this point? In addition, which means we can start to prepare the layer changes in the next loop while the display compositor is working? Corner case for the first loop, it might include some earlier changes before entering the loop? If we waiting for BeginMainFrame, is the same thing all the layer changes in the current loop will be processed? (changes happened before DoBeginMainFrame() should be updated in LayerTreeHost::UpdateLayers() in single_thread_proxy?). Then this will work too in this test case? Anyway, we might need to WaitForBeginMainFrame before entering the loops to clear state and WaitForCompositingEnded after all the loops to wait the last change finish. And in each loop, we WaitForCompositingDidCommit? If we do not want to add new function to CompositorObserver, then WaitForCompositingDidCommit should work as well. WDYT?
https://codereview.chromium.org/2932503003/diff/20001/ash/perftests/ash_backg... File ash/perftests/ash_background_filter_blur_perftest.cc (right): https://codereview.chromium.org/2932503003/diff/20001/ash/perftests/ash_backg... ash/perftests/ash_background_filter_blur_perftest.cc:78: compositor_->ScheduleFullRedraw(); On 2017/06/13 19:32:49, wutao wrote: > On 2017/06/13 15:57:42, danakj wrote: > > Why this? > > Here I want to calculate the time with fresh setup. The damaged area could > depend on how we setup the tests, which might affect the calculation time. > Therefore I mark the whole viewport to damage and needs redraw. > > But there are other things also "cached" or reused. e.g. the property nodes and > render surface. We will not create them multiple times. Which means we are still > not measuring the time with fresh setup. > > So this sounds ScheduleFullRedraw() is not necessary. WDYT? I think you want to measure the time of the blur, so you want as little else to happen inside the loop as possible. So I would just resize the blur layer, forcing it to be redrawn, and nothing else. https://codereview.chromium.org/2932503003/diff/20001/ash/perftests/ash_backg... ash/perftests/ash_background_filter_blur_perftest.cc:79: ui::DrawWaiterForTest::WaitForBeginMainFrame(compositor_); On 2017/06/13 19:32:49, wutao wrote: > On 2017/06/13 15:57:42, danakj wrote: > > Why not WaitForCompositingDidCommit? > > I found there is no implementation of compositor->BeginMainFrame. What is the > reason? By adding this will make the performance worse? > > I think there is slightly difference between BeginMainFrame and DidCommit in > this test: > If we waiting for DidCommit, then the layer changes in the current loop will be > effective at this point? Once they are committed, we will have to draw the changes before we commit anything new. So if you wait for commit then change them, it will request another commit once the draw is done, which is what you want. > In addition, which means we can start to prepare the > layer changes in the next loop while the display compositor is working? The UI compositor will not make another frame until the display compositor has acked that it is drawing. > Corner case for the first loop, it might include some earlier changes before > entering the loop? I'm not sure what you mean here. > If we waiting for BeginMainFrame, is the same thing all the layer changes in the > current loop will be processed? (changes happened before DoBeginMainFrame() > should be updated in LayerTreeHost::UpdateLayers() in single_thread_proxy?). Changes made in BeginMainFrame will be part of the next commit, just as changes made after the last commit (DidCommit) and before BeginMainFrame. > Then this will work too in this test case? > > Anyway, we might need to WaitForBeginMainFrame before entering the loops to > clear state and WaitForCompositingEnded after all the loops to wait the last > change finish. What if you do some warmup runs before you start measuring, or wait for a DidCommit before you start the loop, and don't measure the last iteration of the loop (ie don't WaitForCompositingEnded)? > And in each loop, we WaitForCompositingDidCommit? > > If we do not want to add new function to CompositorObserver, then > WaitForCompositingDidCommit should work as well. I prefer to not add it if we don't need it.
Hi danakj@ and oshima@, PTAL. I reverted code to add new function in CompositorObserver and only use WaitForCommit in the tests. Thanks, Tao https://codereview.chromium.org/2932503003/diff/20001/ash/perftests/ash_backg... File ash/perftests/ash_background_filter_blur_perftest.cc (right): https://codereview.chromium.org/2932503003/diff/20001/ash/perftests/ash_backg... ash/perftests/ash_background_filter_blur_perftest.cc:78: compositor_->ScheduleFullRedraw(); On 2017/06/13 19:47:43, danakj wrote: > On 2017/06/13 19:32:49, wutao wrote: > > On 2017/06/13 15:57:42, danakj wrote: > > > Why this? > > > > Here I want to calculate the time with fresh setup. The damaged area could > > depend on how we setup the tests, which might affect the calculation time. > > Therefore I mark the whole viewport to damage and needs redraw. > > > > But there are other things also "cached" or reused. e.g. the property nodes > and > > render surface. We will not create them multiple times. Which means we are > still > > not measuring the time with fresh setup. > > > > So this sounds ScheduleFullRedraw() is not necessary. WDYT? > > I think you want to measure the time of the blur, so you want as little else to > happen inside the loop as possible. So I would just resize the blur layer, > forcing it to be redrawn, and nothing else. Done. https://codereview.chromium.org/2932503003/diff/20001/ash/perftests/ash_backg... ash/perftests/ash_background_filter_blur_perftest.cc:79: ui::DrawWaiterForTest::WaitForBeginMainFrame(compositor_); On 2017/06/13 19:47:43, danakj wrote: > On 2017/06/13 19:32:49, wutao wrote: > > On 2017/06/13 15:57:42, danakj wrote: > > > Why not WaitForCompositingDidCommit? > > > > I found there is no implementation of compositor->BeginMainFrame. What is the > > reason? By adding this will make the performance worse? > > > > I think there is slightly difference between BeginMainFrame and DidCommit in > > this test: > > If we waiting for DidCommit, then the layer changes in the current loop will > be > > effective at this point? > > Once they are committed, we will have to draw the changes before we commit > anything new. So if you wait for commit then change them, it will request > another commit once the draw is done, which is what you want. > > > In addition, which means we can start to prepare the > > layer changes in the next loop while the display compositor is working? > > The UI compositor will not make another frame until the display compositor has > acked that it is drawing. > > > Corner case for the first loop, it might include some earlier changes before > > entering the loop? > > I'm not sure what you mean here. > > > If we waiting for BeginMainFrame, is the same thing all the layer changes in > the > > current loop will be processed? (changes happened before DoBeginMainFrame() > > should be updated in LayerTreeHost::UpdateLayers() in single_thread_proxy?). > > Changes made in BeginMainFrame will be part of the next commit, just as changes > made after the last commit (DidCommit) and before BeginMainFrame. > > > Then this will work too in this test case? > > > > Anyway, we might need to WaitForBeginMainFrame before entering the loops to > > clear state and WaitForCompositingEnded after all the loops to wait the last > > change finish. > > What if you do some warmup runs before you start measuring, or wait for a > DidCommit before you start the loop, and don't measure the last iteration of the > loop (ie don't WaitForCompositingEnded)? > > > And in each loop, we WaitForCompositingDidCommit? > > > > If we do not want to add new function to CompositorObserver, then > > WaitForCompositingDidCommit should work as well. > > I prefer to not add it if we don't need it. Done.
The CQ bit was checked by wutao@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.
lgtm https://codereview.chromium.org/2932503003/diff/40001/ash/perftests/ash_backg... File ash/perftests/ash_background_filter_blur_perftest.cc (right): https://codereview.chromium.org/2932503003/diff/40001/ash/perftests/ash_backg... ash/perftests/ash_background_filter_blur_perftest.cc:14: class AshBackgroundFilterBlurPerfTest : public test::AshTestBase { nit: put this in anonymous namespace https://codereview.chromium.org/2932503003/diff/40001/ash/perftests/ash_backg... ash/perftests/ash_background_filter_blur_perftest.cc:40: ui::Compositor* compositor_; ditto https://codereview.chromium.org/2932503003/diff/40001/ash/perftests/ash_backg... ash/perftests/ash_background_filter_blur_perftest.cc:109: perf_test::PrintResult("AshBackgroundFilterBlurPerfTest", "", test_name, nit: std::string()
https://codereview.chromium.org/2932503003/diff/40001/ash/BUILD.gn File ash/BUILD.gn (right): https://codereview.chromium.org/2932503003/diff/40001/ash/BUILD.gn#newcode1516 ash/BUILD.gn:1516: "test/ash_test_suite.cc", AshTestSuite sets up software GL (ie, OSMesa). Is that what you're intending to perf test? https://cs.chromium.org/chromium/src/ui/gl/test/gl_surface_test_support.cc?rc... https://codereview.chromium.org/2932503003/diff/40001/ash/perftests/ash_backg... File ash/perftests/ash_background_filter_blur_perftest.cc (right): https://codereview.chromium.org/2932503003/diff/40001/ash/perftests/ash_backg... ash/perftests/ash_background_filter_blur_perftest.cc:50: // This is for consistnecy even if the default display size changed. consistency
Uploaded a new patch which will use hardware gpu. But at linux desktop, must set use_ozone = false to create the gl_context_egl with non-offscreen context. Not sure it is a limitation or some setting is not correct. https://codereview.chromium.org/2932503003/diff/40001/ash/BUILD.gn File ash/BUILD.gn (right): https://codereview.chromium.org/2932503003/diff/40001/ash/BUILD.gn#newcode1516 ash/BUILD.gn:1516: "test/ash_test_suite.cc", On 2017/06/13 23:20:40, danakj wrote: > AshTestSuite sets up software GL (ie, OSMesa). Is that what you're intending to > perf test? > > https://cs.chromium.org/chromium/src/ui/gl/test/gl_surface_test_support.cc?rc... Thanks to point this out. In new patch, I append the switch kUseGpuInTests when launching the ash_perftest. But some how, I must set use_ozone = false in the args.gn to set up the gpu properly. It seems gl_context_egl cannot MakeCurrent when create a NON-offscreen context. Will dig more. https://codereview.chromium.org/2932503003/diff/40001/ash/perftests/ash_backg... File ash/perftests/ash_background_filter_blur_perftest.cc (right): https://codereview.chromium.org/2932503003/diff/40001/ash/perftests/ash_backg... ash/perftests/ash_background_filter_blur_perftest.cc:14: class AshBackgroundFilterBlurPerfTest : public test::AshTestBase { On 2017/06/13 23:15:44, oshima wrote: > nit: put this in anonymous namespace Done. https://codereview.chromium.org/2932503003/diff/40001/ash/perftests/ash_backg... ash/perftests/ash_background_filter_blur_perftest.cc:40: ui::Compositor* compositor_; On 2017/06/13 23:15:44, oshima wrote: > ditto Done. https://codereview.chromium.org/2932503003/diff/40001/ash/perftests/ash_backg... ash/perftests/ash_background_filter_blur_perftest.cc:50: // This is for consistnecy even if the default display size changed. On 2017/06/13 23:20:40, danakj wrote: > consistency Done. https://codereview.chromium.org/2932503003/diff/40001/ash/perftests/ash_backg... ash/perftests/ash_background_filter_blur_perftest.cc:109: perf_test::PrintResult("AshBackgroundFilterBlurPerfTest", "", test_name, On 2017/06/13 23:15:44, oshima wrote: > nit: std::string() Done.
The CQ bit was checked by wutao@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.
> TEST=perftest This field is meant as an explanation for QA folks to verify a change, and is not to describe automated tests, nor needed for changes that include automated tests and do not need manual verification (hopefully that's all CLs these days). So you can leave this out of most CLs, including this one. https://codereview.chromium.org/2932503003/diff/60001/ash/perftests/ash_backg... File ash/perftests/ash_background_filter_blur_perftest.cc (right): https://codereview.chromium.org/2932503003/diff/60001/ash/perftests/ash_backg... ash/perftests/ash_background_filter_blur_perftest.cc:16: // TODO(wutao): At linux desktop, the tests only run with use_ozone = false. nit: On chromeos_linux builds, .. https://codereview.chromium.org/2932503003/diff/60001/ash/perftests/ash_backg... ash/perftests/ash_background_filter_blur_perftest.cc:81: for (int i = 1; i <= num_iteration + 1; ++i) { By changing to <= and +1, you're adding an extra iteration, idk if that was intended. https://codereview.chromium.org/2932503003/diff/60001/ash/test/ash_perftests.cc File ash/test/ash_perftests.cc (right): https://codereview.chromium.org/2932503003/diff/60001/ash/test/ash_perftests.... ash/test/ash_perftests.cc:14: int RunHelper(ash::test::AshTestSuite* testSuite) { test_suite https://codereview.chromium.org/2932503003/diff/60001/ash/test/ash_perftests.... ash/test/ash_perftests.cc:15: base::CommandLine::ForCurrentProcess()->AppendSwitch( Leave a comment what this does and why
LGTM w/ that
The CQ bit was checked by wutao@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...
https://codereview.chromium.org/2932503003/diff/60001/ash/perftests/ash_backg... File ash/perftests/ash_background_filter_blur_perftest.cc (right): https://codereview.chromium.org/2932503003/diff/60001/ash/perftests/ash_backg... ash/perftests/ash_background_filter_blur_perftest.cc:16: // TODO(wutao): At linux desktop, the tests only run with use_ozone = false. On 2017/06/15 15:15:00, danakj wrote: > nit: On chromeos_linux builds, .. Done. https://codereview.chromium.org/2932503003/diff/60001/ash/perftests/ash_backg... ash/perftests/ash_background_filter_blur_perftest.cc:81: for (int i = 1; i <= num_iteration + 1; ++i) { On 2017/06/15 15:14:59, danakj wrote: > By changing to <= and +1, you're adding an extra iteration, idk if that was > intended. Yes, as we discussed, we do not measure the last iteration num_iteration + 1 since it might not finish. There is a check at the end of loop I only measure the time if (i <= num_iteration). So the total measure is 1..num_iteration. https://codereview.chromium.org/2932503003/diff/60001/ash/test/ash_perftests.cc File ash/test/ash_perftests.cc (right): https://codereview.chromium.org/2932503003/diff/60001/ash/test/ash_perftests.... ash/test/ash_perftests.cc:14: int RunHelper(ash::test::AshTestSuite* testSuite) { On 2017/06/15 15:15:00, danakj wrote: > test_suite Done. https://codereview.chromium.org/2932503003/diff/60001/ash/test/ash_perftests.... ash/test/ash_perftests.cc:15: base::CommandLine::ForCurrentProcess()->AppendSwitch( On 2017/06/15 15:15:00, danakj wrote: > Leave a comment what this does and why Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by wutao@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.
The CQ bit was checked by wutao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2932503003/#ps80001 (title: "Fix nits in patch 4.")
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": 80001, "attempt_start_ts": 1497555860997030,
"parent_rev": "5b29b7b17c700688dacde791d73ea86709336950", "commit_rev":
"5c36dc747da0feb63a206edaf0dfc470471ade20"}
Message was sent while issue was closed.
Description was changed from ========== Add ash perftest on background blur filter. Add an ash perftest to evaluate the background blur filter efficiency. BUG=730841,646062,682082 TEST=perftest ========== to ========== Add ash perftest on background blur filter. Add an ash perftest to evaluate the background blur filter efficiency. BUG=730841,646062,682082 TEST=perftest Review-Url: https://codereview.chromium.org/2932503003 Cr-Commit-Position: refs/heads/master@{#479797} Committed: https://chromium.googlesource.com/chromium/src/+/5c36dc747da0feb63a206edaf0df... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/5c36dc747da0feb63a206edaf0df... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
