Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8052016:
Adding support for animated pages in the FrameRate tests (Closed)
|
Created:
9 years, 2 months ago by Justin Novosad Modified:
9 years, 2 months ago Reviewers:
nduca CC:
chromium-reviews, Paweł Hajdan Jr. Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdding support for animated pages in the FrameRate tests
BUG=98273
TEST=none
REVIEW=http://codereview.chromium.org/8052016/
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=106967
Patch Set 1 #
Total comments: 1
Patch Set 2 : '' #
Total comments: 7
Patch Set 3 : '' #
Total comments: 3
Patch Set 4 : '' #
Total comments: 8
Patch Set 5 : '' #
Total comments: 3
Patch Set 6 : '' #
Total comments: 1
Patch Set 7 : '' #Patch Set 8 : '' #
Total comments: 6
Patch Set 9 : '' #
Messages
Total messages: 15 (0 generated)
PTAL http://codereview.chromium.org/8052016/diff/1/chrome/test/data/perf/frame_rat... File chrome/test/data/perf/frame_rate/head.js (right): http://codereview.chromium.org/8052016/diff/1/chrome/test/data/perf/frame_rat... chrome/test/data/perf/frame_rate/head.js:168: {"time_ms":5, "y":10} factored this out of __gestures so that we don't have to redefine it every time we override __gestures in a test page
http://codereview.chromium.org/8052016/diff/3001/chrome/test/data/perf/frame_... File chrome/test/data/perf/frame_rate/head.js (right): http://codereview.chromium.org/8052016/diff/3001/chrome/test/data/perf/frame_... chrome/test/data/perf/frame_rate/head.js:209: if ("requestAnimationFrame" in window) Nice! http://codereview.chromium.org/8052016/diff/3001/chrome/test/data/perf/frame_... chrome/test/data/perf/frame_rate/head.js:234: __init_raf(true); Init raf here? Really? http://codereview.chromium.org/8052016/diff/3001/chrome/test/data/perf/frame_... chrome/test/data/perf/frame_rate/head.js:335: var use_raf = true; Did you consider doing this feature outside of the test harness. E.g. launch chrome with --disable-vsync? By pulling this in, I'm not quite sure what you're buying... but its probably because you have something in mind that I dont. :) http://codereview.chromium.org/8052016/diff/3001/chrome/test/data/perf/frame_... chrome/test/data/perf/frame_rate/head.js:357: // Toggle usage of request animantion frame I'm struggling to understand this code... and the code above that's looking at gestures[i0].raf. Is there a cleaner factoring of this? http://codereview.chromium.org/8052016/diff/3001/chrome/test/data/perf/frame_... chrome/test/data/perf/frame_rate/head.js:415: var animated = __animation(); Is there any merit to this rather than just having the user call raf themselves? http://codereview.chromium.org/8052016/diff/3001/chrome/test/data/perf/frame_... chrome/test/data/perf/frame_rate/head.js:437: __init_raf(true); We init_raf at the global scope too? Is that one redundant? http://codereview.chromium.org/8052016/diff/3001/chrome/test/data/perf/frame_... chrome/test/data/perf/frame_rate/head.js:478: __start("init"); Not that this is your fault, but since we're here, do you have a sense of whether this is this even needed? By the comment, which predates me, I'm guessing the goal is to force a style recalc/etc by just asking for document.body.offsetTop
> chrome/test/data/perf/frame_rate/head.js:478: __start("init");
> Not that this is your fault, but since we're here, do you have a sense of
> whether this is this even needed? By the comment, which predates me, I'm
> guessing the goal is to force a style recalc/etc by just asking for
> document.body.offsetTop
We can probably ignore this for now. :) But I think we should probably replace
this with a "do a <user specified> seconds of nothing before starting the test."
On 2011/10/10 23:11:19, nduca wrote: > chrome/test/data/perf/frame_rate/head.js:335: var use_raf = true; > Did you consider doing this feature outside of the test harness. E.g. launch > chrome with --disable-vsync? By pulling this in, I'm not quite sure what you're > buying... but its probably because you have something in mind that I dont. :) Yeah, the idea is that we can benchmark both a RAF and non-RAF version of the animation loop in a single run/page load. My idea was to streamline test execution by doing it this way. > > chrome/test/data/perf/frame_rate/head.js:357: // Toggle usage of request > animantion frame > I'm struggling to understand this code... and the code above that's looking at > gestures[i0].raf. Is there a cleaner factoring of this? I think gestures in general need to be documented better. I think I should add some explanations at the top of the file. Basically, this is how it works: all gestures start with RAF enabled by default. The raf attribute can be set on a gesture control point in order to disable or enbable the use of RAF at a particular point in time. > > http://codereview.chromium.org/8052016/diff/3001/chrome/test/data/perf/frame_... > chrome/test/data/perf/frame_rate/head.js:415: var animated = __animation(); > Is there any merit to this rather than just having the user call raf themselves? If the user calls raf (is in control of the main loop), then the user is also resposible for executing gestures and exiting the loop when the last gesture has executed. I like it better this way because the test code requires little knowledge how the test harness works (better encapsulation). I think this will make it easier if we want to add testing functionality in the future. > > http://codereview.chromium.org/8052016/diff/3001/chrome/test/data/perf/frame_... > chrome/test/data/perf/frame_rate/head.js:437: __init_raf(true); > We init_raf at the global scope too? Is that one redundant? Godd catch. the one at global scope is a remnant of an earlier iteration. > http://codereview.chromium.org/8052016/diff/3001/chrome/test/data/perf/frame_... > chrome/test/data/perf/frame_rate/head.js:478: __start("init"); > Not that this is your fault, but since we're here, do you have a sense of > whether this is this even needed? By the comment, which predates me, I'm > guessing the goal is to force a style recalc/etc by just asking for > document.body.offsetTop Yeah, I wondered about this too. My understanding that because of how gestures get repeated in a loop (see __create_repeating_gesture_function) this gesture results in a fast vertical sweep of the entire page, which means that all elements in the page _should_ get rendered and composited at least once before the benchmarking starts. I assume this is to get lazily initialized stuff out of the way so that we end-up measuring fps in a steady state. I think it makes sense, but I haven't measured the impact of it.
http://codereview.chromium.org/8052016/diff/8001/chrome/test/perf/frame_rate/... File chrome/test/perf/frame_rate/frame_rate_tests.cc (right): http://codereview.chromium.org/8052016/diff/8001/chrome/test/perf/frame_rate/... chrome/test/perf/frame_rate/frame_rate_tests.cc:28: template<unsigned TestFlags> I chose to use a template arg here because the TEST_F macro provides no convenient way to pass parameters to the test constructor or SetUp method. I find this way is cleaner and more concise and maintainable than creating a subclass for each test variant. http://codereview.chromium.org/8052016/diff/8001/chrome/test/perf/frame_rate/... chrome/test/perf/frame_rate/frame_rate_tests.cc:58: launch_arguments_.AppendSwitch(switches::kDisableAccelerated2dCanvas); because the switch dependencies may change over time, I am just disabling all accelerations explicitly. http://codereview.chromium.org/8052016/diff/8001/chrome/test/perf/frame_rate/... chrome/test/perf/frame_rate/frame_rate_tests.cc:95: GURL("javascript:__init_raf(false);"))); I attempted to use the --disable-gpu-vsync switch but it does not behave the way I hoped: it artificially maintains a 60 fps rate even when not frame locked. :-( So, I chose to keep the javascript implementation for disbling raf, except that I am now controlling it from C++
http://codereview.chromium.org/8052016/diff/12001/chrome/test/data/perf/frame... File chrome/test/data/perf/frame_rate/head.js (left): http://codereview.chromium.org/8052016/diff/12001/chrome/test/data/perf/frame... chrome/test/data/perf/frame_rate/head.js:397: } Did we file a bug to move the multiple-geseture stuff out to the frame_Rate test? http://codereview.chromium.org/8052016/diff/12001/chrome/test/data/perf/frame... File chrome/test/data/perf/frame_rate/head.js (right): http://codereview.chromium.org/8052016/diff/12001/chrome/test/data/perf/frame... chrome/test/data/perf/frame_rate/head.js:56: var __animation = false; Add comment explaining what it does? Just declare it this way: var __animation_callback; Then you can do if (__animation_callback) since var x; makes x undefined, and undefined is falsy. http://codereview.chromium.org/8052016/diff/12001/chrome/test/data/perf/frame... File chrome/test/data/perf/frame_rate/head_animation.js (right): http://codereview.chromium.org/8052016/diff/12001/chrome/test/data/perf/frame... chrome/test/data/perf/frame_rate/head_animation.js:32: __animation = true; as I mentioend before, how about just saying __animation_callback = function() { // body of your animation hook } http://codereview.chromium.org/8052016/diff/12001/chrome/test/perf/frame_rate... File chrome/test/perf/frame_rate/frame_rate_tests.cc (left): http://codereview.chromium.org/8052016/diff/12001/chrome/test/perf/frame_rate... chrome/test/perf/frame_rate/frame_rate_tests.cc:143: FRAME_RATE_TEST_WITH_AND_WITHOUT_ACCELERATED_COMPOSITING(blank); move these up next to the define? http://codereview.chromium.org/8052016/diff/12001/chrome/test/perf/frame_rate... File chrome/test/perf/frame_rate/frame_rate_tests.cc (right): http://codereview.chromium.org/8052016/diff/12001/chrome/test/perf/frame_rate... chrome/test/perf/frame_rate/frame_rate_tests.cc:56: if (TestFlags & kDisableGpu) { Do we have a global flag for this? @zmo might know, this feels fragile to have to maintain (oops, someone added somethign else and now the frame rate "disabled" tests are accidentally using the gpu"). If there isn't a central flag, we shoudl add one or move this into test_launcher_utils. http://codereview.chromium.org/8052016/diff/12001/chrome/test/perf/frame_rate... chrome/test/perf/frame_rate/frame_rate_tests.cc:67: launch_arguments_.AppendSwitch(switches::kAllowWebUICompositing); Append this always? There's no harm in it if we put it with the disable flags. http://codereview.chromium.org/8052016/diff/12001/chrome/test/perf/frame_rate... chrome/test/perf/frame_rate/frame_rate_tests.cc:140: typedef FrameRateTest_<0> I think gtest has parameterized tests, though it might not make sense here. Take a peek and tell me if I'm full of it? :) http://codereview.chromium.org/8052016/diff/12001/chrome/test/perf/frame_rate... chrome/test/perf/frame_rate/frame_rate_tests.cc:188: #define FRAME_RATE_TEST_ANIMATED_2D_CANVAS(content) \ I really think you should factor this differently. I realize you're just covering the bases here, but I think you should take a step back and go "my end goal is to make sure that I can track performance gains and regressions against performance. Do you really need to run 8 signals for every piece of canvas content? Did you consider defining different types of test content and then have configuraitons tailored for the types of things you want to learn from those tests? That is to say, the vast majority of canvas tests probabaly exercise canvas performance. I imagine for those, you just want to see the unbounded performance and also the performance were they on a vsync'd machine (e.g. did it run at 60fps or not). You *could* track software mode against all of these, but honestly, since for the majority of things you know its a win. So why not just make a few specific targeted pieces of content where you know they're not good vs software, and make a configuration for those tests that does software and gpu mode. There are a small number of tests that want to test raf/nonraf performance. Again, won't 1 or 2 pieces of content work here?
Also, in light of the fact that I've been raking you over the coals about this, I'm fine landing the current patch with whatever edits you can manager. Lets do a followup when we can manage. LGTM. On 2011/10/14 21:01:46, nduca wrote: > http://codereview.chromium.org/8052016/diff/12001/chrome/test/data/perf/frame... > File chrome/test/data/perf/frame_rate/head.js (left): > > http://codereview.chromium.org/8052016/diff/12001/chrome/test/data/perf/frame... > chrome/test/data/perf/frame_rate/head.js:397: } > Did we file a bug to move the multiple-geseture stuff out to the frame_Rate > test? > > http://codereview.chromium.org/8052016/diff/12001/chrome/test/data/perf/frame... > File chrome/test/data/perf/frame_rate/head.js (right): > > http://codereview.chromium.org/8052016/diff/12001/chrome/test/data/perf/frame... > chrome/test/data/perf/frame_rate/head.js:56: var __animation = false; > Add comment explaining what it does? > > Just declare it this way: > var __animation_callback; > > Then you can do if (__animation_callback) > since var x; makes x undefined, and undefined is falsy. > > http://codereview.chromium.org/8052016/diff/12001/chrome/test/data/perf/frame... > File chrome/test/data/perf/frame_rate/head_animation.js (right): > > http://codereview.chromium.org/8052016/diff/12001/chrome/test/data/perf/frame... > chrome/test/data/perf/frame_rate/head_animation.js:32: __animation = true; > as I mentioend before, how about just saying __animation_callback = function() { > // body of your animation hook > } > > http://codereview.chromium.org/8052016/diff/12001/chrome/test/perf/frame_rate... > File chrome/test/perf/frame_rate/frame_rate_tests.cc (left): > > http://codereview.chromium.org/8052016/diff/12001/chrome/test/perf/frame_rate... > chrome/test/perf/frame_rate/frame_rate_tests.cc:143: > FRAME_RATE_TEST_WITH_AND_WITHOUT_ACCELERATED_COMPOSITING(blank); > move these up next to the define? > > http://codereview.chromium.org/8052016/diff/12001/chrome/test/perf/frame_rate... > File chrome/test/perf/frame_rate/frame_rate_tests.cc (right): > > http://codereview.chromium.org/8052016/diff/12001/chrome/test/perf/frame_rate... > chrome/test/perf/frame_rate/frame_rate_tests.cc:56: if (TestFlags & kDisableGpu) > { > Do we have a global flag for this? @zmo might know, this feels fragile to have > to maintain (oops, someone added somethign else and now the frame rate > "disabled" tests are accidentally using the gpu"). If there isn't a central > flag, we shoudl add one or move this into test_launcher_utils. > > http://codereview.chromium.org/8052016/diff/12001/chrome/test/perf/frame_rate... > chrome/test/perf/frame_rate/frame_rate_tests.cc:67: > launch_arguments_.AppendSwitch(switches::kAllowWebUICompositing); > Append this always? There's no harm in it if we put it with the disable flags. > > http://codereview.chromium.org/8052016/diff/12001/chrome/test/perf/frame_rate... > chrome/test/perf/frame_rate/frame_rate_tests.cc:140: typedef FrameRateTest_<0> > I think gtest has parameterized tests, though it might not make sense here. Take > a peek and tell me if I'm full of it? :) > > http://codereview.chromium.org/8052016/diff/12001/chrome/test/perf/frame_rate... > chrome/test/perf/frame_rate/frame_rate_tests.cc:188: #define > FRAME_RATE_TEST_ANIMATED_2D_CANVAS(content) \ > I really think you should factor this differently. > > I realize you're just covering the bases here, but I think you should take a > step back and go "my end goal is to make sure that I can track performance gains > and regressions against performance. > > Do you really need to run 8 signals for every piece of canvas content? > > Did you consider defining different types of test content and then have > configuraitons tailored for the types of things you want to learn from those > tests? > > > > That is to say, the vast majority of canvas tests probabaly exercise canvas > performance. I imagine for those, you just want to see the unbounded performance > and also the performance were they on a vsync'd machine (e.g. did it run at > 60fps or not). > > You *could* track software mode against all of these, but honestly, since for > the majority of things you know its a win. So why not just make a few specific > targeted pieces of content where you know they're not good vs software, and make > a configuration for those tests that does software and gpu mode. > > There are a small number of tests that want to test raf/nonraf performance. > Again, won't 1 or 2 pieces of content work here?
http://codereview.chromium.org/8052016/diff/17001/chrome/test/perf/frame_rate... File chrome/test/perf/frame_rate/frame_rate_tests.cc (right): http://codereview.chromium.org/8052016/diff/17001/chrome/test/perf/frame_rate... chrome/test/perf/frame_rate/frame_rate_tests.cc:27: kReference = 1 << 3, kUseReferenceBuild http://codereview.chromium.org/8052016/diff/17001/chrome/test/perf/frame_rate... chrome/test/perf/frame_rate/frame_rate_tests.cc:31: class FrameRateTestVariant { Would this be cleaner with FrameRateTestVariant having a bunch of bool fields instead of an enum? You'd still need the PrintTo code, but it'd be a lot easier to read the printTo function... http://codereview.chromium.org/8052016/diff/17001/chrome/test/perf/frame_rate... chrome/test/perf/frame_rate/frame_rate_tests.cc:157: std::string trace_name = "fps"; can't you do suffix = (GetParam().GetFlags() & kReference) ? "_ref" : ""; The string being passed in here is what the performance graphs use --- name is which graph its part of, and trace_name is the data series. (Maybe we should s/trace_name/data_series)
On 2011/10/19 19:25:48, nduca wrote: > Would this be cleaner with FrameRateTestVariant having a bunch of bool fields > instead of an enum? You'd still need the PrintTo code, but it'd be a lot easier > to read the printTo function... Yeah, but the constructor calls would be less readable if it received a chain of bools.
Yeah, fair point. Your call, from my point of view. The big bit was the _ref suffixing.
http://codereview.chromium.org/8052016/diff/20001/chrome/test/perf/frame_rate... File chrome/test/perf/frame_rate/frame_rate_tests.cc (right): http://codereview.chromium.org/8052016/diff/20001/chrome/test/perf/frame_rate... chrome/test/perf/frame_rate/frame_rate_tests.cc:62: std::string ParamTestNameSuffix(FrameRateTestVariant param, int) { I know this unused int param looks weird. It is so that this function gets automatically picked up when this change goes in: http://codereview.appspot.com/5305047/
Never mind comment #11. I reverted that bit in the latest Patch Set.
LGTM with nits and a crbug.com request. http://codereview.chromium.org/8052016/diff/23001/chrome/test/perf/frame_rate... File chrome/test/perf/frame_rate/frame_rate_tests.cc (right): http://codereview.chromium.org/8052016/diff/23001/chrome/test/perf/frame_rate... chrome/test/perf/frame_rate/frame_rate_tests.cc:39: void PrintTo(const FrameRateTestVariant& v, std::ostream* os) { Do we need this bit now? If we don't need it, I'd prefer we don't have it. http://codereview.chromium.org/8052016/diff/23001/chrome/test/perf/frame_rate... chrome/test/perf/frame_rate/frame_rate_tests.cc:62: std::string ParamTestNameSuffix(FrameRateTestVariant param) { Move this up to the flags? And make it take in the FrameRateTestFlags intead? E.g. GetSuffixForTestFlags? http://codereview.chromium.org/8052016/diff/23001/chrome/test/perf/frame_rate... chrome/test/perf/frame_rate/frame_rate_tests.cc:77: , public ::testing::WithParamInterface<FrameRateTestVariant> { Can we parameterize tests on the enum directly? NBD if not, but it'd be cool if we didn't need the FrameRateTestVariant. http://codereview.chromium.org/8052016/diff/23001/chrome/test/perf/frame_rate... chrome/test/perf/frame_rate/frame_rate_tests.cc:197: RunTest(#content); \ typedef here? Its awkward how we use FrameRateTest here but FrameRateCanvasTest down below. Ideally, they'd follow a similar pattern. http://codereview.chromium.org/8052016/diff/23001/chrome/test/perf/frame_rate... chrome/test/perf/frame_rate/frame_rate_tests.cc:203: INSTANTIATE_TEST_CASE_P(, FrameRateTest, ::testing::Values( can we put this after test_p and before frame_rate_test_with_and_without-accelerated_compositing? http://codereview.chromium.org/8052016/diff/23001/chrome/test/perf/frame_rate... chrome/test/perf/frame_rate/frame_rate_tests.cc:218: INSTANTIATE_TEST_CASE_P(, FrameRateCanvasTest, ::testing::Values( Lets file a bug for "don't run 5 tests for every single canvas frame rate test." I don't want that bit of feedback to fall on the floor --- from a practical side and from a bot-cycling-time perspective, this is a problem waiting to happen.
On 2011/10/20 20:09:27, nduca wrote: > Can we parameterize tests on the enum directly? NBD if not, but it'd be cool if > we didn't need the FrameRateTestVariant. Yes, I a removing that class. It was only needed because of PrintTo, which I am removing. However, I can't parameterize on on the enum. It has to be an integer type since two flags 'OR'ed together gives a value that is not a member of the enum. > > typedef here? Its awkward how we use FrameRateTest here but FrameRateCanvasTest > down below. Ideally, they'd follow a similar pattern. The problem is with how the gtest macros are designed. If I use the same test class name then the test instances in the INTANTIATE_TEST_CASE_P call will get merged together and applied to all test pages, which is not what we want. We will have to use a new class name ever ytime we want to use a new set of test variants.
> The problem is with how the gtest macros are designed. If I use the same test > class name then the test instances in the INTANTIATE_TEST_CASE_P call will get > merged together and applied to all test pages, which is not what we want. We > will have to use a new class name ever ytime we want to use a new set of test > variants. I get it. I was just saying, never use the base type? E.g. for the FrameRateTest that gets used for the compositing tests we make something like FrameRateCompositingTest |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
