[Chromium] Pre-paint more aggressively during fast scrolls.
Created attachment 124236 [details] Patch
Updated description: More aggressive predictive pre-painting for fast scrolls. Idle painting isn't sufficient for very fast scrolls because it has a lower memory priority (the reclaim limit is used for prepainting) and it can fall cronically behind if the scroll is fast enough to expose visible dirty tiles (pre-painting no longer executes, so we can't 'catch up'). This CL predictively paints up to one viewport worth of tiles in the scroll direction when a fast scroll is detected. Since each paint is expensive, this should also give us better paint performance by reducing the number of paints during a fast scroll, and let's us 'catch up' if a particular paint is expensive.
In parallel to this patch, can you start thinking about a patch to guess what our ideal upper memory allocation would be for the compositor? We are working on a central gpu memory manager, and the building block for that is a guess saying 'ideally, i'd have X' mb for textures...
(In reply to comment #3) > In parallel to this patch, can you start thinking about a patch to guess what our ideal upper memory allocation would be for the compositor? We are working on a central gpu memory manager, and the building block for that is a guess saying 'ideally, i'd have X' mb for textures... For ideal, do you mean ideal upper bound for the particular page being rendered, or ideal for any possible page?
(In reply to comment #4) > > For ideal, do you mean ideal upper bound for the particular page being rendered, or ideal for any possible page? For the current CCLayerTreeHost, ideally considering things like paint-time culling.
(In reply to comment #5) > (In reply to comment #4) > > > > For ideal, do you mean ideal upper bound for the particular page being rendered, or ideal for any possible page? > For the current CCLayerTreeHost, ideally considering things like paint-time culling. Right.. Yeah that seems useful. I'll think about it.
Comment on attachment 124236 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124236&action=review > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:479 > + IntSize scroll = scrollPrediction(); > + setScrollPrediction(IntSize()); It's weird to clear this LayerChromium field in TiledLayerChromium code. Why clear it at all? Don't you want to keep prepainting in the direction of the scroll prediction? > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:492 > + int maxWidthOfPaint = right - left; What makes you pick a full viewport as your default size? It seems like there's some tradeoff here about how big your commits are and how responsively new tiles appear. It also seems like there's some potentially strange timing issues here. If painting were really fast, you could have dirty visible tiles disappear because you didn't paint them while they were still visible. If painting were really slow, it's possible you'll have scrolled past this entire viewport. Maybe I'd feel better with more justification for this choice. > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:617 > + // FIXME: This function only calls reserve() to pre-protect visible > + // tiles during predictive scroll. It can be removed if we > + // pre-protect visible tiles elsewhere. I recently added a reserveTextures() function that reserves all visible tiles, if that's helpful.
Comment on attachment 124236 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124236&action=review >> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:479 >> + setScrollPrediction(IntSize()); > > It's weird to clear this LayerChromium field in TiledLayerChromium code. Why clear it at all? Don't you want to keep prepainting in the direction of the scroll prediction? The underlying idea is to track the amount of scroll since the last paint (that is the prediction for the next scroll), so when we get a chance to paint it should be reset. I could just set it only in WebViewImpl::applyScrollAndScale, but then it won't be set back to zero at the end of a scroll, since there is no scroll of 0,0 when the user stops scrolling. If there is another good place to reset it, this could be removed. >> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:492 >> + int maxWidthOfPaint = right - left; > > What makes you pick a full viewport as your default size? > > It seems like there's some tradeoff here about how big your commits are and how responsively new tiles appear. It also seems like there's some potentially strange timing issues here. If painting were really fast, you could have dirty visible tiles disappear because you didn't paint them while they were still visible. If painting were really slow, it's possible you'll have scrolled past this entire viewport. Maybe I'd feel better with more justification for this choice. One viewport was mainly chosen to be reasonable memory-wise. The responsiveness is definitely a trade-off, so possibly the paint size (impacts responsiveness) could be chosen separately from the amount of buffering (impacts memory). However, larger paints give us a greater chance of catching up when painting is falling behind. Ideally I think the idle-painting apron would be larger such that most of the extra viewport would already be painted, in which case this behavior should work well. Regarding the timing, all visible tiles are protected first (or painted if dirty), so they shouldn't disappear as you scroll. The only time you should see unpainted tiles is when painting can't keep up (we scroll more than one viewport between paints). At that point, the idea is to always connect the next paint to the visible viewport. If we *still* can't keep up with that constraint, it means that paint-rate < scroll-rate and there is just no way to make scrolling look good. Some tiles will have to fly-by unpainted in between the painted viewports, so better to just let painting fall behind IMO (rather than to have chunks of interspersed painted and un-painted tiles flying by). At that point I think it might be better to aggressively reduce paint time (eg. multi-resolution, cheap-paint-mode, etc.) >> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:617 >> + // pre-protect visible tiles elsewhere. > > I recently added a reserveTextures() function that reserves all visible tiles, if that's helpful. It's a little be more complicated, since for this we need to protect up to 2 viewports of tiles but paint as little as one row. This was just going to test if a tile is dirty, but we also need to insure we protect all tiles leading up to the painted tiles (so none get reclaimed and disappear). Note that this is what protects all the unpainted tiles, such that no visible tile should ever disappear during paint.
One additional thought about this: If were able request all our desired tiles during a separate step (like a prioritize pass), and then knew which tiles were allocated to this layer when we started to paint, we could merge all the painting into one pass and just chose a reasonable size of paint each frame (or just paint the N highest priority tiles if we had an SkPicture).
Comment on attachment 124236 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124236&action=review Can you explain better what the underlying problem(s) with the current prepainting that you're trying to address here are? It seems like there's a few things going on here. Is the overhead on rows and columns and painting too high that you need larger paints? Does prepainting just need more information about what direction to be prepainting in? Is the current prepainting apron of 1 tile just not big enough when scrolling starts? I don't mean to kibbitz here, but all this new code just feels a lot like the prepainting code that already exists. Maybe I'm just wary of adding yet more code for walking tiles in different directions without any tests for any of those functions. I'm really wondering if all of this can be handled by just making idlePaintRect smarter, taking into account scroll prediction. >>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:479 >>> + setScrollPrediction(IntSize()); >> >> It's weird to clear this LayerChromium field in TiledLayerChromium code. Why clear it at all? Don't you want to keep prepainting in the direction of the scroll prediction? > > The underlying idea is to track the amount of scroll since the last paint (that is the prediction for the next scroll), so when we get a chance to paint it should be reset. I could just set it only in WebViewImpl::applyScrollAndScale, but then it won't be set back to zero at the end of a scroll, since there is no scroll of 0,0 when the user stops scrolling. > > If there is another good place to reset it, this could be removed. Maybe LayerChromium::pushPropertiesTo would be a better place. That's in more general layer code and happens prior to processing the scroll deltas. Alternatively, if it's easy to send explicitly send back a prediction of zero at the end of a scroll, that might be clearer too. > Source/WebKit/chromium/src/WebViewImpl.cpp:3069 > +#if OS(ANDROID) > + if (m_nonCompositedContentHost) { > + LayerChromium* layer = m_nonCompositedContentHost->topLevelRootLayer()->platformLayer(); > + layer->setScrollPrediction(layer->scrollPrediction() + scrollDelta); > + } > +#endif This doesn't need to be #ifdef'd. I think just for clarity, I'd prefer that all layers get a scroll prediction if it gets sent, but only some platforms act on it.
(In reply to comment #10) > (From update of attachment 124236 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124236&action=review > > Can you explain better what the underlying problem(s) with the current prepainting that you're trying to address here are? It seems like there's a few things going on here. Is the overhead on rows and columns and painting too high that you need larger paints? Does prepainting just need more information about what direction to be prepainting in? Is the current prepainting apron of 1 tile just not big enough when scrolling starts? > For sure. Here are a few constraints that led me to do it this way: 1.) If you paint anything on the visible paint pass, idle-painting is cancelled. This can happen over an over again, resulting in no idle painting when you are scrolling quickly. It's easy for a single slow paint to cause this. 2.) The memory limit is set the preferred limit before idle-painting. This was important for idle painting because we don't want to paint tiles and then just reclaim them immediately, or chronically push our memory usage to the high limit. For scroll prediction, we don't want to disable it until the max limit is reached. 3.) The current idle painting logic is nice for passive filling in tiles, but for scroll prediction we want to paint as fast as we can, and also need to paint visible tiles if they happen to be dirty. It's tricky to combine the logic while only performing one paint operation. > I don't mean to kibbitz here, but all this new code just feels a lot like the prepainting code that already exists. Maybe I'm just wary of adding yet more code for walking tiles in different directions without any tests for any of those functions. I'm really wondering if all of this can be handled by just making idlePaintRect smarter, taking into account scroll prediction. > I agree there, I was trying to reason how to do this in idle paint pass before moving the code into the main paint pass. I think even more ideal would be to combine all the painting logic into one function again, which I think could be done with some changes to TextureManager (which could also let us increase the pre-paint apron without bad side-effects). I'd be happy to do that first and then land this later? Or land this now and refactor it later. Alternatively, possibly scroll prediction could be squeezed into idle painting. But to solve the problems above I think it will end up being as complicated as now (eg. maybe the main paint could invoke some combined pre-paint logic when scrolling, to avoid 1/2). > >>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:479 > >>> + setScrollPrediction(IntSize()); > >> > >> It's weird to clear this LayerChromium field in TiledLayerChromium code. Why clear it at all? Don't you want to keep prepainting in the direction of the scroll prediction? > > > > The underlying idea is to track the amount of scroll since the last paint (that is the prediction for the next scroll), so when we get a chance to paint it should be reset. I could just set it only in WebViewImpl::applyScrollAndScale, but then it won't be set back to zero at the end of a scroll, since there is no scroll of 0,0 when the user stops scrolling. > > > > If there is another good place to reset it, this could be removed. > > Maybe LayerChromium::pushPropertiesTo would be a better place. That's in more general layer code and happens prior to processing the scroll deltas. Alternatively, if it's easy to send explicitly send back a prediction of zero at the end of a scroll, that might be clearer too. > > > Source/WebKit/chromium/src/WebViewImpl.cpp:3069 > > +#if OS(ANDROID) > > + if (m_nonCompositedContentHost) { > > + LayerChromium* layer = m_nonCompositedContentHost->topLevelRootLayer()->platformLayer(); > > + layer->setScrollPrediction(layer->scrollPrediction() + scrollDelta); > > + } > > +#endif > > This doesn't need to be #ifdef'd. I think just for clarity, I'd prefer that all layers get a scroll prediction if it gets sent, but only some platforms act on it.