Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
413243004:
cc: Fix DCHECK in tile manager queue perftests. (Closed)
|
Created:
6 years, 5 months ago by vmpstr Modified:
6 years, 5 months ago CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
Descriptioncc: Fix DCHECK in tile manager queue perftests.
There's an issue with adding some of the tilings, since the scales
collide (or are not valid otherwise), which is causing some DCHECKs
to be triggered.
This just shuffles the scales, as well as make the layer smaller so
that we don't create massive amounts of textures (which is pretty slow
even on my desktop).
R=reveman, jbedley
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285506
Patch Set 1 #
Total comments: 11
Patch Set 2 : update #Messages
Total messages: 9 (0 generated)
PTAL
https://codereview.chromium.org/413243004/diff/1/cc/resources/tile_manager_pe... File cc/resources/tile_manager_perftest.cc (right): https://codereview.chromium.org/413243004/diff/1/cc/resources/tile_manager_pe... cc/resources/tile_manager_perftest.cc:354: SetupDefaultTrees(gfx::Size(1000, 1000)); What's the motivation for using this specific size? Does it closer match what is used in the wild? https://codereview.chromium.org/413243004/diff/1/cc/resources/tile_manager_pe... cc/resources/tile_manager_perftest.cc:373: RunRasterQueueConstructTest("50"); why not 100? is the overlap in scales between the first 10 and last 40 intentional and necessary? seems like it would be a bit easier to make sure they don't overlap if that's not the case.. https://codereview.chromium.org/413243004/diff/1/cc/resources/tile_manager_pe... cc/resources/tile_manager_perftest.cc:377: SetupDefaultTrees(gfx::Size(10000, 10000)); should the size change here too?
https://codereview.chromium.org/413243004/diff/1/cc/resources/tile_manager_pe... File cc/resources/tile_manager_perftest.cc (right): https://codereview.chromium.org/413243004/diff/1/cc/resources/tile_manager_pe... cc/resources/tile_manager_perftest.cc:354: SetupDefaultTrees(gfx::Size(1000, 1000)); On 2014/07/24 19:52:58, reveman wrote: > What's the motivation for using this specific size? Does it closer match what is > used in the wild? The problem is really just creating all the tiles for 100 layers with tree being 10000x10000. It takes about 40 seconds just to build that in a debug build on linux. This change is just so it doesn't time out on the bots. The current approach creates everything fairly quickly (1-2 seconds). https://codereview.chromium.org/413243004/diff/1/cc/resources/tile_manager_pe... cc/resources/tile_manager_perftest.cc:373: RunRasterQueueConstructTest("50"); On 2014/07/24 19:52:58, reveman wrote: > why not 100? is the overlap in scales between the first 10 and last 40 > intentional and necessary? seems like it would be a bit easier to make sure they > don't overlap if that's not the case.. I don't think there's overlap? I mean the fix was here against add tiling adding a scale that already exists, so an overlap would dcheck. https://codereview.chromium.org/413243004/diff/1/cc/resources/tile_manager_pe... cc/resources/tile_manager_perftest.cc:377: SetupDefaultTrees(gfx::Size(10000, 10000)); On 2014/07/24 19:52:58, reveman wrote: > should the size change here too? This needs to be large in order to get 128 tiles out of it. This is only testing 2 layers, so the creation of the layer is fast. I could go to 10000x10000 in the above case to be consistent? My only worry is the long running time of the test.
https://codereview.chromium.org/413243004/diff/1/cc/resources/tile_manager_pe... File cc/resources/tile_manager_perftest.cc (right): https://codereview.chromium.org/413243004/diff/1/cc/resources/tile_manager_pe... cc/resources/tile_manager_perftest.cc:354: SetupDefaultTrees(gfx::Size(1000, 1000)); On 2014/07/24 19:58:21, vmpstr wrote: > On 2014/07/24 19:52:58, reveman wrote: > > What's the motivation for using this specific size? Does it closer match what > is > > used in the wild? > > The problem is really just creating all the tiles for 100 layers with tree being > 10000x10000. It takes about 40 seconds just to build that in a debug build on > linux. This change is just so it doesn't time out on the bots. The current > approach creates everything fairly quickly (1-2 seconds). Acknowledged. https://codereview.chromium.org/413243004/diff/1/cc/resources/tile_manager_pe... cc/resources/tile_manager_perftest.cc:373: RunRasterQueueConstructTest("50"); On 2014/07/24 19:58:21, vmpstr wrote: > On 2014/07/24 19:52:58, reveman wrote: > > why not 100? is the overlap in scales between the first 10 and last 40 > > intentional and necessary? seems like it would be a bit easier to make sure > they > > don't overlap if that's not the case.. > > I don't think there's overlap? I mean the fix was here against add tiling adding > a scale that already exists, so an overlap would dcheck. I meant overlap in the sense that: 1.3f + i * 0.3f, where i=7 is greater than 2.03f + i * 0.03f, where i=0, which is greater than 1.3f + i * 0.3f, where i=0 just makes it harder to see if the scales might collide compared to something simple as: 1.3f + num_tilings * 0.3f in both cases Still curious to why you changed it from 100 to 50. Is that part of fixing timeout issues? https://codereview.chromium.org/413243004/diff/1/cc/resources/tile_manager_pe... cc/resources/tile_manager_perftest.cc:377: SetupDefaultTrees(gfx::Size(10000, 10000)); On 2014/07/24 19:58:21, vmpstr wrote: > On 2014/07/24 19:52:58, reveman wrote: > > should the size change here too? > > This needs to be large in order to get 128 tiles out of it. This is only testing > 2 layers, so the creation of the layer is fast. > > I could go to 10000x10000 in the above case to be consistent? My only worry is > the long running time of the test. How it is now is fine. I was just curious to why this was different.
PTAL https://codereview.chromium.org/413243004/diff/1/cc/resources/tile_manager_pe... File cc/resources/tile_manager_perftest.cc (right): https://codereview.chromium.org/413243004/diff/1/cc/resources/tile_manager_pe... cc/resources/tile_manager_perftest.cc:373: RunRasterQueueConstructTest("50"); On 2014/07/24 20:14:37, reveman wrote: > On 2014/07/24 19:58:21, vmpstr wrote: > > On 2014/07/24 19:52:58, reveman wrote: > > > why not 100? is the overlap in scales between the first 10 and last 40 > > > intentional and necessary? seems like it would be a bit easier to make sure > > they > > > don't overlap if that's not the case.. > > > > I don't think there's overlap? I mean the fix was here against add tiling > adding > > a scale that already exists, so an overlap would dcheck. > > I meant overlap in the sense that: > > 1.3f + i * 0.3f, where i=7 is greater than > 2.03f + i * 0.03f, where i=0, which is greater than > 1.3f + i * 0.3f, where i=0 Ah I see what you mean. That is better, I've changed it. > > just makes it harder to see if the scales might collide compared to something > simple as: > > 1.3f + num_tilings * 0.3f in both cases > > Still curious to why you changed it from 100 to 50. Is that part of fixing > timeout issues? Yes, this is due to timeouts... https://codereview.chromium.org/413243004/diff/1/cc/resources/tile_manager_pe... cc/resources/tile_manager_perftest.cc:377: SetupDefaultTrees(gfx::Size(10000, 10000)); On 2014/07/24 20:14:38, reveman wrote: > On 2014/07/24 19:58:21, vmpstr wrote: > > On 2014/07/24 19:52:58, reveman wrote: > > > should the size change here too? > > > > This needs to be large in order to get 128 tiles out of it. This is only > testing > > 2 layers, so the creation of the layer is fast. > > > > I could go to 10000x10000 in the above case to be consistent? My only worry is > > the long running time of the test. > > How it is now is fine. I was just curious to why this was different. Acknowledged.
lgtm
The CQ bit was checked by vmpstr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vmpstr@chromium.org/413243004/20001
Message was sent while issue was closed.
Change committed as 285506 |
