Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11044003:
Reduce texture uploads during scrollbar invalidations. (Closed)
|
Created:
8 years, 2 months ago by wjmaclean Modified:
8 years, 2 months ago CC:
chromium-reviews, backer, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionReduce texture uploads during scrollbar invalidations.
BUG=152286
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=159983
Patch Set 1 #Patch Set 2 : Fix DEPS for skia/ext access. #
Total comments: 28
Patch Set 3 : Address review comments. #
Total comments: 5
Patch Set 4 : Use hash instead, don't override updateRect. #
Total comments: 2
Patch Set 5 : Revert use of hash; address texture eviction. #
Total comments: 10
Patch Set 6 : Remove virtual pixelsDidChange, fix nits. #
Total comments: 2
Messages
Total messages: 23 (0 generated)
Here is the bitmap comparison approach as promised. Sorry for the delay, had to re-learn Chromium coding style (may not be perfect, I will apologize in advance).
Some style issues https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... File cc/caching_bitmap_canvas_layer_texture_updater.cc (right): https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.cc:7: #if USE(ACCELERATED_COMPOSITING) no https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.cc:9: #include "caching_bitmap_canvas_layer_texture_updater.h" use full path for chromium-style includes https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.cc:35: { if you want this to be chromium style, the { goes on the the previous line https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... File cc/caching_bitmap_canvas_layer_texture_updater.h (right): https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.h:8: #if USE(ACCELERATED_COMPOSITING) don't do this https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.h:20: virtual void prepareToUpdate(const IntRect& content_rect, what style is this intended to be? In WebKit style, this should be 4-space indented. In chromium style, this should be 2-space indented and the public/private declarations should have a 1-space indent https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.h:25: CCRenderingStats&) OVERRIDE; you need base/compiler_specific.h to use OVERRIDE in chromium style code https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.h:26: virtual void updateTextureRect(CCResourceProvider*, blank line between functions please https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.h:35: bool texture_ueeds_upload_; typo 'ueeds'
Did you consider just caching the pixels directly on either ScrollbarLayerChromium or CCScrollbarLayerImpl rather than hiding it in the LayerTextureUpdater?
https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... File cc/caching_bitmap_canvas_layer_texture_updater.cc (right): https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.cc:46: for (unsigned row = 0; pixels_match && row < num_rows; we know the stride will always match for scrollbar parts so this seems unnecessarily complicated. this should just be one big memcmp()
On 2012/10/02 20:59:15, jamesr wrote: > Did you consider just caching the pixels directly on either > ScrollbarLayerChromium or CCScrollbarLayerImpl rather than hiding it in the > LayerTextureUpdater? I think this approach is cleaner, since it keeps ScrollbarLayerChromium from having to manage and compare three bitmaps. Is there something that this approach makes more complicated that I'm not seeing?
https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... File cc/caching_bitmap_canvas_layer_texture_updater.cc (right): https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.cc:88: if (texture_ueeds_upload_) { style nit: just early out here. https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.cc:98: new_bitmap.deepCopyTo(&cached_bitmap_, new_bitmap.config()); Is there some Skia-specific way to just swap rather than copy? https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... File cc/caching_bitmap_canvas_layer_texture_updater.h (right): https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.h:5: #ifndef CachingBitmapCanvasLayerTextureUpdater_h If this is intended to be Chromium style, this is wrong. https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.h:39: } // namespace cc Two spaces between } and // https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.h:42: #endif // CachingBitmapCanvasLayerTextureUpdater_h Two spaces between #endif and // too.
New patch in a few minutes ... https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... File cc/caching_bitmap_canvas_layer_texture_updater.cc (right): https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.cc:7: #if USE(ACCELERATED_COMPOSITING) On 2012/10/02 20:57:54, jamesr wrote: > no Done. https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.cc:9: #include "caching_bitmap_canvas_layer_texture_updater.h" On 2012/10/02 20:57:54, jamesr wrote: > use full path for chromium-style includes Done. https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.cc:35: { On 2012/10/02 20:57:54, jamesr wrote: > if you want this to be chromium style, the { goes on the the previous line Done. https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.cc:46: for (unsigned row = 0; pixels_match && row < num_rows; On 2012/10/02 21:02:01, jamesr wrote: > we know the stride will always match You mean rowBytes() = width() * bytesPerPixel() always, no padding, right? >for scrollbar parts so this seems > unnecessarily complicated. this should just be one big memcmp() Yeah, I originally coded it that way, but then didn't want to make assumptions that might break down the road. It's much simpler as a single memcmp. https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.cc:88: if (texture_ueeds_upload_) { On 2012/10/02 21:07:18, enne wrote: > style nit: just early out here. Done. https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.cc:98: new_bitmap.deepCopyTo(&cached_bitmap_, new_bitmap.config()); On 2012/10/02 21:07:18, enne wrote: > Is there some Skia-specific way to just swap rather than copy? No, as the SkCanvas will only give me a const ref to the bitmap. I tried to use swap() at first ... at least it only happens on the uploads. https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... File cc/caching_bitmap_canvas_layer_texture_updater.h (right): https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.h:5: #ifndef CachingBitmapCanvasLayerTextureUpdater_h On 2012/10/02 21:07:18, enne wrote: > If this is intended to be Chromium style, this is wrong. Done. https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.h:8: #if USE(ACCELERATED_COMPOSITING) On 2012/10/02 20:57:54, jamesr wrote: > don't do this Done. https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.h:20: virtual void prepareToUpdate(const IntRect& content_rect, On 2012/10/02 20:57:54, jamesr wrote: > what style is this intended to be? In WebKit style, this should be 4-space > indented. In chromium style, this should be 2-space indented and the > public/private declarations should have a 1-space indent It's meant to be Chromium-style, but I've been absent from that for a while and so I'm rusty on it ... will fix these (and any others) ... Done. https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.h:25: CCRenderingStats&) OVERRIDE; On 2012/10/02 20:57:54, jamesr wrote: > you need base/compiler_specific.h to use OVERRIDE in chromium style code Done. https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.h:26: virtual void updateTextureRect(CCResourceProvider*, On 2012/10/02 20:57:54, jamesr wrote: > blank line between functions please Done. https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.h:35: bool texture_ueeds_upload_; On 2012/10/02 20:57:54, jamesr wrote: > typo 'ueeds' Done. https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.h:39: } // namespace cc On 2012/10/02 21:07:18, enne wrote: > Two spaces between } and // Done. https://codereview.chromium.org/11044003/diff/2001/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.h:42: #endif // CachingBitmapCanvasLayerTextureUpdater_h On 2012/10/02 21:07:18, enne wrote: > Two spaces between #endif and // too. Done.
What happens if we upload a part, get the texture evicted, and then go to repaint but paint the exact same part bits? What's making sure we actually do an upload?
http://codereview.chromium.org/11044003/diff/3007/cc/caching_bitmap_canvas_la... File cc/caching_bitmap_canvas_layer_texture_updater.h (right): http://codereview.chromium.org/11044003/diff/3007/cc/caching_bitmap_canvas_la... cc/caching_bitmap_canvas_layer_texture_updater.h:5: #ifndef CACHING_BITMAP_CANVAS_LAYER_TEXTURE_UPDATER_H_ CC_CACHING_BITMAP_CANVAS_LAYER_TEXTURE_UPDATER_H_ http://codereview.chromium.org/11044003/diff/3007/cc/caching_bitmap_canvas_la... cc/caching_bitmap_canvas_layer_texture_updater.h:33: PassOwnPtr<LayerPainterChromium> painter); can you go with scoped_ptr instead of PassOwnPtr? http://codereview.chromium.org/11044003/diff/3007/cc/caching_bitmap_canvas_la... cc/caching_bitmap_canvas_layer_texture_updater.h:37: }; DISALLOW_? http://codereview.chromium.org/11044003/diff/3007/cc/caching_bitmap_canvas_la... cc/caching_bitmap_canvas_layer_texture_updater.h:41: #endif // CACHINGBITMAPCANVASLAYERTEXTUREUPDATER_H_ CC_CACHING_BITMAP_CANVAS_LAYER_TEXTURE_UPDATER_H_
I think this is OK, but the LayerTextureUpdater system is a lot more general than what scrollbar layers actually need. Scrollbar parts aren't tiled or partially painted, so the logic in the tracker has to be more general than we need for this case but is still probably not general enough to work instead of any LayerTextureUpdater. Doing this optimization using inheritance instead of composition also feels pretty funny to me. I think what I'd do is use the normal textureUpdater() and then copy the pixels out to the side in ScrollbarLayerChromium::updatePart() and use that to decide whether to enqueue an upload or not. Another issue with overloading updateTextureRect() to not actually do an upload some of the time is it'll mess up the incremental texture uploading system which will budget for this update even if nothing actually happens. https://codereview.chromium.org/11044003/diff/3007/cc/caching_bitmap_canvas_l... File cc/caching_bitmap_canvas_layer_texture_updater.cc (right): https://codereview.chromium.org/11044003/diff/3007/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.cc:44: new_bitmap.height() != cached_bitmap_.height() || this doesn't quite align
On 2012/10/02 21:50:48, jamesr wrote: > What happens if we upload a part, get the texture evicted, and then go to > repaint but paint the exact same part bits? What's making sure we actually do an > upload? Good point, I hadn't realised this might happen. Is it OK to call backingResourceWasEvicted() on the input texture to determine if an eviction has taken place, and if so proceed with the upload? Will include in a new patch.
OK, this patch doesn't use memcmp, instead it computes a hash signature (kudos backer@ for this suggestion). This eliminates storing the extra bitmap, and the copy (and the extra read may already be in the cache). If the occasional collision occurs in the hash, no biggie. I also no longer override updateRect, so this should solve some problems. Can the other architectural changes happen another day? The branch point is today for M23 beta, and the daisy folks need this soon ... Oh, and I need a better name now for CachingBitmap... any ideas?
I steered you wrong wjmaclean@. I think that the memcmp is neon optimized whereas the base::Hash function is not. The net result is that this patch is 2x slower. My bad. I will glance and see if we have a libc hash (likely to be optimized). On 2012/10/03 15:49:24, wjmaclean wrote: > OK, this patch doesn't use memcmp, instead it computes a hash signature (kudos > backer@ for this suggestion). This eliminates storing the extra bitmap, and the > copy (and the extra read may already be in the cache). If the occasional > collision occurs in the hash, no biggie. > > I also no longer override updateRect, so this should solve some problems. > > Can the other architectural changes happen another day? The branch point is > today for M23 beta, and the daisy folks need this soon ... > > Oh, and I need a better name now for CachingBitmap... any ideas?
On 2012/10/03 16:55:57, jonathan.backer wrote: > I steered you wrong wjmaclean@. I think that the memcmp is neon optimized > whereas the base::Hash function is not. The net result is that this patch is 2x > slower. My bad. I will glance and see if we have a libc hash (likely to be > optimized). Ok. So adler32 from third_party/zlib/zlib.h is much faster than base::Hash. It's comparable to memcmp in this use case. However, this is all moot. BitmapCanvasLayerTextureUpdater::prepareToUpdate is the heavy call here: 80-90%. Presumably for raster. So just use whatever is cleaner.
https://codereview.chromium.org/11044003/diff/6002/cc/ScrollbarLayerChromium.cpp File cc/ScrollbarLayerChromium.cpp (right): https://codereview.chromium.org/11044003/diff/6002/cc/ScrollbarLayerChromium.... cc/ScrollbarLayerChromium.cpp:234: if (!static_cast<CachingBitmapCanvasLayerTextureUpdater*>(painter)->PixelsDidChange()) Please change the signature on updatePart to have the fully derived type rather than adding this static_cast. I think the way to handle evicted textures is to check haveBackingTexture() before you call canAcquireBackingTexture(). If the answer is false, then ignore this PixelsDidChange() early-out.
Here's a new patch. I took out the hash as it was slower than memcmp, and have revised according to enne@'s latest comments. https://codereview.chromium.org/11044003/diff/6002/cc/ScrollbarLayerChromium.cpp File cc/ScrollbarLayerChromium.cpp (right): https://codereview.chromium.org/11044003/diff/6002/cc/ScrollbarLayerChromium.... cc/ScrollbarLayerChromium.cpp:234: if (!static_cast<CachingBitmapCanvasLayerTextureUpdater*>(painter)->PixelsDidChange()) On 2012/10/03 18:05:26, enne wrote: > Please change the signature on updatePart to have the fully derived type rather > than adding this static_cast. Hmm, no, I don't think so. I'd have to change the member variable declarations in ScrollbarLayerChromium to be of the derived type to avoid the cast, or am I misunderstanding what you wanted here? Perhaps I can add a virtual to LayerTextureUpdater? > I think the way to handle evicted textures is to check haveBackingTexture() > before you call canAcquireBackingTexture(). If the answer is false, then ignore > this PixelsDidChange() early-out. OK. I thought by early-outing in update part I wouldn't need to worry about evicted textures, but I guess that doesn't make sense. Will do this.
https://codereview.chromium.org/11044003/diff/1015/cc/LayerTextureUpdater.h File cc/LayerTextureUpdater.h (right): https://codereview.chromium.org/11044003/diff/1015/cc/LayerTextureUpdater.h#n... cc/LayerTextureUpdater.h:64: virtual bool pixelsDidChange() const; this is unnecessary. Keep the more specific type in ScrollbarLayerChromium
lgtm with those fixes. Enne? https://codereview.chromium.org/11044003/diff/1015/cc/caching_bitmap_canvas_l... File cc/caching_bitmap_canvas_layer_texture_updater.h (right): https://codereview.chromium.org/11044003/diff/1015/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.h:29: CachingBitmapCanvasLayerTextureUpdater( explicit
This is really close, in my opinion. https://codereview.chromium.org/11044003/diff/1015/cc/LayerTextureUpdater.h File cc/LayerTextureUpdater.h (right): https://codereview.chromium.org/11044003/diff/1015/cc/LayerTextureUpdater.h#n... cc/LayerTextureUpdater.h:64: virtual bool pixelsDidChange() const; Please don't do this. updatePart is a non-virtual member function on ScrollbarLayerChromium. There are no derived classes from ScrollbarLayerChromium. There's no impact other than to ScrollbarLayerChromium to change the member variable types and the updatePart signature to the derived updater type. I think that's a lot cleaner than exposing this to all updaters. https://codereview.chromium.org/11044003/diff/1015/cc/caching_bitmap_canvas_l... File cc/caching_bitmap_canvas_layer_texture_updater.cc (right): https://codereview.chromium.org/11044003/diff/1015/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.cc:59: } // namespace cc } // namespace cc https://codereview.chromium.org/11044003/diff/1015/cc/caching_bitmap_canvas_l... File cc/caching_bitmap_canvas_layer_texture_updater.h (right): https://codereview.chromium.org/11044003/diff/1015/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.h:38: #endif // CACHINGBITMAPCANVASLAYERTEXTUREUPDATER_H_ CACHING_BITMAP_CANVAS_LAYER_TEXTURE_UPDATER_H_
I think I've addressed all the comments, ptal. https://codereview.chromium.org/11044003/diff/1015/cc/LayerTextureUpdater.h File cc/LayerTextureUpdater.h (right): https://codereview.chromium.org/11044003/diff/1015/cc/LayerTextureUpdater.h#n... cc/LayerTextureUpdater.h:64: virtual bool pixelsDidChange() const; On 2012/10/03 19:31:43, enne wrote: > Please don't do this. > > updatePart is a non-virtual member function on ScrollbarLayerChromium. There > are no derived classes from ScrollbarLayerChromium. There's no impact other > than to ScrollbarLayerChromium to change the member variable types and the > updatePart signature to the derived updater type. I think that's a lot cleaner > than exposing this to all updaters. Done. NP, I just wasn't sure if you'd want me to change the member var type on SLC. https://codereview.chromium.org/11044003/diff/1015/cc/LayerTextureUpdater.h#n... cc/LayerTextureUpdater.h:64: virtual bool pixelsDidChange() const; On 2012/10/03 19:28:02, jamesr wrote: > this is unnecessary. Keep the more specific type in ScrollbarLayerChromium Done. https://codereview.chromium.org/11044003/diff/1015/cc/caching_bitmap_canvas_l... File cc/caching_bitmap_canvas_layer_texture_updater.cc (right): https://codereview.chromium.org/11044003/diff/1015/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.cc:59: } // namespace cc On 2012/10/03 19:31:43, enne wrote: > } // namespace cc Done. https://codereview.chromium.org/11044003/diff/1015/cc/caching_bitmap_canvas_l... File cc/caching_bitmap_canvas_layer_texture_updater.h (right): https://codereview.chromium.org/11044003/diff/1015/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.h:29: CachingBitmapCanvasLayerTextureUpdater( On 2012/10/03 19:29:13, jamesr wrote: > explicit Done. https://codereview.chromium.org/11044003/diff/1015/cc/caching_bitmap_canvas_l... cc/caching_bitmap_canvas_layer_texture_updater.h:38: #endif // CACHINGBITMAPCANVASLAYERTEXTUREUPDATER_H_ On 2012/10/03 19:31:43, enne wrote: > CACHING_BITMAP_CANVAS_LAYER_TEXTURE_UPDATER_H_ Done.
lgtm https://codereview.chromium.org/11044003/diff/16002/cc/ScrollbarLayerChromium... File cc/ScrollbarLayerChromium.cpp (right): https://codereview.chromium.org/11044003/diff/16002/cc/ScrollbarLayerChromium... cc/ScrollbarLayerChromium.cpp:233: return; place add a TRACE_INSTANT0("cc", ...) here so we can know when we're hitting (and thus not hitting) this optimization in the wild
lgtm
CL dcommitted with TRACE added. https://codereview.chromium.org/11044003/diff/16002/cc/ScrollbarLayerChromium... File cc/ScrollbarLayerChromium.cpp (right): https://codereview.chromium.org/11044003/diff/16002/cc/ScrollbarLayerChromium... cc/ScrollbarLayerChromium.cpp:233: return; On 2012/10/03 20:22:33, jamesr wrote: > place add a TRACE_INSTANT0("cc", ...) here so we can know when we're hitting > (and thus not hitting) this optimization in the wild Good plan - done! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
