Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(59)

Issue 196023015: Revert of cc: Replace recorded region with direct map lookup (Closed)

Created:
6 years, 9 months ago by Nico
Modified:
6 years, 9 months ago
Reviewers:
danakj, vmpstr, enne (OOO)
CC:
chromium-reviews, cc-bugs_chromium.org, Tom Hudson, reveman, benm (inactive), reed2
Visibility:
Public.

Description

Revert of cc: Replace recorded region with direct map lookup (https://codereview.chromium.org/196343005/) Reason for revert: Probably broke telemetry's testMeasurementSmoke (which only runs on the cq and not the main waterfall for some reason): http://crbug.com/350697 Original issue's description: > cc: Replace recorded region with direct map lookup > > If the viewport is extremely large, then keeping track of the recorded > region in PicturePile with an actual Region becomes extremely slow due > to a large number of rects being inserted into it. > > The recorded region behaves as a cache to the picture map; it's a > simpler way to know the state of all of the recordings contained within. > > In practice, the recorded region is only used for two things: a "should > this pile bother to create tilings" optimization and a "can a tile be > rastered in this content rect" check aka CanRaster. > > The optimization for "should create tilings" is replaced by a > has_any_recordings_ boolean, which could have a false positive in theory > (resizing to a smaller but non-empty size), but which shouldn't happen > in practice. Even if it did, this would only be a performance penalty > for creating no-op tilings that can't create tiles (due to CanRaster). > > The CanRaster check is replaced by a viewport hint, as most tiles that > the tiling creates will be inside of the very large expanded viewport > during recording, turning an expensive Region.Contains check to a > Rect.Contains one. In the edge cases where tiles are being created > outside of that expanded viewport, it will check the picture map > directly. This should only happen when the user has scrolled thousands > of pixels without a commit. > > BUG=b/13302269 > > Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256953 TBR=danakj@chromium.org,vmpstr@chromium.org,enne@chromium.org NOTREECHECKS=true NOTRY=true BUG=b/13302269 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257051

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+101 lines, -106 lines) Patch
M cc/layers/picture_layer.cc View 1 chunk +1 line, -0 lines 0 comments Download
M cc/layers/picture_layer_impl.cc View 3 chunks +8 lines, -2 lines 0 comments Download
M cc/layers/picture_layer_impl_unittest.cc View 4 chunks +16 lines, -13 lines 0 comments Download
M cc/layers/picture_layer_unittest.cc View 1 chunk +1 line, -1 line 0 comments Download
M cc/resources/picture_pile.cc View 4 chunks +7 lines, -9 lines 0 comments Download
M cc/resources/picture_pile_base.h View 3 chunks +8 lines, -12 lines 0 comments Download
M cc/resources/picture_pile_base.cc View 8 chunks +23 lines, -41 lines 0 comments Download
M cc/resources/prioritized_tile_set_unittest.cc View 1 chunk +1 line, -1 line 0 comments Download
M cc/resources/tile_manager_perftest.cc View 1 chunk +1 line, -1 line 0 comments Download
M cc/resources/tile_manager_unittest.cc View 1 chunk +1 line, -1 line 0 comments Download
M cc/test/fake_picture_layer_tiling_client.cc View 2 chunks +15 lines, -3 lines 0 comments Download
M cc/test/fake_picture_pile_impl.h View 1 chunk +8 lines, -4 lines 0 comments Download
M cc/test/fake_picture_pile_impl.cc View 4 chunks +11 lines, -18 lines 0 comments Download

Messages

Total messages: 3 (0 generated)
Nico
Created Revert of cc: Replace recorded region with direct map lookup
6 years, 9 months ago (2014-03-14 05:43:13 UTC) #1
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thakis@chromium.org/196023015/1
6 years, 9 months ago (2014-03-14 05:43:36 UTC) #2
commit-bot: I haz the power
6 years, 9 months ago (2014-03-14 06:51:57 UTC) #3
Message was sent while issue was closed.
Change committed as 257051

Powered by Google App Engine
This is Rietveld 408576698